Make ChromeVox cursor robust to deleted nodes

chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js

Index: chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js b/chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
index 35ebe6d51e6f4d4b780e740c7467e6789561f31a..b0ec3b35c1a9dd94a93708ab00fd0f71b364cdfa 100644
--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
@@ -71,10 +71,17 @@ var Unit = cursors.Unit;
  * is pointed to and covers the case where the accessible text is empty.
  */
 cursors.Cursor = function(node, index) {
-  /** @type {!AutomationNode} @private */
-  this.node_ = node;
   /** @type {number} @private */
   this.index_ = index;
+  /** @type {Array<AutomationNode>} @private */
+  this.ancestry_ = [];
+  var nodeWalker = node;
+  while (nodeWalker) {
+    this.ancestry_.push(nodeWalker);
+    nodeWalker = nodeWalker.parent;
+    if (nodeWalker == node.root)
+      break;
+  }
 };
 
 /**
@@ -93,15 +100,24 @@ cursors.Cursor.prototype = {
    * @return {boolean}
    */
   equals: function(rhs) {
-    return this.node_ === rhs.node &&
-        this.index_ === rhs.index;
+    return this.node === rhs.node &&
+        this.index === rhs.index;
   },
 
   /**
-   * @return {!AutomationNode}
+   * Returns the node. If the node is invalid since the last time it
+   * was accessed, moves the cursor to the nearest valid ancestor first.
+   * @return {?AutomationNode}

[David Tseng, 2016/05/25 20:58:10]

You don't need the '?' (objects are nullable by default).

[dmazzoni, 2016/06/01 16:36:50]

Done.

    */
   get node() {
-    return this.node_;
+    for (var index = 0; index < this.ancestry_.length; index++) {

[David Tseng, 2016/05/25 20:58:10]

The dual use of index here is confusing. Maybe use for (let x of y).

[dmazzoni, 2016/06/01 16:36:50]

Fixed, but the Closure compiler doesn't let us use ES6
syntax as currently configured so I kept it a simple
loop for now. Should we enable ES6 in a follow-on change?

+      var foo = this.ancestry_[index];

[David Tseng, 2016/05/25 20:58:10]

How about firstValidNode?

[dmazzoni, 2016/06/01 16:36:50]

Acknowledged.

+      if (foo != null && foo.role !== undefined)

[David Tseng, 2016/05/25 20:58:10]

This won't work. There are nodes that have a role but have no root; check for
the presence of root as well.

[dmazzoni, 2016/06/01 16:36:50]

Done.

+        return foo;

[David Tseng, 2016/05/25 20:58:10]

Do you need to do bookkeeping on the ancestry chain?

[dmazzoni, 2016/06/01 16:36:50]

I don't think so, because this object is basically immutable - if you call it
again it will search again, but as soon as you move to another node, it creates
a new Cursor with a new ancestry.

+      // If we have to walk up to an ancestor, reset the index to zero.
+      this.index_ = 0;

[David Tseng, 2016/05/25 20:58:10]

Reset to cursors.NODE_INDEX

[dmazzoni, 2016/06/01 16:36:50]

Thanks, that made it work better.

+    }
+    return null;
   },
 
   /**
@@ -141,7 +157,7 @@ cursors.Cursor.prototype = {
    * @return {string}
    */
   getText: function(opt_node) {
-    var node = opt_node || this.node_;
+    var node = opt_node || this.node;
     if (node.role === RoleType.textField)
       return node.value;
     return node.name || '';
@@ -156,7 +172,11 @@ cursors.Cursor.prototype = {
    * @return {!cursors.Cursor} The moved cursor.
    */
   move: function(unit, movement, dir) {
-    var newNode = this.node_;
+    var originalNode = this.node;
+    if (!originalNode)
+      return this;
+
+    var newNode = originalNode;
     var newIndex = this.index_;
 
     if ((unit != Unit.NODE || unit != Unit.DOM_NODE) &&
@@ -257,7 +277,7 @@ cursors.Cursor.prototype = {
             var pred = unit == Unit.NODE ?
                 AutomationPredicate.leaf : AutomationPredicate.element;
             newNode = AutomationUtil.findNextNode(
-                newNode, dir, pred) || this.node_;
+                newNode, dir, pred) || originalNode;
             newIndex = cursors.NODE_INDEX;
             break;
         }
@@ -268,7 +288,7 @@ cursors.Cursor.prototype = {
           case Movement.BOUND:
             newNode = AutomationUtil.findNodeUntil(newNode, dir,
                 AutomationPredicate.linebreak, true);
-            newNode = newNode || this.node_;
+            newNode = newNode || originalNode;
             newIndex =
                 dir == Dir.FORWARD ? this.getText(newNode).length : 0;
             break;
@@ -281,7 +301,7 @@ cursors.Cursor.prototype = {
       default:
         throw Error('Unrecognized unit: ' + unit);
     }
-    newNode = newNode || this.node_;
+    newNode = newNode || originalNode;
     newIndex = goog.isDef(newIndex) ? newIndex : this.index_;
     return new cursors.Cursor(newNode, newIndex);
   },
@@ -324,6 +344,8 @@ cursors.WrappingCursor.prototype = {
   /** @override */
   move: function(unit, movement, dir) {
     var result = this;
+    if (!result.node)
+      return this;
 
     // Regular movement.
     if (!AutomationUtil.isTraversalRoot(this.node) || dir == Dir.FORWARD)
@@ -339,6 +361,8 @@ cursors.WrappingCursor.prototype = {
       var pred = unit == Unit.DOM_NODE ?
           AutomationPredicate.element : AutomationPredicate.leaf;
       var endpoint = this.node;
+      if (!endpoint)
+        return this;
 
       // Case 1: forwards (find the root-like node).
       while (!AutomationUtil.isTraversalRoot(endpoint) && endpoint.parent)
@@ -392,6 +416,10 @@ cursors.Range.getDirection = function(rangeA, rangeB) {
   if (!rangeA || !rangeB)
     return Dir.FORWARD;
 
+  if (!rangeA.start.node || !rangeA.end.node ||
+      !rangeB.start.node || !rangeB.end.node)
+    return Dir.FORWARD;
+
   // They are the same range.
   if (rangeA.start.node === rangeB.start.node &&
       rangeB.end.node === rangeA.end.node)
@@ -471,6 +499,9 @@ cursors.Range.prototype = {
    */
   move: function(unit, dir) {
     var newStart = this.start_;
+    if (!newStart.node)
+      return this;
+
     var newEnd;
     switch (unit) {
       case Unit.CHARACTER:
@@ -504,6 +535,9 @@ cursors.Range.prototype = {
     var start = this.start.node;
     var end = this.end.node;
 
+    if (!start || !end)
+      return;
+
     // Find the most common root.
     var uniqueAncestors = AutomationUtil.getUniqueAncestors(start, end);
     var mcr = start.root;