Bug 911209 - Display hidden nodes differently in the markup-view; r=miker
☠☠ backed out by 51b428be6213 ☠ ☠
authorPatrick Brosset <pbrosset@mozilla.com>
Wed, 04 Jun 2014 21:42:16 +0200
changeset 186585 52068b669c2af0023292655c64a568a63e94608f
parent 186584 e160cbe83f39d9cd6bacadd1f0a44ce54f1de0d0
child 186586 8fac243930da445352fd91c5d254b7253ba06d54
push idunknown
push userunknown
push dateunknown
reviewersmiker
bugs911209
milestone32.0a1
Bug 911209 - Display hidden nodes differently in the markup-view; r=miker
browser/devtools/markupview/markup-view.js
browser/devtools/markupview/test/browser.ini
browser/devtools/markupview/test/browser_markupview_node_not_displayed_01.js
browser/devtools/markupview/test/browser_markupview_node_not_displayed_02.js
browser/devtools/markupview/test/doc_markup_not_displayed.html
browser/themes/shared/devtools/markup-view.css
toolkit/devtools/LayoutHelpers.jsm
toolkit/devtools/server/actors/inspector.js
--- a/browser/devtools/markupview/markup-view.js
+++ b/browser/devtools/markupview/markup-view.js
@@ -80,16 +80,19 @@ function MarkupView(aInspector, aFrame, 
   this.undo = new UndoStack();
   this.undo.installController(aControllerWindow);
 
   this._containers = new Map();
 
   this._boundMutationObserver = this._mutationObserver.bind(this);
   this.walker.on("mutations", this._boundMutationObserver);
 
+  this._boundOnDisplayChange = this._onDisplayChange.bind(this);
+  this.walker.on("display-change", this._boundOnDisplayChange);
+
   this._boundOnNewSelection = this._onNewSelection.bind(this);
   this._inspector.selection.on("new-node-front", this._boundOnNewSelection);
   this._onNewSelection();
 
   this._boundKeyDown = this._onKeyDown.bind(this);
   this._frame.contentWindow.addEventListener("keydown", this._boundKeyDown, false);
 
   this._boundFocus = this._onFocus.bind(this);
@@ -610,16 +613,29 @@ MarkupView.prototype = {
           }
         });
 
       }
     });
   },
 
   /**
+   * React to display-change events from the walker
+   * @param {Array} nodes An array of nodeFronts
+   */
+  _onDisplayChange: function(nodes) {
+    for (let node of nodes) {
+      let container = this._containers.get(node);
+      if (container) {
+        container.isDisplayed = node.isDisplayed;
+      }
+    }
+  },
+
+  /**
    * Given a list of mutations returned by the mutation observer, flash the
    * corresponding containers to attract attention.
    */
   _flashMutatedNodes: function(aMutations) {
     let addedOrEditedContainers = new Set();
     let removedContainers = new Set();
 
     for (let {type, target, added, removed} of aMutations) {
@@ -1105,16 +1121,19 @@ MarkupView.prototype = {
     this._boundKeyDown = null;
 
     this._inspector.selection.off("new-node-front", this._boundOnNewSelection);
     this._boundOnNewSelection = null;
 
     this.walker.off("mutations", this._boundMutationObserver)
     this._boundMutationObserver = null;
 
+    this.walker.off("display-change", this._boundOnDisplayChange);
+    this._boundOnDisplayChange = null;
+
     this._elt.removeEventListener("mousemove", this._onMouseMove, false);
     this._elt.removeEventListener("mouseleave", this._onMouseLeave, false);
     this._elt = null;
 
     for (let [key, container] of this._containers) {
       container.destroy();
     }
     this._containers = null;
@@ -1263,16 +1282,19 @@ function MarkupContainer(aMarkupView, aN
   // Appending the editor element and attaching event listeners
   this.tagLine.appendChild(this.editor.elt);
 
   this._onMouseDown = this._onMouseDown.bind(this);
   this.elt.addEventListener("mousedown", this._onMouseDown, false);
 
   // Prepare the image preview tooltip data if any
   this._prepareImagePreview();
+
+  // Marking the node as shown or hidden
+  this.isDisplayed = this.node.isDisplayed;
 }
 
 MarkupContainer.prototype = {
   toString: function() {
     return "[MarkupContainer for " + this.node + "]";
   },
 
   isPreviewable: function() {
@@ -1337,16 +1359,26 @@ MarkupContainer.prototype = {
 
     return this.tooltipData.data.then(({data, size}) => {
       tooltip.setImageContent(data, size);
     }, () => {
       tooltip.setBrokenImageContent();
     });
   },
 
+  /**
+   * Show the element has displayed or not
+   */
+  set isDisplayed(isDisplayed) {
+    this.elt.classList.remove("not-displayed");
+    if (!isDisplayed) {
+      this.elt.classList.add("not-displayed");
+    }
+  },
+
   copyImageDataUri: function() {
     // We need to send again a request to gettooltipData even if one was sent for
     // the tooltip, because we want the full-size image
     this.node.getImageData().then(data => {
       data.data.string().then(str => {
         clipboardHelper.copyString(str, this.markup.doc);
       });
     });
--- a/browser/devtools/markupview/test/browser.ini
+++ b/browser/devtools/markupview/test/browser.ini
@@ -1,16 +1,17 @@
 [DEFAULT]
 skip-if = e10s # Bug ?????? - devtools tests disabled with e10s
 subsuite = devtools
 support-files =
   doc_markup_edit.html
   doc_markup_flashing.html
   doc_markup_mutation.html
   doc_markup_navigation.html
+  doc_markup_not_displayed.html
   doc_markup_pagesize_01.html
   doc_markup_pagesize_02.html
   doc_markup_search.html
   doc_markup_toggle.html
   doc_markup_tooltip.png
   head.js
   helper_attributes_test_runner.js
   helper_outerhtml_test_runner.js
@@ -21,16 +22,18 @@ support-files =
 [browser_markupview_highlight_hover_02.js]
 [browser_markupview_html_edit_01.js]
 [browser_markupview_html_edit_02.js]
 [browser_markupview_html_edit_03.js]
 [browser_markupview_image_tooltip.js]
 [browser_markupview_mutation_01.js]
 [browser_markupview_mutation_02.js]
 [browser_markupview_navigation.js]
+[browser_markupview_node_not_displayed_01.js]
+[browser_markupview_node_not_displayed_02.js]
 [browser_markupview_pagesize_01.js]
 [browser_markupview_pagesize_02.js]
 [browser_markupview_search_01.js]
 [browser_markupview_tag_edit_01.js]
 [browser_markupview_tag_edit_02.js]
 [browser_markupview_tag_edit_03.js]
 [browser_markupview_tag_edit_04.js]
 [browser_markupview_tag_edit_05.js]
new file mode 100644
--- /dev/null
+++ b/browser/devtools/markupview/test/browser_markupview_node_not_displayed_01.js
@@ -0,0 +1,33 @@
+/* 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";
+
+// Tests that nodes that are not displayed appear differently in the markup-view
+// when these nodes are imported in the view.
+
+// Note that nodes inside a display:none parent are obviously not displayed too
+// but the markup-view uses css inheritance to mark those as hidden instead of
+// having to visit each and every child of a hidden node. So there's no sense
+// testing children nodes.
+
+const TEST_URL = TEST_URL_ROOT + "doc_markup_not_displayed.html";
+const TEST_DATA = [
+  {selector: "#normal-div", isDisplayed: true},
+  {selector: "head", isDisplayed: false},
+  {selector: "#display-none", isDisplayed: false},
+  {selector: "#hidden-true", isDisplayed: false},
+  {selector: "#visibility-hidden", isDisplayed: true}
+];
+
+let test = asyncTest(function*() {
+  let {inspector} = yield addTab(TEST_URL).then(openInspector);
+
+  for (let {selector, isDisplayed} of TEST_DATA) {
+    info("Getting node " + selector);
+    let container = getContainerForRawNode(selector, inspector);
+    is(!container.elt.classList.contains("not-displayed"), isDisplayed,
+      "The container for " + selector + " is marked as displayed " + isDisplayed);
+  }
+});
new file mode 100644
--- /dev/null
+++ b/browser/devtools/markupview/test/browser_markupview_node_not_displayed_02.js
@@ -0,0 +1,132 @@
+/* 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";
+
+// Tests that nodes are marked as displayed and not-displayed dynamically, when
+// their display changes
+
+const TEST_URL = TEST_URL_ROOT + "doc_markup_not_displayed.html";
+const TEST_DATA = [
+  {
+    desc: "Hiding a node by creating a new stylesheet",
+    selector: "#normal-div",
+    before: true,
+    changeStyle: (doc, node) => {
+      let div = doc.createElement("div");
+      div.id = "new-style";
+      div.innerHTML = "<style>#normal-div {display:none;}</style>";
+      doc.body.appendChild(div);
+    },
+    after: false
+  },
+  {
+    desc: "Showing a node by deleting an existing stylesheet",
+    selector: "#normal-div",
+    before: false,
+    changeStyle: (doc, node) => {
+      doc.getElementById("new-style").remove();
+    },
+    after: true
+  },
+  {
+    desc: "Hiding a node by changing its style property",
+    selector: "#display-none",
+    before: false,
+    changeStyle: (doc, node) => {
+      node.style.display = "block";
+    },
+    after: true
+  },
+  {
+    desc: "Showing a node by removing its hidden attribute",
+    selector: "#hidden-true",
+    before: false,
+    changeStyle: (doc, node) => {
+      node.removeAttribute("hidden");
+    },
+    after: true
+  },
+  {
+    desc: "Hiding a node by adding a hidden attribute",
+    selector: "#hidden-true",
+    before: true,
+    changeStyle: (doc, node) => {
+      node.setAttribute("hidden", "true");
+    },
+    after: false
+  },
+  {
+    desc: "Showing a node by changin a stylesheet's rule",
+    selector: "#hidden-via-stylesheet",
+    before: false,
+    changeStyle: (doc, node) => {
+      doc.styleSheets[0].cssRules[0].style.setProperty("display", "inline");
+    },
+    after: true
+  },
+  {
+    desc: "Hiding a node by adding a new rule to a stylesheet",
+    selector: "#hidden-via-stylesheet",
+    before: true,
+    changeStyle: (doc, node) => {
+      doc.styleSheets[0].insertRule(
+        "#hidden-via-stylesheet {display: none;}", 1);
+    },
+    after: false
+  },
+  {
+    desc: "Hiding a node by adding a class that matches an existing rule",
+    selector: "#normal-div",
+    before: true,
+    changeStyle: (doc, node) => {
+      doc.styleSheets[0].insertRule(
+        ".a-new-class {display: none;}", 2);
+      node.classList.add("a-new-class");
+    },
+    after: false
+  }
+];
+
+let test = asyncTest(function*() {
+  let {inspector} = yield addTab(TEST_URL).then(openInspector);
+
+  for (let data of TEST_DATA) {
+    info("Running test case: " + data.desc);
+    yield runTestData(inspector, data);
+  }
+});
+
+function runTestData(inspector, {selector, before, changeStyle, after}) {
+  let def = promise.defer();
+
+  info("Getting the " + selector + " test node");
+  let container = getContainerForRawNode(selector, inspector);
+  is(!container.elt.classList.contains("not-displayed"), before,
+    "The container is marked as " + (before ? "shown" : "hidden"));
+
+  info("Listening for the display-change event");
+  inspector.markup.walker.once("display-change", nodes => {
+    info("Verifying that the list of changed nodes include our container");
+
+    ok(nodes.length, "The display-change event was received with a nodes");
+    let foundContainer = false;
+    for (let node of nodes) {
+      if (inspector.markup.getContainer(node) === container) {
+        foundContainer = true;
+        break;
+      }
+    }
+    ok(foundContainer, "Container is part of the list of changed nodes");
+
+    is(!container.elt.classList.contains("not-displayed"), after,
+      "The container is marked as " + (after ? "shown" : "hidden"));
+    def.resolve();
+  });
+
+  info("Making style changes");
+  changeStyle(content.document, getNode(selector));
+
+  return def.promise;
+}
new file mode 100644
--- /dev/null
+++ b/browser/devtools/markupview/test/doc_markup_not_displayed.html
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <style>
+    #hidden-via-stylesheet {
+      display: none;
+    }
+  </style>
+</head>
+<body>
+  <div id="normal-div"></div>
+  <div id="display-none" style="display:none;"></div>
+  <div id="hidden-true" hidden="true"></div>
+  <div id="visibility-hidden" style="visibility:hidden;"></div>
+  <div id="hidden-via-stylesheet"></div>
+</body>
+</html>
\ No newline at end of file
--- a/browser/themes/shared/devtools/markup-view.css
+++ b/browser/themes/shared/devtools/markup-view.css
@@ -25,16 +25,22 @@
 .theme-selected ~ .editor .theme-fg-color3,
 .theme-selected ~ .editor .theme-fg-color4,
 .theme-selected ~ .editor .theme-fg-color5,
 .theme-selected ~ .editor .theme-fg-color6,
 .theme-selected ~ .editor .theme-fg-color7 {
   color: #f5f7fa; /* Light foreground text */
 }
 
+/* In case a node isn't displayed in the page, we fade the syntax highlighting */
+.not-displayed .open,
+.not-displayed .close {
+  opacity: .7;
+}
+
 .tag-line {
   padding-left: 2px;
 }
 
 /* Preview */
 
 #previewbar {
   position: fixed;
--- a/toolkit/devtools/LayoutHelpers.jsm
+++ b/toolkit/devtools/LayoutHelpers.jsm
@@ -468,16 +468,20 @@ LayoutHelpers.prototype = {
 
     // If the boxType is border, no need to go any further, we're done
     if (region === "border") {
       return this._getBoxQuadsFromRect(borderRect, node);
     }
 
     // Else, need to get margin/padding/border distances
     let style = node.ownerDocument.defaultView.getComputedStyle(node);
+    if (!style) {
+      return null;
+    }
+
     let camel = s => s.substring(0, 1).toUpperCase() + s.substring(1);
     let distances = {border:{}, padding:{}, margin: {}};
 
     for (let side of ["top", "right", "bottom", "left"]) {
       distances.border[side] = this._parseNb(style["border" + camel(side) + "Width"]);
       distances.padding[side] = this._parseNb(style["padding" + camel(side)]);
       distances.margin[side] = this._parseNb(style["margin" + camel(side)]);
     }
--- a/toolkit/devtools/server/actors/inspector.js
+++ b/toolkit/devtools/server/actors/inspector.js
@@ -57,16 +57,18 @@ const {Arg, Option, method, RetVal, type
 const {LongStringActor, ShortLongString} = require("devtools/server/actors/string");
 const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
 const object = require("sdk/util/object");
 const events = require("sdk/event/core");
 const {Unknown} = require("sdk/platform/xpcom");
 const {Class} = require("sdk/core/heritage");
 const {PageStyleActor} = require("devtools/server/actors/styles");
 const {HighlighterActor} = require("devtools/server/actors/highlighter");
+const {getLayoutChangesObserver, releaseLayoutChangesObserver} =
+  require("devtools/server/actors/layout");
 
 const FONT_FAMILY_PREVIEW_TEXT = "The quick brown fox jumps over the lazy dog";
 const FONT_FAMILY_PREVIEW_TEXT_SIZE = 20;
 const PSEUDO_CLASSES = [":hover", ":active", ":focus"];
 const HIDDEN_CLASS = "__fx-devtools-hide-shortcut__";
 const XHTML_NS = "http://www.w3.org/1999/xhtml";
 const IMAGE_FETCHING_TIMEOUT = 500;
 // The possible completions to a ':' with added score to give certain values
@@ -172,16 +174,20 @@ exports.setValueSummaryLength = function
  */
 var NodeActor = exports.NodeActor = protocol.ActorClass({
   typeName: "domnode",
 
   initialize: function(walker, node) {
     protocol.Actor.prototype.initialize.call(this, null);
     this.walker = walker;
     this.rawNode = node;
+
+    // Storing the original display of the node, to track changes when reflows
+    // occur
+    this.wasDisplayed = this.isDisplayed;
   },
 
   toString: function() {
     return "[NodeActor " + this.actorID + " for " + this.rawNode.toString() + "]";
   },
 
   /**
    * Instead of storing a connection object, the NodeActor gets its connection
@@ -222,16 +228,18 @@ var NodeActor = exports.NodeActor = prot
       // doctype attributes
       name: this.rawNode.name,
       publicId: this.rawNode.publicId,
       systemId: this.rawNode.systemId,
 
       attrs: this.writeAttrs(),
 
       pseudoClassLocks: this.writePseudoClassLocks(),
+
+      isDisplayed: this.isDisplayed,
     };
 
     if (this.isDocumentElement()) {
       form.isDocumentElement = true;
     }
 
     if (this.rawNode.nodeValue) {
       // We only include a short version of the value if it's longer than
@@ -242,16 +250,38 @@ var NodeActor = exports.NodeActor = prot
       } else {
         form.shortValue = this.rawNode.nodeValue;
       }
     }
 
     return form;
   },
 
+  get computedStyle() {
+    if (Cu.isDeadWrapper(this.rawNode) ||
+        this.rawNode.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE ||
+        !this.rawNode.ownerDocument) {
+      return null;
+    }
+    return this.rawNode.ownerDocument.defaultView.getComputedStyle(this.rawNode);
+  },
+
+  /**
+   * Is the node's display computed style value other than "none"
+   */
+  get isDisplayed() {
+    let style = this.computedStyle;
+    if (!style) {
+      // Consider all non-element nodes as displayed
+      return true;
+    } else {
+      return style.display !== "none";
+    }
+  },
+
   writeAttrs: function() {
     if (!this.rawNode.attributes) {
       return undefined;
     }
     return [{namespace: attr.namespace, name: attr.name, value: attr.value }
             for (attr of this.rawNode.attributes)];
   },
 
@@ -543,16 +573,22 @@ let NodeFront = protocol.FrontClass(Node
 
   get attributes() this._form.attrs,
 
   get pseudoClassLocks() this._form.pseudoClassLocks || [],
   hasPseudoClassLock: function(pseudo) {
     return this.pseudoClassLocks.some(locked => locked === pseudo);
   },
 
+  get isDisplayed() {
+    // The NodeActor's form contains the isDisplayed information as a boolean
+    // starting from FF32. Before that, the property is missing
+    return "isDisplayed" in this._form ? this._form.isDisplayed : true;
+  },
+
   getNodeValue: protocol.custom(function() {
     if (!this.incompleteValue) {
       return delayedResolve(new ShortLongString(this.shortValue));
     } else {
       return this._getNodeValue();
     }
   }, {
     impl: "_getNodeValue"
@@ -831,16 +867,20 @@ var WalkerActor = protocol.ActorClass({
       type: "pickerNodeHovered",
       node: Arg(0, "disconnectedNode")
     },
     "highlighter-ready" : {
       type: "highlighter-ready"
     },
     "highlighter-hide" : {
       type: "highlighter-hide"
+    },
+    "display-change" : {
+      type: "display-change",
+      nodes: Arg(0, "array:domnode")
     }
   },
 
   /**
    * Create the WalkerActor
    * @param DebuggerServerConnection conn
    *    The server connection.
    */
@@ -870,16 +910,20 @@ var WalkerActor = protocol.ActorClass({
     this.onFrameUnload = this.onFrameUnload.bind(this);
 
     events.on(tabActor, "will-navigate", this.onFrameUnload);
     events.on(tabActor, "navigate", this.onFrameLoad);
 
     // Ensure that the root document node actor is ready and
     // managed.
     this.rootNode = this.document();
+
+    this.reflowObserver = getLayoutChangesObserver(this.tabActor);
+    this._onReflows = this._onReflows.bind(this);
+    this.reflowObserver.on("reflows", this._onReflows);
   },
 
   // Returns the JSON representation of this object over the wire.
   form: function() {
     return {
       actor: this.actorID,
       root: this.rootNode.form()
     }
@@ -889,27 +933,32 @@ var WalkerActor = protocol.ActorClass({
     return "[WalkerActor " + this.actorID + "]";
   },
 
   destroy: function() {
     this._hoveredNode = null;
     this.clearPseudoClassLocks();
     this._activePseudoClassLocks = null;
     this.rootDoc = null;
+
+    this.reflowObserver.off("reflows", this._onReflows);
+    this.reflowObserver = null;
+    releaseLayoutChangesObserver(this.tabActor);
+
     events.emit(this, "destroyed");
     protocol.Actor.prototype.destroy.call(this);
   },
 
   release: method(function() {}, { release: true }),
 
   unmanage: function(actor) {
     if (actor instanceof NodeActor) {
       if (this._activePseudoClassLocks &&
           this._activePseudoClassLocks.has(actor)) {
-        this.clearPsuedoClassLocks(actor);
+        this.clearPseudoClassLocks(actor);
       }
       this._refMap.delete(actor.rawNode);
     }
     protocol.Actor.prototype.unmanage.call(this, actor);
   },
 
   _ref: function(node) {
     let actor = this._refMap.get(node);
@@ -923,16 +972,34 @@ var WalkerActor = protocol.ActorClass({
     this._refMap.set(node, actor);
 
     if (node.nodeType === Ci.nsIDOMNode.DOCUMENT_NODE) {
       this._watchDocument(actor);
     }
     return actor;
   },
 
+  _onReflows: function(reflows) {
+    // Going through the nodes the walker knows about, see which ones have
+    // had their display changed and send a display-change event if any
+    let changes = [];
+    for (let [node, actor] of this._refMap) {
+      let isDisplayed = actor.isDisplayed;
+      if (isDisplayed !== actor.wasDisplayed) {
+        changes.push(actor);
+        // Updating the original value
+        actor.wasDisplayed = isDisplayed;
+      }
+    }
+
+    if (changes.length) {
+      events.emit(this, "display-change", changes);
+    }
+  },
+
   /**
    * This is kept for backward-compatibility reasons with older remote targets.
    * Targets prior to bug 916443.
    *
    * pick/cancelPick are used to pick a node on click on the content
    * document. But in their implementation prior to bug 916443, they don't allow
    * highlighting on hover.
    * The client-side now uses the highlighter actor's pick and cancelPick