Make ChromeVox cursor robust to deleted nodes

If a node becomes invalid, the cursor snaps to the nearest valid ancestor.
Fixing this uncovered a lot of places where we were assuming that a cursor's
node must be valid, but it's possible that even with this new check that
a cursor could still be invalid, so I added checks in a lot of places.

BUG=613694
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2007183002
Cr-Commit-Position: refs/heads/master@{#397860}

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

@@ -769,22 +769,25 @@ Background.prototype = {
     }
 
     if (pred) {
-      var node = AutomationUtil.findNextNode(
-          current.getBound(dir).node, dir, pred, {skipInitialAncestry: true});
-
-      if (node) {
-        node = AutomationUtil.findNodePre(
-            node, dir, AutomationPredicate.object) || node;
-      }
+      var bound = current.getBound(dir).node;
+      if (bound) {
+        var node = AutomationUtil.findNextNode(
+            bound, dir, pred, {skipInitialAncestry: true});
+
+        if (node) {
+          node = AutomationUtil.findNodePre(
+              node, dir, AutomationPredicate.object) || node;
+        }
 
-      if (node) {
-        current = cursors.Range.fromNode(node);
-      } else {
-        if (predErrorMsg) {
-          cvox.ChromeVox.tts.speak(Msgs.getMsg(predErrorMsg),
-                                   cvox.QueueMode.FLUSH);
+        if (node) {
+          current = cursors.Range.fromNode(node);
+        } else {
+          if (predErrorMsg) {
+            cvox.ChromeVox.tts.speak(Msgs.getMsg(predErrorMsg),
+                                     cvox.QueueMode.FLUSH);
+          }
+          return false;
         }
-        return false;
       }
     }
 

chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs

@@ -24,6 +24,7 @@ BackgroundTest.prototype = {
   setUp: function() {
     global.backgroundObj.forceChromeVoxNextActive();
     window.RoleType = chrome.automation.RoleType;
+    window.doCmd = this.doCmd;
   },
 
   /**
@@ -74,6 +75,33 @@ BackgroundTest.prototype = {
     <iframe srcdoc="<button>Inside</button><h1>Inside</h1>"></iframe>
     <button>After</button>
   */},
+
+  disappearingObjectDoc: function() {/*!
+    <p>start</p>
+    <article>
+      <p>Before1</p>
+      <p>Before2</p>
+      <p>Before3</p>
+    </article>
+    <article>
+      <p id="disappearing">Disappearing</p>
+    </article>
+    <article>
+      <p>After1</p>
+      <p>After2</p>
+      <p>After3</p>
+    </article>
+    </div>
+    <div id="live" aria-live="polite"></div>
+    <div id="delete" role="button">Delete</div>
+    <script>
+      document.getElementById('delete').addEventListener('click', function() {
+        var d = document.getElementById('disappearing');
+        d.parentElement.removeChild(d);
+        document.getElementById('live').innerText = 'Deleted';
+      });
+    </script>
+  */},
 };
 
 /** Tests that ChromeVox classic is in this context. */
@@ -91,8 +119,6 @@ SYNC_TEST_F('BackgroundTest', 'NextNamespaces', function() {
 TEST_F('BackgroundTest', 'ForwardBackwardNavigation', function() {
   var mockFeedback = this.createMockFeedback();
   this.runWithLoadedTree(this.linksAndHeadingsDoc, function() {
-    var doCmd = this.doCmd.bind(this);
-
     mockFeedback.expectSpeech('start').expectBraille('start');
 
     mockFeedback.call(doCmd('nextLink'))
@@ -151,8 +177,6 @@ TEST_F('BackgroundTest', 'CaretNavigation', function() {
   // TODO(plundblad): Add braille expectaions when crbug.com/523285 is fixed.
   var mockFeedback = this.createMockFeedback();
   this.runWithLoadedTree(this.linksAndHeadingsDoc, function() {
-    var doCmd = this.doCmd.bind(this);
-
     mockFeedback.expectSpeech('start');
     mockFeedback.call(doCmd('nextCharacter'))
         .expectSpeech('t');
@@ -213,7 +237,7 @@ TEST_F('BackgroundTest', 'ContinuousRead', function() {
   var mockFeedback = this.createMockFeedback();
   this.runWithLoadedTree(this.linksAndHeadingsDoc, function() {
     mockFeedback.expectSpeech('start')
-        .call(this.doCmd('readFromHere'))
+        .call(doCmd('readFromHere'))
         .expectSpeech(
             'start',
             'alpha', 'Link',
@@ -239,7 +263,7 @@ TEST_F('BackgroundTest', 'AriaLabel', function() {
   var mockFeedback = this.createMockFeedback();
   this.runWithLoadedTree('<a aria-label="foo" href="a">a</a>',
     function(rootNode) {
-      rootNode.find({role: 'link'}).focus();
+      rootNode.find({role: RoleType.link}).focus();
       mockFeedback.expectSpeech('foo')
           .expectSpeech('Link')
           .expectBraille('foo lnk');
@@ -263,7 +287,7 @@ TEST_F('BackgroundTest', 'ShowContextMenu', function() {
 
       var go = rootNode.find({ role: RoleType.link });
       this.listenOnce(go, 'focus', function(e) {
-        this.doCmd('contextMenu')();
+        doCmd('contextMenu')();
       }.bind(this), true);
       go.focus();
     }.bind(this));
@@ -293,7 +317,7 @@ TEST_F('BackgroundTest', 'BrailleRouting', function() {
       */},
       function(rootNode) {
         var button1 = rootNode.find({role: RoleType.button,
-                                     name: 'Click me'});
+                                     attributes: { name: 'Click me' }});
         var textField = rootNode.find(
             {role: RoleType.textField});
         mockFeedback.expectBraille('start')
@@ -379,8 +403,6 @@ TEST_F('BackgroundTest', 'EarconsForControls', function() {
       <input type=range value=5>
     */},
     function(rootNode) {
-      var doCmd = this.doCmd.bind(this);
-
       mockFeedback.call(doCmd('nextObject'))
           .expectSpeech('MyLink')
           .expectEarcon(cvox.Earcon.LINK)
@@ -450,7 +472,9 @@ TEST_F('BackgroundTest', 'ActiveOrInactive', function() {
       <input type="text"></input>
     */},
     function(rootNode) {
-      var focusButton = function() { rootNode.find({role: 'button'}).focus(); };
+      var focusButton = function() {
+        rootNode.find({role: RoleType.button}).focus();
+      };
       var on = function() { cvox.ChromeVox.isActive = true; };
       var off = function() { cvox.ChromeVox.isActive = false; };
 
@@ -467,8 +491,10 @@ TEST_F('BackgroundTest', 'ActiveOrInactive', function() {
       mockFeedback.call(focusButton)
           .expectSpeech('b').expectSpeech('Button')
           .call(off)
-          .call(focusThen.bind(this, rootNode.find({ role: 'link' }), on))
-          .call(focusThen.bind(this, rootNode.find({ role: 'textField' })))
+          .call(focusThen.bind(this, rootNode.find(
+              { role: RoleType.link }), on))
+          .call(focusThen.bind(this, rootNode.find(
+              { role: RoleType.textField })))
           .expectNextSpeechUtteranceIsNot('a')
           .expectSpeech('Edit text');
 
@@ -541,8 +567,8 @@ TEST_F('BackgroundTest', 'FocusIframe', function() {
     <iframe tabindex=0 src="data:text/html,<p>Inside</p>"></iframe>
     <button>outside</button>
   */}, function(root) {
-    var iframe = root.find({role: 'iframe'});
-    var button = root.find({role: 'button'});
+    var iframe = root.find({role: RoleType.iframe});
+    var button = root.find({role: RoleType.button});
 
     assertEquals('iframe', iframe.role);
     assertEquals('button', button.role);
@@ -572,8 +598,8 @@ TEST_F('BackgroundTest', 'NoisySlider', function() {
       update();
     </script>
   */}, function(root) {
-    var go = root.find({role: 'button'});
-    var slider = root.find({role: 'slider'});
+    var go = root.find({role: RoleType.button});
+    var slider = root.find({role: RoleType.slider});
     var focusButton = go.focus.bind(go);
     var focusSlider = slider.focus.bind(slider);
     mockFeedback.call(focusButton)
@@ -601,7 +627,7 @@ TEST_F('BackgroundTest', 'Checkbox', function() {
       });
     </script>
   */}, function(root) {
-    var cbx = root.find({role: 'checkBox'});
+    var cbx = root.find({role: RoleType.checkBox});
     var click = cbx.doDefault.bind(cbx);
     mockFeedback.call(click)
         .expectSpeech('go')
@@ -625,14 +651,12 @@ TEST_F('BackgroundTest', 'ForwardNavigationThroughIframeButtons', function() {
       return;
 
     // Return if the iframe hasn't loaded yet.
-    var iframe = rootNode.find({role: 'iframe'});
+    var iframe = rootNode.find({role: RoleType.iframe});
     var childDoc = iframe.firstChild;
     if (!childDoc || childDoc.children.length == 0)
       return;
 
     running = true;
-    var doCmd = this.doCmd.bind(this);
-
     var beforeButton = rootNode.find({role: RoleType.button,
                                       name: 'Before'});
     beforeButton.focus();
@@ -675,8 +699,6 @@ TEST_F('BackgroundTest', 'ForwardObjectNavigationThroughIframes', function() {
       return;
 
     running = true;
-    var doCmd = this.doCmd.bind(this);
-
     var beforeButton = rootNode.find({role: RoleType.button,
                                       name: 'Before'});
     beforeButton.focus();
@@ -716,7 +738,7 @@ TEST_F('BackgroundTest', 'SelectOptionSelected', function() {
       <option>grapefruit
     </select>
   */}, function(root) {
-    var select = root.find({role: 'popUpButton'});
+    var select = root.find({role: RoleType.popUpButton});
     var clickSelect = select.doDefault.bind(select);
     var lastOption = select.lastChild.lastChild;
     var selectLastOption = lastOption.doDefault.bind(lastOption);
@@ -764,8 +786,8 @@ TEST_F('BackgroundTest', 'EditText', function() {
     <input type="text"></input>
     <input role="combobox" type="text"></input>
   */}, function(root) {
-    var nextEditText = this.doCmd('nextEditText');
-    var previousEditText = this.doCmd('previousEditText');
+    var nextEditText = doCmd('nextEditText');
+    var previousEditText = doCmd('previousEditText');
     mockFeedback.call(nextEditText)
         .expectSpeech('Combo box')
         .call(previousEditText)
@@ -803,3 +825,35 @@ TEST_F('BackgroundTest', 'BackwardForwardSync', function() {
         .replay();
   });
 });
+
+/** Tests that navigation works when the current object disappears. */
+TEST_F('BackgroundTest', 'DisappearingObject', function() {
+  var mockFeedback = this.createMockFeedback();
+  this.runWithLoadedTree(this.disappearingObjectDoc, function(rootNode) {
+    var deleteButton = rootNode.find({role: RoleType.button,
+                                      attributes: { name: 'Delete' }});
+    var pressDelete = deleteButton.doDefault.bind(deleteButton);
+    mockFeedback.expectSpeech('start').expectBraille('start');
+
+    mockFeedback.call(doCmd('nextObject'))
+        .expectSpeech('Before1')
+        .call(doCmd('nextObject'))
+        .expectSpeech('Before2')
+        .call(doCmd('nextObject'))
+        .expectSpeech('Before3')
+        .call(doCmd('nextObject'))
+        .expectSpeech('Disappearing')
+        .call(pressDelete)
+        .expectSpeech('Deleted')
+        .call(doCmd('nextObject'))
+        .expectSpeech('After1')
+        .call(doCmd('nextObject'))
+        .expectSpeech('After2')
+        .call(doCmd('previousObject'))
+        .expectSpeech('After1')
+        .call(doCmd('previousObject'))
+        .expectSpeech('Before3');
+
+    mockFeedback.replay();
+  });
+});

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

@@ -12,6 +12,7 @@ goog.provide('cursors.Movement');
 goog.provide('cursors.Range');
 goog.provide('cursors.Unit');
 
+goog.require('AutomationPredicate');
 goog.require('AutomationUtil');
 goog.require('StringUtil');
 goog.require('constants');
@@ -71,10 +72,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 && AutomationPredicate.root(nodeWalker))
+      break;
+  }
 };
 
 /**
@@ -93,15 +101,26 @@ 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}
    */
   get node() {
-    return this.node_;
+    for (var i = 0; i < this.ancestry_.length; i++) {
+      var firstValidNode = this.ancestry_[i];
+      if (firstValidNode != null && firstValidNode.role !== undefined &&
+          firstValidNode.root !== undefined) {
+        return firstValidNode;
+      }
+      // If we have to walk up to an ancestor, reset the index to NODE_INDEX.
+      this.index_ = cursors.NODE_INDEX;
+    }
+    return null;
   },
 
   /**
@@ -141,7 +160,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 +175,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 +280,7 @@ cursors.Cursor.prototype = {
             var pred = unit == Unit.NODE ?
                 AutomationPredicate.leaf : AutomationPredicate.object;
             newNode = AutomationUtil.findNextNode(
-                newNode, dir, pred) || this.node_;
+                newNode, dir, pred) || originalNode;
             newIndex = cursors.NODE_INDEX;
             break;
         }
@@ -268,7 +291,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 +304,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);
   },
@@ -291,7 +314,7 @@ cursors.Cursor.prototype = {
    * @return {boolean}
    */
   isValid: function() {
-    return !!this.node.root;
+    return !!this.node && !!this.node.root;
   }
 };
 
@@ -324,6 +347,8 @@ cursors.WrappingCursor.prototype = {
   /** @override */
   move: function(unit, movement, dir) {
     var result = this;
+    if (!result.node)
+      return this;
 
     // Regular movement.
     if (!AutomationPredicate.root(this.node) || dir == Dir.FORWARD)
@@ -340,6 +365,8 @@ cursors.WrappingCursor.prototype = {
       var pred = unit == Unit.DOM_NODE ?
           AutomationPredicate.object : AutomationPredicate.leaf;
       var endpoint = this.node;
+      if (!endpoint)
+        return this;
 
       // Case 1: forwards (find the root-like node).
       while (!AutomationPredicate.root(endpoint) && endpoint.parent)
@@ -349,7 +376,7 @@ cursors.WrappingCursor.prototype = {
       var playEarcon = dir == Dir.FORWARD;
 
       // Case 2: backward (sync downwards to a leaf), if already on the root.
-      if (dir == Dir.BACKWARD && endpoint == this.node_) {
+      if (dir == Dir.BACKWARD && endpoint == this.node) {
         playEarcon = true;
         endpoint = AutomationUtil.findNodePre(endpoint,
             dir,
@@ -403,6 +430,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)
@@ -482,6 +513,9 @@ cursors.Range.prototype = {
    */
   move: function(unit, dir) {
     var newStart = this.start_;
+    if (!newStart.node)
+      return this;
+
     var newEnd;
     switch (unit) {
       case Unit.CHARACTER:
@@ -515,6 +549,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;

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

@@ -1254,6 +1254,9 @@ Output.prototype = {
    * @private
    */
   range_: function(range, prevRange, type, rangeBuff) {
+    if (!range.start.node || !range.end.node)
+      return;
+
     if (!prevRange)
       prevRange = cursors.Range.fromNode(range.start.node.root);
     var cursor = cursors.Cursor.fromNode(range.start.node);
@@ -1386,6 +1389,9 @@ Output.prototype = {
     var dir = cursors.Range.getDirection(prevRange, range);
     var node = range.start.node;
     var prevNode = prevRange.getBound(dir).node;
+    if (!node || !prevNode)
+      return;
+
     var options = {annotation: ['name'], isUnique: true};
     var startIndex = range.start.index;
     var endIndex = range.end.index;