Bug 1345529 - fix inspector DocumentWaler children() method;r=pbro
authorJulian Descottes <jdescottes@mozilla.com>
Thu, 06 Apr 2017 23:17:03 +0200
changeset 560245 d508a91f09e9de268da1b7831c823c6a42f16526
parent 560244 e2bcdd1501405f8bc8e5841a06088fd3fc179878
child 560246 92fb427b464c49134341747d146ddef6f43c87b4
push id53365
push userjichen@mozilla.com
push dateTue, 11 Apr 2017 08:35:12 +0000
reviewerspbro
bugs1345529
milestone55.0a1
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
devtools/client/inspector/markup/test/browser.ini
devtools/client/inspector/markup/test/browser_markup_tag_delete_whitespace_node.js
devtools/server/actors/inspector.js
--- a/devtools/client/inspector/markup/test/browser.ini
+++ b/devtools/client/inspector/markup/test/browser.ini
@@ -143,16 +143,17 @@ skip-if = (os == 'linux' && bits == 32 &
 [browser_markup_node_names_namespaced.js]
 [browser_markup_node_not_displayed_01.js]
 [browser_markup_node_not_displayed_02.js]
 [browser_markup_pagesize_01.js]
 [browser_markup_pagesize_02.js]
 [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]
 [browser_markup_tag_edit_04-backspace.js]
 [browser_markup_tag_edit_04-delete.js]
 [browser_markup_tag_edit_05.js]
 [browser_markup_tag_edit_06.js]
 [browser_markup_tag_edit_07.js]
new file mode 100644
--- /dev/null
+++ b/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");
+});
--- a/devtools/server/actors/inspector.js
+++ b/devtools/server/actors/inspector.js
@@ -85,16 +85,21 @@ const FONT_FAMILY_PREVIEW_TEXT = "The qu
 const FONT_FAMILY_PREVIEW_TEXT_SIZE = 20;
 const PSEUDO_CLASSES = [":hover", ":active", ":focus"];
 const HIDDEN_CLASS = "__fx-devtools-hide-shortcut__";
 const SVG_NS = "http://www.w3.org/2000/svg";
 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 = [
   [":active", 1],
   [":hover", 1],
   [":focus", 1],
   [":visited", 0],
   [":link", 0],
@@ -929,22 +934,22 @@ var WalkerActor = protocol.ActorClassWit
       }
     };
   },
 
   toString: function () {
     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 () {
     if (this._destroyed) {
       return;
     }
     this._destroyed = true;
     protocol.Actor.prototype.destroy.call(this);
@@ -1365,17 +1370,20 @@ var WalkerActor = protocol.ActorClassWit
     let maxNodes = options.maxNodes || -1;
     if (maxNodes == -1) {
       maxNodes = Number.MAX_VALUE;
     }
 
     // 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.
     let rawNode = node.rawNode;
     let firstChild = getFilteredWalker(rawNode).firstChild();
     let lastChild = getFilteredWalker(rawNode).lastChild();
 
     if (!firstChild) {
@@ -1391,17 +1399,17 @@ var WalkerActor = protocol.ActorClassWit
     } else {
       start = firstChild;
     }
 
     let nodes = [];
 
     // 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);
       nodes = backwardNodes;
     }
 
     // Then read forward by any slack left in the max children...
     let forwardWalker = getFilteredWalker(start);
@@ -1513,33 +1521,43 @@ var WalkerActor = protocol.ActorClassWit
   },
 
   /**
    * Helper function for the `children` method: Read forward in the sibling
    * list into an array with `count` items, including the current node.
    */
   _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;
   },
 
   /**
    * Helper function for the `children` method: Read backward in the sibling
    * list into an array with `count` items, including the current node.
    */
   _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();
     return ret;
   },
 
   /**
    * Return the first node in the document that matches the given selector.
@@ -2963,50 +2981,48 @@ function isNodeDead(node) {
 }
 
 /**
  * Wrapper for inDeepTreeWalker.  Adds filtering to the traversal methods.
  * See inDeepTreeWalker for more information about the methods.
  *
  * @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");
   }
 
   this.walker = Cc["@mozilla.org/inspector/deep-tree-walker;1"]
     .createInstance(Ci.inIDeepTreeWalker);
   this.walker.showAnonymousContent = true;
   this.walker.showSubDocuments = true;
   this.walker.showDocumentsAsNodes = true;
   this.walker.init(rootWin.document, whatToShow);
   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;
   },
   get currentNode() {
     return this.walker.currentNode;
   },
   set currentNode(val) {
     this.walker.currentNode = val;
@@ -3018,69 +3034,117 @@ DocumentWalker.prototype = {
 
   nextNode: function () {
     let node = this.walker.currentNode;
     if (!node) {
       return null;
     }
 
     let nextNode = this.walker.nextNode();
-    while (nextNode &&
-           this.filter(nextNode) === nodeFilterConstants.FILTER_SKIP) {
+    while (nextNode && this.isSkippedNode(nextNode)) {
       nextNode = this.walker.nextNode();
     }
 
     return nextNode;
   },
 
   firstChild: function () {
     let node = this.walker.currentNode;
     if (!node) {
       return null;
     }
 
     let firstChild = this.walker.firstChild();
-    while (firstChild &&
-           this.filter(firstChild) === nodeFilterConstants.FILTER_SKIP) {
+    while (firstChild && this.isSkippedNode(firstChild)) {
       firstChild = this.walker.nextSibling();
     }
 
     return firstChild;
   },
 
   lastChild: function () {
     let node = this.walker.currentNode;
     if (!node) {
       return null;
     }
 
     let lastChild = this.walker.lastChild();
-    while (lastChild &&
-           this.filter(lastChild) === nodeFilterConstants.FILTER_SKIP) {
+    while (lastChild && this.isSkippedNode(lastChild)) {
       lastChild = this.walker.previousSibling();
     }
 
     return lastChild;
   },
 
   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;
   },
 
   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) {
   let doc = nodeDocument(el);
   return doc &&
          doc.documentElement &&
          doc.documentElement.namespaceURI === XUL_NS;
 }