Bug 1345529 - fix inspector DocumentWaler children() method;r=pbro

The inspector's DocumentWalker had several issues when trying to retrieve
children for a given node, especially if the starting node was filtered
out by the filter function of the walker.

If the starting node was provided by options.center or options.start
and if this starting node was filtered out by the walker's filter
then the walker would fallback to the first valid parent of this node.

eg with
parent1 > parent2 > [valid-node, invalid-node, valid-node]

When asking for the children of parent2, if the walker started on
"invalid-node", then the walker would instead use parent2 and in turn
we would retrieve the children of parent 1

To fix that we can either tell the walker wether it should fallback to a
sibling of the starting node or to a parent, or make sure that the nodes
provided to the walker are valid.

A second issue was with the utility methods _readForward and _readBackward.
They both use the next/previousSibling() methods of a walker in order to
collect all the valid siblings of the walker's current node. But they were
always including the current node of the walker in their return array. And
there is no guarantee that the walker's currentNode is actually valid for it's
filter.

eg with a walker containing [invalid-node-1, invalid-node-2, valid-node].
Let's say the walker is currently on valid-node and we call previousSibling
The walker will do 3 steps:
- this.walker.previousSibling() > returns invalid-node-2, fails filtering
- this.walker.previousSibling() > returns invalid-node-1, fails filtering
- this.walker.previousSibling() > returns null, stop looping and return null

But at this stage the internal walker still points to the last visited node
(invalid-node-1). So if _readForward/Backward blindly add the current node
of the walker, we might be returning invalid nodes.

MozReview-Commit-ID: 72Be7DP5ky6

--HG--
extra : rebase_source : 31e7d3321abef04243b741196d4ca6279cefd53a

devtools/client/inspector/markup/test/browser.ini

@@ -148,6 +148,7 @@ skip-if = (os == 'linux' && bits == 32 && debug) # bug 1328915, disable linux32
 [browser_markup_remove_xul_attributes.js]
 skip-if = e10s # Bug 1036409 - The last selected node isn't reselected
 [browser_markup_search_01.js]
+[browser_markup_tag_delete_whitespace_node.js]
 [browser_markup_tag_edit_01.js]
 [browser_markup_tag_edit_02.js]
 [browser_markup_tag_edit_03.js]

devtools/client/inspector/markup/test/browser_markup_tag_delete_whitespace_node.js

@@ -0,0 +1,51 @@
+/* vim: set ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// After deleting a node, whitespace siblings that had an impact on the layout might no
+// longer have any impact. This tests that the markup view is correctly rendered after
+// deleting a node that triggers such a change.
+
+const HTML =
+  `<div>
+    <p id="container">
+      <span>1</span>      <span id="after-whitespace">2</span>
+    </p>
+  </div>`;
+
+const TEST_URL = "data:text/html;charset=utf-8," + encodeURIComponent(HTML);
+
+add_task(function* () {
+  let {inspector} = yield openInspectorForURL(TEST_URL);
+
+  info("Test deleting a node that will modify the whitespace nodes rendered in the " +
+    "markup view.");
+
+  info("Select node #after-whitespace and make sure it is focused");
+  yield selectNode("#after-whitespace", inspector);
+  yield clickContainer("#after-whitespace", inspector);
+
+  info("Delete the node with the delete key");
+  let mutated = inspector.once("markupmutation");
+  EventUtils.sendKey("delete", inspector.panelWin);
+  yield Promise.all([mutated, inspector.once("inspector-updated")]);
+
+  // TODO: There is still an issue with selection here.  When the span is deleted, the
+  // selection goes to text-node. But since the text-node gets removed from the markup
+  // view after losing its impact on the layout, the selection remains on a node which
+  // is no longer part of the markup view (but still a valid node in the content DOM).
+  let parentNodeFront = yield inspector.selection.nodeFront.parentNode();
+  let nodeFront = yield getNodeFront("#container", inspector);
+  is(parentNodeFront, nodeFront, "Selection is as expected after deletion");
+
+  info("Check that the node was really removed");
+  let node = yield getNodeFront("#after-whitespace", inspector);
+  ok(!node, "The node can't be found in the page anymore");
+
+  info("Undo the deletion to restore the original markup");
+  yield undoChange(inspector);
+  node = yield getNodeFront("#after-whitespace", inspector);
+  ok(node, "The node is back");
+});

devtools/server/actors/inspector.js

@@ -90,6 +90,11 @@ const XHTML_NS = "http://www.w3.org/1999/xhtml";
 const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
 const IMAGE_FETCHING_TIMEOUT = 500;
 
+// SKIP_TO_* arguments are used with the DocumentWalker, driving the strategy to use if
+// the starting node is incompatible with the filter function of the walker.
+const SKIP_TO_PARENT = "SKIP_TO_PARENT";
+const SKIP_TO_SIBLING = "SKIP_TO_SIBLING";
+
 // The possible completions to a ':' with added score to give certain values
 // some preference.
 const PSEUDO_SELECTORS = [
@@ -934,12 +939,12 @@ var WalkerActor = protocol.ActorClassWithSpec(walkerSpec, {
     return "[WalkerActor " + this.actorID + "]";
   },
 
-  getDocumentWalker: function (node, whatToShow) {
+  getDocumentWalker: function (node, whatToShow, skipTo) {
     // Allow native anon content (like <video> controls) if preffed on
     let nodeFilter = this.showAllAnonymousContent
-                        ? allAnonymousContentTreeWalkerFilter
-                        : standardTreeWalkerFilter;
-    return new DocumentWalker(node, this.rootWin, whatToShow, nodeFilter);
+                    ? allAnonymousContentTreeWalkerFilter
+                    : standardTreeWalkerFilter;
+    return new DocumentWalker(node, this.rootWin, whatToShow, nodeFilter, skipTo);
   },
 
   destroy: function () {
@@ -1370,7 +1375,10 @@ var WalkerActor = protocol.ActorClassWithSpec(walkerSpec, {
     // We're going to create a few document walkers with the same filter,
     // make it easier.
     let getFilteredWalker = documentWalkerNode => {
-      return this.getDocumentWalker(documentWalkerNode, options.whatToShow);
+      let { whatToShow } = options;
+      // Use SKIP_TO_SIBLING to force the walker to use a sibling of the provided node
+      // in case this one is incompatible with the walker's filter function.
+      return this.getDocumentWalker(documentWalkerNode, whatToShow, SKIP_TO_SIBLING);
     };
 
     // Need to know the first and last child.
@@ -1396,7 +1404,7 @@ var WalkerActor = protocol.ActorClassWithSpec(walkerSpec, {
 
     // Start by reading backward from the starting point if we're centering...
     let backwardWalker = getFilteredWalker(start);
-    if (start != firstChild && options.center) {
+    if (backwardWalker.currentNode != firstChild && options.center) {
       backwardWalker.previousSibling();
       let backwardCount = Math.floor(maxNodes / 2);
       let backwardNodes = this._readBackward(backwardWalker, backwardCount);
@@ -1518,9 +1526,14 @@ var WalkerActor = protocol.ActorClassWithSpec(walkerSpec, {
    */
   _readForward: function (walker, count) {
     let ret = [];
+
     let node = walker.currentNode;
     do {
-      ret.push(this._ref(node));
+      if (!walker.isSkippedNode(node)) {
+        // The walker can be on a node that would be filtered out if it didn't find any
+        // other node to fallback to.
+        ret.push(this._ref(node));
+      }
       node = walker.nextSibling();
     } while (node && --count);
     return ret;
@@ -1532,9 +1545,14 @@ var WalkerActor = protocol.ActorClassWithSpec(walkerSpec, {
    */
   _readBackward: function (walker, count) {
     let ret = [];
+
     let node = walker.currentNode;
     do {
-      ret.push(this._ref(node));
+      if (!walker.isSkippedNode(node)) {
+        // The walker can be on a node that would be filtered out if it didn't find any
+        // other node to fallback to.
+        ret.push(this._ref(node));
+      }
       node = walker.previousSibling();
     } while (node && --count);
     ret.reverse();
@@ -2968,14 +2986,20 @@ function isNodeDead(node) {
  *
  * @param {DOMNode} node
  * @param {Window} rootWin
- * @param {Int} whatToShow See nodeFilterConstants / inIDeepTreeWalker for
- * options.
- * @param {Function} filter A custom filter function Taking in a DOMNode
- *        and returning an Int. See WalkerActor.nodeFilter for an example.
+ * @param {Number} whatToShow
+ *        See nodeFilterConstants / inIDeepTreeWalker for options.
+ * @param {Function} filter
+ *        A custom filter function Taking in a DOMNode and returning an Int. See
+ *        WalkerActor.nodeFilter for an example.
+ * @param {String} skipTo
+ *        Either SKIP_TO_PARENT or SKIP_TO_SIBLING. If the provided node is not compatible
+ *        with the filter function for this walker, try to find a compatible one either
+ *        in the parents or in the siblings of the node.
  */
 function DocumentWalker(node, rootWin,
     whatToShow = nodeFilterConstants.SHOW_ALL,
-    filter = standardTreeWalkerFilter) {
+    filter = standardTreeWalkerFilter,
+    skipTo = SKIP_TO_PARENT) {
   if (!rootWin.location) {
     throw new Error("Got an invalid root window in DocumentWalker");
   }
@@ -2989,19 +3013,11 @@ function DocumentWalker(node, rootWin,
   this.filter = filter;
 
   // Make sure that the walker knows about the initial node (which could
-  // be skipped due to a filter).  Note that simply calling parentNode()
-  // causes currentNode to be updated.
-  this.walker.currentNode = node;
-  while (node &&
-         this.filter(node) === nodeFilterConstants.FILTER_SKIP) {
-    node = this.walker.parentNode();
-  }
+  // be skipped due to a filter).
+  this.walker.currentNode = this.getStartingNode(node, skipTo);
 }
 
 DocumentWalker.prototype = {
-  get node() {
-    return this.walker.node;
-  },
   get whatToShow() {
     return this.walker.whatToShow;
   },
@@ -3023,8 +3039,7 @@ DocumentWalker.prototype = {
     }
 
     let nextNode = this.walker.nextNode();
-    while (nextNode &&
-           this.filter(nextNode) === nodeFilterConstants.FILTER_SKIP) {
+    while (nextNode && this.isSkippedNode(nextNode)) {
       nextNode = this.walker.nextNode();
     }
 
@@ -3038,8 +3053,7 @@ DocumentWalker.prototype = {
     }
 
     let firstChild = this.walker.firstChild();
-    while (firstChild &&
-           this.filter(firstChild) === nodeFilterConstants.FILTER_SKIP) {
+    while (firstChild && this.isSkippedNode(firstChild)) {
       firstChild = this.walker.nextSibling();
     }
 
@@ -3053,8 +3067,7 @@ DocumentWalker.prototype = {
     }
 
     let lastChild = this.walker.lastChild();
-    while (lastChild &&
-           this.filter(lastChild) === nodeFilterConstants.FILTER_SKIP) {
+    while (lastChild && this.isSkippedNode(lastChild)) {
       lastChild = this.walker.previousSibling();
     }
 
@@ -3063,7 +3076,7 @@ DocumentWalker.prototype = {
 
   previousSibling: function () {
     let node = this.walker.previousSibling();
-    while (node && this.filter(node) === nodeFilterConstants.FILTER_SKIP) {
+    while (node && this.isSkippedNode(node)) {
       node = this.walker.previousSibling();
     }
     return node;
@@ -3071,11 +3084,62 @@ DocumentWalker.prototype = {
 
   nextSibling: function () {
     let node = this.walker.nextSibling();
-    while (node && this.filter(node) === nodeFilterConstants.FILTER_SKIP) {
+    while (node && this.isSkippedNode(node)) {
       node = this.walker.nextSibling();
     }
     return node;
-  }
+  },
+
+  getStartingNode: function (node, skipTo) {
+    // Keep a reference on the starting node in case we can't find a node compatible with
+    // the filter.
+    let startingNode = node;
+
+    if (skipTo === SKIP_TO_PARENT) {
+      while (node && this.isSkippedNode(node)) {
+        node = node.parentNode;
+      }
+    } else if (skipTo === SKIP_TO_SIBLING) {
+      node = this.getClosestAcceptedSibling(node);
+    }
+
+    return node || startingNode;
+  },
+
+  /**
+   * Loop on all of the provided node siblings until finding one that is compliant with
+   * the filter function.
+   */
+  getClosestAcceptedSibling: function (node) {
+    if (this.filter(node) === nodeFilterConstants.FILTER_ACCEPT) {
+      // node is already valid, return immediately.
+      return node;
+    }
+
+    // Loop on starting node siblings.
+    let previous = node;
+    let next = node;
+    while (previous || next) {
+      previous = previous && previous.previousSibling;
+      next = next && next.nextSibling;
+
+      if (this.filter(previous) === nodeFilterConstants.FILTER_ACCEPT) {
+        // A valid node was found in the previous siblings of the node.
+        return previous;
+      }
+
+      if (this.filter(next) === nodeFilterConstants.FILTER_ACCEPT) {
+        // A valid node was found in the next siblings of the node.
+        return next;
+      }
+    }
+
+    return null;
+  },
+
+  isSkippedNode: function (node) {
+    return this.filter(node) === nodeFilterConstants.FILTER_SKIP;
+  },
 };
 
 function isInXULDocument(el) {