Bug 1342310 - preserve grid highlighter only when reloading the page;r=zer0 draft
authorJulian Descottes <jdescottes@mozilla.com>
Mon, 06 Mar 2017 19:19:58 +0100
changeset 495916 2b89bf5949fbaef16b6cab49946f18c52c1f65a2
parent 495547 47cbfa55cfb6a67f40dbf1f9802894c971a4b097
child 548501 7f6f310a9ed00b1ab49a37fb270328ed27a829c4
push id48475
push userjdescottes@mozilla.com
push dateThu, 09 Mar 2017 15:13:39 +0000
reviewerszer0
bugs1342310
milestone55.0a1
Bug 1342310 - preserve grid highlighter only when reloading the page;r=zer0 Store url in highlighters-overlay grid state to avoid reapplying a state after navigating on a different page. MozReview-Commit-ID: 6rTsAPuPhvx
devtools/client/inspector/inspector.js
devtools/client/inspector/rules/test/browser_rules_grid-highlighter-restored-after-reload.js
devtools/client/inspector/shared/highlighters-overlay.js
--- a/devtools/client/inspector/inspector.js
+++ b/devtools/client/inspector/inspector.js
@@ -951,29 +951,28 @@ Inspector.prototype = {
 
     this.teardownToolbar();
     this.breadcrumbs.destroy();
     this.selection.off("new-node-front", this.onNewSelection);
     this.selection.off("detached-front", this.onDetached);
 
     let markupDestroyer = this._destroyMarkup();
 
+    this.highlighters.destroy();
+    this.search.destroy();
+
     this._toolbox = null;
     this.breadcrumbs = null;
     this.panelDoc = null;
     this.panelWin.inspector = null;
     this.panelWin = null;
     this.sidebar = null;
     this.store = null;
     this.target = null;
-
-    this.highlighters.destroy();
     this.highlighters = null;
-
-    this.search.destroy();
     this.search = null;
     this.searchBox = null;
 
     this._panelDestroyer = promise.all([
       sidebarDestroyer,
       markupDestroyer,
       cssPropertiesDestroyer
     ]);
--- a/devtools/client/inspector/rules/test/browser_rules_grid-highlighter-restored-after-reload.js
+++ b/devtools/client/inspector/rules/test/browser_rules_grid-highlighter-restored-after-reload.js
@@ -13,16 +13,29 @@ const TEST_URI = `
     }
   </style>
   <div id="grid">
     <div id="cell1">cell1</div>
     <div id="cell2">cell2</div>
   </div>
 `;
 
+const OTHER_URI = `
+  <style type='text/css'>
+    #grid {
+      display: grid;
+    }
+  </style>
+  <div id="grid">
+    <div id="cell1">cell1</div>
+    <div id="cell2">cell2</div>
+    <div id="cell3">cell3</div>
+  </div>
+`;
+
 add_task(function* () {
   yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
 
   info("Check that the grid highlighter can be displayed");
   let {inspector, view} = yield openRuleView();
   let {highlighters} = view;
 
   yield selectNode("#grid", inspector);
@@ -32,15 +45,24 @@ add_task(function* () {
   info("Toggling ON the CSS grid highlighter from the rule-view.");
   let onHighlighterShown = highlighters.once("grid-highlighter-shown");
   gridToggle.click();
   yield onHighlighterShown;
 
   ok(highlighters.gridHighlighterShown, "CSS grid highlighter is shown.");
 
   info("Reload the page, expect the highlighter to be displayed once again");
-  onHighlighterShown = highlighters.once("grid-highlighter-shown");
+  let onStateRestored = highlighters.once("state-restored");
   yield refreshTab(gBrowser.selectedTab);
-  yield onHighlighterShown;
+  let { restored } = yield onStateRestored;
+  ok(restored, "The highlighter state was restored");
 
   info("Check that the grid highlighter can be displayed after reloading the page");
   ok(highlighters.gridHighlighterShown, "CSS grid highlighter is shown.");
+
+  info("Navigate to another URL, and check that the highlighter is hidden");
+  let otherUri = "data:text/html;charset=utf-8," + encodeURIComponent(OTHER_URI);
+  onStateRestored = highlighters.once("state-restored");
+  yield navigateTo(inspector, otherUri);
+  ({ restored } = yield onStateRestored);
+  ok(!restored, "The highlighter state was not restored");
+  ok(!highlighters.gridHighlighterShown, "CSS grid highlighter is hidden.");
 });
--- a/devtools/client/inspector/shared/highlighters-overlay.js
+++ b/devtools/client/inspector/shared/highlighters-overlay.js
@@ -41,16 +41,20 @@ function HighlightersOverlay(inspector) 
 
   this.onClick = this.onClick.bind(this);
   this.onMouseMove = this.onMouseMove.bind(this);
   this.onMouseOut = this.onMouseOut.bind(this);
   this.onWillNavigate = this.onWillNavigate.bind(this);
   this.onNavigate = this.onNavigate.bind(this);
   this._handleRejection = this._handleRejection.bind(this);
 
+  // Add target events, not specific to a given view.
+  this.inspector.target.on("navigate", this.onNavigate);
+  this.inspector.target.on("will-navigate", this.onWillNavigate);
+
   EventEmitter.decorate(this);
 }
 
 HighlightersOverlay.prototype = {
   get isRuleView() {
     return this.inspector.sidebar.getCurrentTabID() == "ruleview";
   },
 
@@ -67,19 +71,16 @@ HighlightersOverlay.prototype = {
       return;
     }
 
     let el = view.element;
     el.addEventListener("click", this.onClick, true);
     el.addEventListener("mousemove", this.onMouseMove);
     el.addEventListener("mouseout", this.onMouseOut);
     el.ownerDocument.defaultView.addEventListener("mouseout", this.onMouseOut);
-
-    this.inspector.target.on("navigate", this.onNavigate);
-    this.inspector.target.on("will-navigate", this.onWillNavigate);
   },
 
   /**
    * Remove the overlay from the given view. This will stop tracking mouse movement and
    * showing highlighters.
    *
    * @param  {CssRuleView|CssComputedView|LayoutView} view
    *         Either the rule-view or computed-view panel to remove the highlighters
@@ -89,19 +90,16 @@ HighlightersOverlay.prototype = {
     if (!this.supportsHighlighters) {
       return;
     }
 
     let el = view.element;
     el.removeEventListener("click", this.onClick, true);
     el.removeEventListener("mousemove", this.onMouseMove);
     el.removeEventListener("mouseout", this.onMouseOut);
-
-    this.inspector.target.off("navigate", this.onNavigate);
-    this.inspector.target.off("will-navigate", this.onWillNavigate);
   },
 
   /**
    * Toggle the grid highlighter for the given grid container element.
    *
    * @param  {NodeFront} node
    *         The NodeFront of the grid container element to highlight.
    * @param  {Object} options
@@ -132,24 +130,29 @@ HighlightersOverlay.prototype = {
 
     let isShown = yield highlighter.show(node, options);
     if (!isShown) {
       return;
     }
 
     this._toggleRuleViewGridIcon(node, true);
 
-    node.getUniqueSelector().then(selector => {
+    try {
       // Save grid highlighter state.
-      this.state.grid = { selector, options };
+      let { url } = this.inspector.target;
+      let selector = yield node.getUniqueSelector();
+      this.state.grid = { selector, options, url };
+
       this.gridHighlighterShown = node;
       // Emit the NodeFront of the grid container element that the grid highlighter was
       // shown for.
       this.emit("grid-highlighter-shown", node, options);
-    }).catch(this._handleRejection);
+    } catch (e) {
+      this._handleRejection(e);
+    }
   }),
 
   /**
    * Hide the grid highlighter for the given grid container element.
    *
    * @param  {NodeFront} node
    *         The NodeFront of the grid container element to unhighlight.
    */
@@ -174,32 +177,37 @@ HighlightersOverlay.prototype = {
 
   /**
    * Restore the saved highlighter states.
    *
    * @return {Promise} that resolves when the highlighter state was restored, and the
    *          expected highlighters are displayed.
    */
   restoreState: Task.async(function* () {
-    let { selector, options } = this.state.grid;
+    let { selector, options, url } = this.state.grid;
 
-    if (!selector) {
+    if (!selector || url !== this.inspector.target.url) {
+      // Bail out if no selector was saved, or if we are on a different page.
+      this.emit("state-restored", { restored: false });
       return;
     }
 
     // Wait for the new root to be ready in the inspector.
     yield this.onInspectorNewRoot;
 
     let walker = this.inspector.walker;
     let rootNode = yield walker.getRootNode();
     let nodeFront = yield walker.querySelector(rootNode, selector);
 
     if (nodeFront) {
       yield this.showGridHighlighter(nodeFront, options);
+      this.emit("state-restored", { restored: true });
     }
+
+    this.emit("state-restored", { restored: false });
   }),
 
   /**
    * Get a highlighter front given a type. It will only be initialized once.
    *
    * @param  {String} type
    *         The highlighter type. One of this.highlighters.
    * @return {Promise} that resolves to the highlighter
@@ -365,19 +373,23 @@ HighlightersOverlay.prototype = {
     // Otherwise, hide the highlighter.
     this._lastHovered = null;
     this._hideHoveredHighlighter();
   },
 
   /**
    * Restore saved highlighter state after navigate.
    */
-  onNavigate: function () {
-    this.restoreState().catch(this._handleRejection);
-  },
+  onNavigate: Task.async(function* () {
+    try {
+      yield this.restoreState();
+    } catch (e) {
+      this._handleRejection(e);
+    }
+  }),
 
   /**
    * Clear saved highlighter shown properties on will-navigate.
    */
   onWillNavigate: function () {
     this.gridHighlighterShown = null;
     this.hoveredHighlighterShown = null;
     this.selectorHighlighterShown = null;
@@ -393,16 +405,20 @@ HighlightersOverlay.prototype = {
   destroy: function () {
     for (let type in this.highlighters) {
       if (this.highlighters[type]) {
         this.highlighters[type].finalize();
         this.highlighters[type] = null;
       }
     }
 
+    // Remove target events.
+    this.inspector.target.off("navigate", this.onNavigate);
+    this.inspector.target.off("will-navigate", this.onWillNavigate);
+
     this._lastHovered = null;
 
     this.inspector = null;
     this.highlighters = null;
     this.highlighterUtils = null;
     this.supportsHighlighters = null;
     this.gridHighlighterShown = null;
     this.hoveredHighlighterShown = null;