From a3b665620e534bdb5a6a23d52c5a33ce16840c90 Mon Sep 17 00:00:00 2001 From: Patrick Brosset Date: Thu, 5 Jun 2014 14:50:03 +0200 Subject: [PATCH] Bug 911209 - Display hidden nodes differently in the markup-view; r=miker --- browser/devtools/markupview/markup-view.js | 32 +++++ browser/devtools/markupview/test/browser.ini | 3 + ...rowser_markupview_node_not_displayed_01.js | 33 +++++ ...rowser_markupview_node_not_displayed_02.js | 132 ++++++++++++++++++ .../test/doc_markup_not_displayed.html | 17 +++ .../themes/shared/devtools/markup-view.css | 6 + toolkit/devtools/server/actors/inspector.js | 70 +++++++++- 7 files changed, 292 insertions(+), 1 deletion(-) create mode 100644 browser/devtools/markupview/test/browser_markupview_node_not_displayed_01.js create mode 100644 browser/devtools/markupview/test/browser_markupview_node_not_displayed_02.js create mode 100644 browser/devtools/markupview/test/doc_markup_not_displayed.html diff --git a/browser/devtools/markupview/markup-view.js b/browser/devtools/markupview/markup-view.js index b15e69da929..ed3da561876 100644 --- a/browser/devtools/markupview/markup-view.js +++ b/browser/devtools/markupview/markup-view.js @@ -85,6 +85,9 @@ function MarkupView(aInspector, aFrame, aControllerWindow) { 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(); @@ -614,6 +617,19 @@ 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. @@ -1110,6 +1126,9 @@ MarkupView.prototype = { 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; @@ -1268,6 +1287,9 @@ function MarkupContainer(aMarkupView, aNode, aInspector) { // Prepare the image preview tooltip data if any this._prepareImagePreview(); + + // Marking the node as shown or hidden + this.isDisplayed = this.node.isDisplayed; } MarkupContainer.prototype = { @@ -1342,6 +1364,16 @@ MarkupContainer.prototype = { }); }, + /** + * 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 diff --git a/browser/devtools/markupview/test/browser.ini b/browser/devtools/markupview/test/browser.ini index c22bc5a2098..d10ae0971a1 100644 --- a/browser/devtools/markupview/test/browser.ini +++ b/browser/devtools/markupview/test/browser.ini @@ -6,6 +6,7 @@ support-files = 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 @@ -26,6 +27,8 @@ support-files = [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] diff --git a/browser/devtools/markupview/test/browser_markupview_node_not_displayed_01.js b/browser/devtools/markupview/test/browser_markupview_node_not_displayed_01.js new file mode 100644 index 00000000000..d713ba39575 --- /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); + } +}); diff --git a/browser/devtools/markupview/test/browser_markupview_node_not_displayed_02.js b/browser/devtools/markupview/test/browser_markupview_node_not_displayed_02.js new file mode 100644 index 00000000000..7009410fdda --- /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 = ""; + 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; +} diff --git a/browser/devtools/markupview/test/doc_markup_not_displayed.html b/browser/devtools/markupview/test/doc_markup_not_displayed.html new file mode 100644 index 00000000000..21a22754694 --- /dev/null +++ b/browser/devtools/markupview/test/doc_markup_not_displayed.html @@ -0,0 +1,17 @@ + + + + + + +
+ + + +
+ + \ No newline at end of file diff --git a/browser/themes/shared/devtools/markup-view.css b/browser/themes/shared/devtools/markup-view.css index 2b021e6364c..4a6a2875c89 100644 --- a/browser/themes/shared/devtools/markup-view.css +++ b/browser/themes/shared/devtools/markup-view.css @@ -30,6 +30,12 @@ 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; } diff --git a/toolkit/devtools/server/actors/inspector.js b/toolkit/devtools/server/actors/inspector.js index 5766d1a5816..37064fae30a 100644 --- a/toolkit/devtools/server/actors/inspector.js +++ b/toolkit/devtools/server/actors/inspector.js @@ -62,6 +62,8 @@ 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; @@ -177,6 +179,10 @@ var NodeActor = exports.NodeActor = protocol.ActorClass({ 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() { @@ -227,6 +233,8 @@ var NodeActor = exports.NodeActor = protocol.ActorClass({ attrs: this.writeAttrs(), pseudoClassLocks: this.writePseudoClassLocks(), + + isDisplayed: this.isDisplayed, }; if (this.isDocumentElement()) { @@ -247,6 +255,29 @@ var NodeActor = exports.NodeActor = protocol.ActorClass({ return form; }, + get computedStyle() { + if (Cu.isDeadWrapper(this.rawNode) || + this.rawNode.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE || + !this.rawNode.ownerDocument || + !this.rawNode.ownerDocument.defaultView) { + 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; @@ -548,6 +579,12 @@ let NodeFront = protocol.FrontClass(NodeActor, { 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)); @@ -836,6 +873,10 @@ var WalkerActor = protocol.ActorClass({ }, "highlighter-hide" : { type: "highlighter-hide" + }, + "display-change" : { + type: "display-change", + nodes: Arg(0, "array:domnode") } }, @@ -875,6 +916,10 @@ var WalkerActor = protocol.ActorClass({ // 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. @@ -894,6 +939,11 @@ var WalkerActor = protocol.ActorClass({ 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); }, @@ -904,7 +954,7 @@ var WalkerActor = protocol.ActorClass({ if (actor instanceof NodeActor) { if (this._activePseudoClassLocks && this._activePseudoClassLocks.has(actor)) { - this.clearPsuedoClassLocks(actor); + this.clearPseudoClassLocks(actor); } this._refMap.delete(actor.rawNode); } @@ -928,6 +978,24 @@ var WalkerActor = protocol.ActorClass({ 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.