Bug 1304685 - Show empty text nodes in markupview if they impact layout; r=jdescottes
authorPatrick Brosset <pbrosset@mozilla.com>
Tue, 11 Oct 2016 16:29:20 +0200
changeset 360355 fe1d7c28668f0eb07c2f91bd0b3b0fa635d82387
parent 360354 687572ea20cb84f1842ad00dcc8ca1712868a361
child 360356 500baee3ce9f1b701ace9d38489cbc112d076c72
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-beta@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes
bugs1304685
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1304685 - Show empty text nodes in markupview if they impact layout; r=jdescottes When the inspector actor walks the DOM in order to find nodes to be displayed in the inspector panel, it ignores text nodes that contain only whitespace characters (in order to avoid cluttering the panel with useless information). This commit changes this logic so that whitespace text nodes are only ignored when the node in fact has no layout at all (all text collapsed). Inside inline formatting contexts, whitespace text nodes may have layout and therefore push elements further apart. So seeing these nodes in the panel actually help debugging issues. MozReview-Commit-ID: GvNMsqsT3w6
devtools/client/inspector/markup/test/browser.ini
devtools/client/inspector/markup/test/browser_markup_navigation.js
devtools/client/inspector/markup/test/browser_markup_whitespace.js
devtools/client/inspector/markup/test/doc_markup_dragdrop.html
devtools/client/inspector/markup/test/doc_markup_whitespace.html
devtools/client/inspector/markup/views/markup-container.js
devtools/client/inspector/markup/views/root-container.js
devtools/client/inspector/markup/views/text-editor.js
devtools/client/locales/en-US/inspector.properties
devtools/client/shared/test/test-actor.js
devtools/client/themes/markup.css
devtools/server/actors/inspector.js
--- a/devtools/client/inspector/markup/test/browser.ini
+++ b/devtools/client/inspector/markup/test/browser.ini
@@ -25,16 +25,17 @@ support-files =
   doc_markup_pagesize_01.html
   doc_markup_pagesize_02.html
   doc_markup_search.html
   doc_markup_svg_attributes.html
   doc_markup_toggle.html
   doc_markup_tooltip.png
   doc_markup_void_elements.html
   doc_markup_void_elements.xhtml
+  doc_markup_whitespace.html
   doc_markup_xul.xul
   head.js
   helper_attributes_test_runner.js
   helper_events_test_runner.js
   helper_markup_accessibility_navigation.js
   helper_outerhtml_test_runner.js
   helper_style_attr_test_runner.js
   lib_jquery_1.0.js
@@ -146,8 +147,9 @@ skip-if = e10s # Bug 1036409 - The last 
 [browser_markup_textcontent_edit_01.js]
 [browser_markup_textcontent_edit_02.js]
 [browser_markup_toggle_01.js]
 [browser_markup_toggle_02.js]
 [browser_markup_toggle_03.js]
 [browser_markup_update-on-navigtion.js]
 [browser_markup_void_elements_html.js]
 [browser_markup_void_elements_xhtml.js]
+[browser_markup_whitespace.js]
--- a/devtools/client/inspector/markup/test/browser_markup_navigation.js
+++ b/devtools/client/inspector/markup/test/browser_markup_navigation.js
@@ -17,30 +17,36 @@ const TEST_DATA = [
   ["down", "node1"],
   ["down", "node2"],
   ["down", "node3"],
   ["down", "*comment*"],
   ["down", "node4"],
   ["right", "node4"],
   ["down", "*text*"],
   ["down", "node5"],
+  ["down", "*text*"],
   ["down", "node6"],
+  ["down", "*text*"],
   ["down", "*comment*"],
   ["down", "node7"],
   ["right", "node7"],
   ["down", "*text*"],
   ["down", "node8"],
   ["left", "node7"],
   ["left", "node7"],
   ["right", "node7"],
   ["right", "*text*"],
   ["down", "node8"],
+  ["down", "*text*"],
   ["down", "node9"],
+  ["down", "*text*"],
   ["down", "node10"],
+  ["down", "*text*"],
   ["down", "node11"],
+  ["down", "*text*"],
   ["down", "node12"],
   ["right", "node12"],
   ["down", "*text*"],
   ["down", "node13"],
   ["down", "node14"],
   ["down", "node15"],
   ["down", "node15"],
   ["down", "node15"],
@@ -48,23 +54,27 @@ const TEST_DATA = [
   ["up", "node13"],
   ["up", "*text*"],
   ["up", "node12"],
   ["left", "node12"],
   ["down", "node14"],
   ["home", "*doctype*"],
   ["pagedown", "*text*"],
   ["down", "node5"],
+  ["down", "*text*"],
   ["down", "node6"],
+  ["down", "*text*"],
   ["down", "*comment*"],
   ["down", "node7"],
   ["left", "node7"],
+  ["down", "*text*"],
   ["down", "node9"],
+  ["down", "*text*"],
   ["down", "node10"],
-  ["pageup", "node2"],
+  ["pageup", "*text*"],
   ["pageup", "*doctype*"],
   ["down", "html"],
   ["left", "html"],
   ["down", "head"]
 ];
 
 add_task(function* () {
   let {inspector} = yield openInspectorForURL(TEST_URL);
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/markup/test/browser_markup_whitespace.js
@@ -0,0 +1,66 @@
+/* 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";
+
+// Test that whitespace text nodes do show up in the markup-view when needed.
+
+const TEST_URL = URL_ROOT + "doc_markup_whitespace.html";
+
+add_task(function* () {
+  let {inspector, testActor} = yield openInspectorForURL(TEST_URL);
+  let {markup} = inspector;
+
+  yield markup.expandAll();
+
+  info("Verify the number of child nodes and child elements in body");
+
+  // Body has 5 element children, but there are 6 text nodes in there too, they come from
+  // the HTML file formatting (spaces and carriage returns).
+  let {numNodes, numChildren} = yield testActor.getNodeInfo("body");
+  is(numNodes, 11, "The body node has 11 child nodes (includes text nodes)");
+  is(numChildren, 5, "The body node has 5 child elements (only element nodes)");
+
+  // In body, there are only block-level elements, so whitespace text nodes do not have
+  // layout, so they should be skipped in the markup-view.
+  info("Check that the body's whitespace text node children aren't shown");
+  let bodyContainer = markup.getContainer(inspector.selection.nodeFront);
+  let childContainers = bodyContainer.getChildContainers();
+  is(childContainers.length, 5,
+     "Only the element nodes are shown in the markup view");
+
+  // div#inline has 3 element children, but there are 4 text nodes in there too, like in
+  // body, they come from spaces and carriage returns in the HTML file.
+  info("Verify the number of child nodes and child elements in div#inline");
+  ({numNodes, numChildren} = yield testActor.getNodeInfo("#inline"));
+  is(numNodes, 7, "The div#inline node has 7 child nodes (includes text nodes)");
+  is(numChildren, 3, "The div#inline node has 3 child elements (only element nodes)");
+
+  // Within the inline formatting context in div#inline, the whitespace text nodes between
+  // the images have layout, so they should appear in the markup-view.
+  info("Check that the div#inline's whitespace text node children are shown");
+  yield selectNode("#inline", inspector);
+  let divContainer = markup.getContainer(inspector.selection.nodeFront);
+  childContainers = divContainer.getChildContainers();
+  is(childContainers.length, 5,
+     "Both the element nodes and some text nodes are shown in the markup view");
+
+  // div#pre has 2 element children, but there are 3 text nodes in there too, like in
+  // div#inline, they come from spaces and carriage returns in the HTML file.
+  info("Verify the number of child nodes and child elements in div#pre");
+  ({numNodes, numChildren} = yield testActor.getNodeInfo("#pre"));
+  is(numNodes, 5, "The div#pre node has 5 child nodes (includes text nodes)");
+  is(numChildren, 2, "The div#pre node has 2 child elements (only element nodes)");
+
+  // Within the inline formatting context in div#pre, the whitespace text nodes between
+  // the images have layout, so they should appear in the markup-view, but since
+  // white-space is set to pre, then the whitespace text nodes before and after the first
+  // and last image should also appear.
+  info("Check that the div#pre's whitespace text node children are shown");
+  yield selectNode("#pre", inspector);
+  divContainer = markup.getContainer(inspector.selection.nodeFront);
+  childContainers = divContainer.getChildContainers();
+  is(childContainers.length, 5,
+     "Both the element nodes and all text nodes are shown in the markup view");
+});
--- a/devtools/client/inspector/markup/test/doc_markup_dragdrop.html
+++ b/devtools/client/inspector/markup/test/doc_markup_dragdrop.html
@@ -11,21 +11,13 @@ https://bugzilla.mozilla.org/show_bug.cg
       content: 'This should not be draggable';
     }
   </style>
 </head>
 <body>
   <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=858038">Mozilla Bug 858038</a>
   <p id="display"></p>
   <div id="content" style="display: none">
-
   </div>
-  <input id="anonymousParent" />
-
-  <span id="before">Before<!-- Force not-inline --></span>
-  <pre id="test">
-    <span id="firstChild">First</span>
-    <span id="middleChild">Middle</span>
-    <span id="lastChild">Last</span>
-  </pre>
-  <span id="after">After</span>
+  <input id="anonymousParent" /><span id="before">Before<!-- Force not-inline --></span>
+  <pre id="test"><span id="firstChild">First</span><span id="middleChild">Middle</span><span id="lastChild">Last</span></pre>  <span id="after">After</span>
 </body>
 </html>
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/markup/test/doc_markup_whitespace.html
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <style>
+      #pre {
+        white-space: pre;
+      }
+    </style>
+  </head>
+  <body>
+    <div>div 1</div>
+    <div>div 2</div>
+    <div>div 3</div>
+    <div id="inline">
+      <img src="chrome://branding/content/about-logo.png" />
+      <img src="chrome://branding/content/about-logo.png" />
+      <img src="chrome://branding/content/about-logo.png" />
+    </div>
+    <div id="pre">
+      <img src="chrome://branding/content/about-logo.png" />
+      <img src="chrome://branding/content/about-logo.png" />
+    </div>
+  </body>
+</html>
--- a/devtools/client/inspector/markup/views/markup-container.js
+++ b/devtools/client/inspector/markup/views/markup-container.js
@@ -254,17 +254,18 @@ MarkupContainer.prototype = {
    * If the node has children, return the list of containers for all these
    * children.
    */
   getChildContainers: function () {
     if (!this.hasChildren) {
       return null;
     }
 
-    return [...this.children.children].map(node => node.container);
+    return [...this.children.children].filter(node => node.container)
+                                      .map(node => node.container);
   },
 
   /**
    * True if the node has been visually expanded in the tree.
    */
   get expanded() {
     return !this.elt.classList.contains("collapsed");
   },
--- a/devtools/client/inspector/markup/views/root-container.js
+++ b/devtools/client/inspector/markup/views/root-container.js
@@ -22,21 +22,22 @@ function RootContainer(markupView, node)
 
 RootContainer.prototype = {
   hasChildren: true,
   expanded: true,
   update: function () {},
   destroy: function () {},
 
   /**
-   * If the node has children, return the list of containers for all these
-   * children.
+   * If the node has children, return the list of containers for all these children.
+   * @return {Array} An array of child containers or null.
    */
   getChildContainers: function () {
-    return [...this.children.children].map(node => node.container);
+    return [...this.children.children].filter(node => node.container)
+                                      .map(node => node.container);
   },
 
   /**
    * Set the expanded state of the container node.
    * @param  {Boolean} value
    */
   setExpanded: function () {},
 
--- a/devtools/client/inspector/markup/views/text-editor.js
+++ b/devtools/client/inspector/markup/views/text-editor.js
@@ -2,16 +2,19 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 const {getAutocompleteMaxWidth} = require("devtools/client/inspector/markup/utils");
 const {editableField} = require("devtools/client/shared/inplace-editor");
 const {getCssProperties} = require("devtools/shared/fronts/css-properties");
+const {LocalizationHelper} = require("devtools/shared/l10n");
+
+const INSPECTOR_L10N = new LocalizationHelper("devtools/locale/inspector.properties");
 
 /**
  * Creates a simple text editor node, used for TEXT and COMMENT
  * nodes.
  *
  * @param  {MarkupContainer} container
  *         The container owning this editor.
  * @param  {DOMNode} node
@@ -74,16 +77,26 @@ TextEditor.prototype = {
   update: function () {
     let longstr = null;
     this.node.getNodeValue().then(ret => {
       longstr = ret;
       return longstr.string();
     }).then(str => {
       longstr.release().then(null, console.error);
       this.value.textContent = str;
+
+      let isWhitespace = !/[^\s]/.exec(str);
+      this.value.classList.toggle("whitespace", isWhitespace);
+
+      let chars = str.replace(/\n/g, "⏎")
+                     .replace(/\t/g, "⇥")
+                     .replace(/ /g, "◦");
+      this.value.setAttribute("title", isWhitespace
+        ? INSPECTOR_L10N.getFormatStr("markupView.whitespaceOnly", chars)
+        : "");
     }).then(null, console.error);
   },
 
   destroy: function () {},
 
   /**
    * Stub method for consistency with ElementEditor.
    */
--- a/devtools/client/locales/en-US/inspector.properties
+++ b/devtools/client/locales/en-US/inspector.properties
@@ -28,16 +28,20 @@ inspector.panelLabel.markupView=Markup V
 # When there are too many nodes to load at once, we will offer to
 # show all the nodes.
 markupView.more.showing=Some nodes were hidden.
 
 # LOCALIZATION NOTE (markupView.more.showAll2): Semi-colon list of plural forms.
 # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
 markupView.more.showAll2=Show one more node;Show all #1 nodes
 
+# LOCALIZATION NOTE (markupView.whitespaceOnly)
+# Used in a tooltip that appears when the user hovers over whitespace-only text nodes in
+# the inspector.
+markupView.whitespaceOnly=Whitespace-only text node: %S
 
 #LOCALIZATION NOTE: Used in the image preview tooltip when the image could not be loaded
 previewTooltip.image.brokenImage=Could not load the image
 
 #LOCALIZATION NOTE: Used in the image preview tooltip when the image could not be loaded
 eventsTooltip.openInDebugger=Open in Debugger
 
 # LOCALIZATION NOTE (docsTooltip.visitMDN): Shown in the tooltip that displays
--- a/devtools/client/shared/test/test-actor.js
+++ b/devtools/client/shared/test/test-actor.js
@@ -749,16 +749,17 @@ var TestActor = exports.TestActor = prot
     let node = this._querySelector(selector);
     let info = null;
 
     if (node) {
       info = {
         tagName: node.tagName,
         namespaceURI: node.namespaceURI,
         numChildren: node.children.length,
+        numNodes: node.childNodes.length,
         attributes: [...node.attributes].map(({name, value, namespaceURI}) => {
           return {name, value, namespaceURI};
         }),
         outerHTML: node.outerHTML,
         innerHTML: node.innerHTML,
         textContent: node.textContent
       };
     }
--- a/devtools/client/themes/markup.css
+++ b/devtools/client/themes/markup.css
@@ -242,16 +242,35 @@ ul.children + .tag-line::before {
   display: inline-block;
 }
 
 .editor.text pre,
 .editor.comment pre {
   font: inherit;
 }
 
+/* Whitespace only text nodes are sometimes shown in the markup-view, and when they do
+   they get a greyed-out whitespace symbol so users know what they are */
+.editor.text .whitespace {
+  padding: 0 .5em;
+}
+
+.editor.text .whitespace::before {
+  content: "";
+  display: inline-block;
+  height: 4px;
+  width: 4px;
+  border: 1px solid var(--theme-body-color-inactive);
+  border-radius: 50%;
+}
+
+.tag-line[selected] .editor.text .whitespace::before {
+  border-color: white;
+}
+
 .more-nodes {
   padding-left: 16px;
 }
 
 .styleinspector-propertyeditor {
   border: 1px solid #CCC;
 }
 
--- a/devtools/server/actors/inspector.js
+++ b/devtools/server/actors/inspector.js
@@ -2953,20 +2953,21 @@ function isInXULDocument(el) {
 function standardTreeWalkerFilter(node) {
   // ::before and ::after are native anonymous content, but we always
   // want to show them
   if (node.nodeName === "_moz_generated_content_before" ||
       node.nodeName === "_moz_generated_content_after") {
     return nodeFilterConstants.FILTER_ACCEPT;
   }
 
-  // Ignore empty whitespace text nodes.
-  if (node.nodeType == Ci.nsIDOMNode.TEXT_NODE &&
-      !/[^\s]/.exec(node.nodeValue)) {
-    return nodeFilterConstants.FILTER_SKIP;
+  // Ignore empty whitespace text nodes that do not impact the layout.
+  if (isWhitespaceTextNode(node)) {
+    return nodeHasSize(node)
+           ? nodeFilterConstants.FILTER_ACCEPT
+           : nodeFilterConstants.FILTER_SKIP;
   }
 
   // Ignore all native and XBL anonymous content inside a non-XUL document
   if (!isInXULDocument(node) && (isXBLAnonymous(node) ||
                                   isNativeAnonymous(node))) {
     // Note: this will skip inspecting the contents of feedSubscribeLine since
     // that's XUL content injected in an HTML document, but we need to because
     // this also skips many other elements that need to be skipped - like form
@@ -2977,25 +2978,49 @@ function standardTreeWalkerFilter(node) 
   return nodeFilterConstants.FILTER_ACCEPT;
 }
 
 /**
  * This DeepTreeWalker filter is like standardTreeWalkerFilter except that
  * it also includes all anonymous content (like internal form controls).
  */
 function allAnonymousContentTreeWalkerFilter(node) {
-  // Ignore empty whitespace text nodes.
-  if (node.nodeType == Ci.nsIDOMNode.TEXT_NODE &&
-      !/[^\s]/.exec(node.nodeValue)) {
-    return nodeFilterConstants.FILTER_SKIP;
+  // Ignore empty whitespace text nodes that do not impact the layout.
+  if (isWhitespaceTextNode(node)) {
+    return nodeHasSize(node)
+           ? nodeFilterConstants.FILTER_ACCEPT
+           : nodeFilterConstants.FILTER_SKIP;
   }
   return nodeFilterConstants.FILTER_ACCEPT;
 }
 
 /**
+ * Is the given node a text node composed of whitespace only?
+ * @param {DOMNode} node
+ * @return {Boolean}
+ */
+function isWhitespaceTextNode(node) {
+  return node.nodeType == Ci.nsIDOMNode.TEXT_NODE && !/[^\s]/.exec(node.nodeValue);
+}
+
+/**
+ * Does the given node have non-0 width and height?
+ * @param {DOMNode} node
+ * @return {Boolean}
+ */
+function nodeHasSize(node) {
+  if (!node.getBoxQuads) {
+    return false;
+  }
+
+  let quads = node.getBoxQuads();
+  return quads.length && quads.some(quad => quad.bounds.width && quad.bounds.height);
+}
+
+/**
  * Returns a promise that is settled once the given HTMLImageElement has
  * finished loading.
  *
  * @param {HTMLImageElement} image - The image element.
  * @param {Number} timeout - Maximum amount of time the image is allowed to load
  * before the waiting is aborted. Ignored if flags.testing is set.
  *
  * @return {Promise} that is fulfilled once the image has loaded. If the image