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 206891 52068b669c2af0023292655c64a568a63e94608f
parent 206890 e160cbe83f39d9cd6bacadd1f0a44ce54f1de0d0
child 206892 8fac243930da445352fd91c5d254b7253ba06d54
push id494
push userraliiev@mozilla.com
push dateMon, 25 Aug 2014 18:42:16 +0000
treeherdermozilla-release@a3cc3e46b571 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmiker
bugs911209
milestone32.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 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