Bug 1192421 - Don't clear rule-view when changing selection; r=pbro

This is to prevent the rule-view from flickering when a new node is
selected.
This also makes the rule-vuew non-interactive (not responsding to
user events) during the refresh time.
If this takes too long, the rule-view is hidden after a delay.
This also fixes a race condition in style-inspector-overlay's
highlighters due to _hideCurrent being unnecessarily async.

--HG--
extra : rebase_source : 87232abdd49cd149a482d6212aa80e960e8918c6

devtools/client/styleinspector/rule-view.js

@@ -1317,6 +1317,7 @@ function CssRuleView(inspector, document, store, pageStyle) {
 
   this._outputParser = new OutputParser(document);
 
+  this._onKeydown = this._onKeydown.bind(this);
   this._onKeypress = this._onKeypress.bind(this);
   this._onAddRule = this._onAddRule.bind(this);
   this._onContextMenu = this._onContextMenu.bind(this);
@@ -1342,6 +1343,7 @@ function CssRuleView(inspector, document, store, pageStyle) {
 
   this.searchClearButton.hidden = true;
 
+  this.styleDocument.addEventListener("keydown", this._onKeydown);
   this.styleDocument.addEventListener("keypress", this._onKeypress);
   this.element.addEventListener("copy", this._onCopy);
   this.element.addEventListener("contextmenu", this._onContextMenu);
@@ -1739,9 +1741,7 @@ CssRuleView.prototype = {
     // Reselect the currently selected element
     let refreshOnPrefs = [PREF_UA_STYLES, PREF_DEFAULT_COLOR_UNIT];
     if (refreshOnPrefs.indexOf(pref) > -1) {
-      let element = this._viewedElement;
-      this._viewedElement = null;
-      this.selectElement(element);
+      this.selectElement(this._viewedElement, true);
     }
   },
 
@@ -1912,6 +1912,8 @@ CssRuleView.prototype = {
     this.highlighters.destroy();
 
     // Remove bound listeners
+    this.styleDocument.removeEventListener("keydown", this._onKeydown);
+    this.styleDocument.removeEventListener("keypress", this._onKeypress);
     this.element.removeEventListener("copy", this._onCopy);
     this.element.removeEventListener("contextmenu", this._onContextMenu);
     this.addRuleButton.removeEventListener("click", this._onAddRule);
@@ -1949,14 +1951,35 @@ CssRuleView.prototype = {
     this.popup.destroy();
   },
 
+
+
+  /**
+   * Mark the view as selecting an element, disabling all interaction, and
+   * visually clearing the view after a few milliseconds to avoid confusion
+   * about which element's styles the rule view shows.
+   */
+  _startSelectingElement: function() {
+    this.element.classList.add("non-interactive");
+  },
+
+  /**
+   * Mark the view as no longer selecting an element, re-enabling interaction.
+   */
+  _stopSelectingElement: function() {
+    this.element.classList.remove("non-interactive");
+  },
+
   /**
    * Update the view with a new selected element.
    *
    * @param {NodeActor} element
    *        The node whose style rules we'll inspect.
+   * @param {Boolean} allowRefresh
+   *        Update the view even if the element is the same as last time.
    */
-  selectElement: function(element) {
-    if (this._viewedElement === element) {
+  selectElement: function(element, allowRefresh=false) {
+    let refresh = (this._viewedElement === element);
+    if (refresh && !allowRefresh) {
       return promise.resolve(undefined);
     }
 
@@ -1964,32 +1987,47 @@ CssRuleView.prototype = {
       this.popup.hidePopup();
     }
 
-    this.clear();
-    this.clearPseudoClassPanel();
-
+    this.clear(false);
     this._viewedElement = element;
+
+    this.clearPseudoClassPanel();
     this.refreshAddRuleButtonState();
 
     if (!this._viewedElement) {
+      this._stopSelectingElement();
+      this._clearRules();
       this._showEmpty();
       this.refreshPseudoClassPanel();
       return promise.resolve(undefined);
     }
 
-    this._elementStyle = new ElementStyle(element, this.store,
+    let elementStyle = new ElementStyle(element, this.store,
       this.pageStyle, this.showUserAgentStyles);
+    this._elementStyle = elementStyle;
+
+    this._startSelectingElement();
 
     return this._elementStyle.init().then(() => {
-      if (this._viewedElement === element) {
+      if (this._elementStyle === elementStyle) {
         return this._populate();
       }
     }).then(() => {
-      if (this._viewedElement === element) {
+      if (this._elementStyle === elementStyle) {
+        if (!refresh) {
+          this.element.scrollTop = 0;
+        }
+        this._stopSelectingElement();
         this._elementStyle.onChanged = () => {
           this._changed();
         };
       }
-    }).then(null, console.error);
+    }).then(null, e => {
+      if (this._elementStyle === elementStyle) {
+        this._stopSelectingElement();
+        this._clearRules();
+      }
+      console.error(e);
+    });
   },
 
   /**
@@ -2010,7 +2048,7 @@ CssRuleView.prototype = {
     }
 
     return promise.all(promises).then(() => {
-      return this._populate(true);
+      return this._populate();
     });
   },
 
@@ -2053,16 +2091,14 @@ CssRuleView.prototype = {
     }
   },
 
-  _populate: function(clearRules = false) {
+  _populate: function() {
     let elementStyle = this._elementStyle;
     return this._elementStyle.populate().then(() => {
       if (this._elementStyle !== elementStyle || this.isDestroyed) {
         return;
       }
 
-      if (clearRules) {
-        this._clearRules();
-      }
+      this._clearRules();
       this._createEditors();
 
       this.refreshPseudoClassPanel();
@@ -2090,18 +2126,18 @@ CssRuleView.prototype = {
    * Clear the rules.
    */
   _clearRules: function() {
-    while (this.element.hasChildNodes()) {
-      this.element.removeChild(this.element.lastChild);
-    }
+    this.element.innerHTML = "";
   },
 
   /**
    * Clear the rule view.
    */
-  clear: function() {
+  clear: function(clearDom=true) {
     this.lastSelectorIcon = null;
 
-    this._clearRules();
+    if (clearDom) {
+      this._clearRules();
+    }
     this._viewedElement = null;
 
     if (this._elementStyle) {
@@ -2583,6 +2619,16 @@ CssRuleView.prototype = {
     this.inspector.togglePseudoClass(target.value);
   },
 
+  /**
+   * Handle the keydown event in the rule view.
+   */
+  _onKeydown: function(event) {
+    if (this.element.classList.contains("non-interactive") &&
+        (event.code === "Enter" || event.code === " ")) {
+      event.preventDefault();
+    }
+  },
+
   /**
    * Handle the keypress event in the rule view.
    */

devtools/client/styleinspector/ruleview.css

@@ -24,6 +24,12 @@ body {
   flex: 1;
 }
 
+#ruleview-container.non-interactive {
+  pointer-events: none;
+  visibility: collapse;
+  transition: visibility 0.25s;
+}
+
 .devtools-toolbar {
   width: 100%;
   display: flex;

devtools/client/styleinspector/style-inspector-overlays.js

@@ -55,7 +55,6 @@ function HighlightersOverlay(view) {
   this._onMouseMove = this._onMouseMove.bind(this);
   this._onMouseLeave = this._onMouseLeave.bind(this);
 
-  this.promises = {};
   this.highlighters = {};
 
   // Only initialize the overlay if at least one of the highlighter types is
@@ -176,20 +175,21 @@ HighlightersOverlay.prototype = {
    * Hide the currently shown highlighter
    */
   _hideCurrent: function() {
-    if (this.highlighterShown) {
-      this._getHighlighter(this.highlighterShown).then(highlighter => {
-        // For some reason, the call to highlighter.hide doesn't always return a
-        // promise. This causes some tests to fail when trying to install a
-        // rejection handler on the result of the call. To avoid this, check
-        // whether the result is truthy before installing the handler.
-        let promise = highlighter.hide();
-        if (promise) {
-          promise.then(null, Cu.reportError);
-        }
-        this.highlighterShown = null;
-        this.emit("highlighter-hidden");
-      });
+    if (!this.highlighterShown || !this.highlighters[this.highlighterShown]) {
+      return;
     }
+
+    // For some reason, the call to highlighter.hide doesn't always return a
+    // promise. This causes some tests to fail when trying to install a
+    // rejection handler on the result of the call. To avoid this, check
+    // whether the result is truthy before installing the handler.
+    let promise = this.highlighters[this.highlighterShown].hide();
+    if (promise) {
+      promise.then(null, e => console.error(e));
+    }
+
+    this.highlighterShown = null;
+    this.emit("highlighter-hidden");
   },
 
   /**
@@ -200,11 +200,11 @@ HighlightersOverlay.prototype = {
   _getHighlighter: function(type) {
     let utils = this.highlighterUtils;
 
-    if (this.promises[type]) {
-      return this.promises[type];
+    if (this.highlighters[type]) {
+      return promise.resolve(this.highlighters[type]);
     }
 
-    return this.promises[type] = utils.getHighlighterByType(type).then(highlighter => {
+    return utils.getHighlighterByType(type).then(highlighter => {
       this.highlighters[type] = highlighter;
       return highlighter;
     });
@@ -224,7 +224,6 @@ HighlightersOverlay.prototype = {
       }
     }
 
-    this.promises = null;
     this.view = null;
     this.highlighterUtils = null;
 

devtools/client/styleinspector/test/browser.ini

@@ -150,6 +150,7 @@ skip-if = (os == "win" && debug) || e10s # bug 963492: win. bug 1040653: e10s.
 [browser_ruleview_pseudo-element_02.js]
 skip-if = e10s # Bug 1090340
 [browser_ruleview_pseudo_lock_options.js]
+[browser_ruleview_refresh-no-flicker.js]
 [browser_ruleview_refresh-on-attribute-change_01.js]
 [browser_ruleview_refresh-on-attribute-change_02.js]
 [browser_ruleview_refresh-on-style-change.js]

devtools/client/styleinspector/test/browser_ruleview_refresh-no-flicker.js

@@ -0,0 +1,38 @@
+/* vim: set ft=javascript 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 the rule view does not go blank while selecting a new node.
+
+const TESTCASE_URI = 'data:text/html;charset=utf-8,' +
+                     '<div id="testdiv" style="font-size:10px;">Test div!</div>';
+
+add_task(function*() {
+  yield addTab(TESTCASE_URI);
+
+  info("Opening the rule view and selecting the test node");
+  let {toolbox, inspector, view} = yield openRuleView();
+  let testdiv = yield getNodeFront("#testdiv", inspector);
+  yield selectNode(testdiv, inspector);
+
+  let htmlBefore = view.element.innerHTML;
+  ok(htmlBefore.indexOf("font-size") > -1,
+     "The rule view should contain a font-size property.");
+
+  // Do the selectNode call manually, because otherwise it's hard to guarantee
+  // that we can make the below checks at a reasonable time.
+  info("refreshing the node");
+  let p = view.selectElement(testdiv, true);
+  is(view.element.innerHTML, htmlBefore,
+     "The rule view is unchanged during selection.");
+  ok(view.element.classList.contains("non-interactive"),
+     "The rule view is marked non-interactive.");
+  yield p;
+
+  info("node refreshed");
+  ok(!view.element.classList.contains("non-interactive"),
+     "The rule view is marked interactive again.");
+});
+

devtools/client/styleinspector/test/browser_styleinspector_transform-highlighter-02.js

@@ -25,24 +25,17 @@ add_task(function*() {
   let hs = view.highlighters;
 
   ok(!hs.highlighters[TYPE], "No highlighter exists in the rule-view (1)");
-  ok(!hs.promises[TYPE],
-    "No highlighter is being created in the rule-view (1)");
 
   info("Faking a mousemove on a non-transform property");
   let {valueSpan} = getRuleViewProperty(view, "body", "color");
   hs._onMouseMove({target: valueSpan});
   ok(!hs.highlighters[TYPE], "No highlighter exists in the rule-view (2)");
-  ok(!hs.promises[TYPE],
-    "No highlighter is being created in the rule-view (2)");
 
   info("Faking a mousemove on a transform property");
   ({valueSpan} = getRuleViewProperty(view, "body", "transform"));
   let onHighlighterShown = hs.once("highlighter-shown");
   hs._onMouseMove({target: valueSpan});
   yield onHighlighterShown;
-  ok(hs.promises[TYPE], "The highlighter is being initialized");
-  let h = yield hs.promises[TYPE];
-  is(h, hs.highlighters[TYPE], "The initialized highlighter is the right one");
 
   let onComputedViewReady = inspector.once("computed-view-refreshed");
   let {view: cView} = yield openComputedView();
@@ -50,22 +43,15 @@ add_task(function*() {
   hs = cView.highlighters;
 
   ok(!hs.highlighters[TYPE], "No highlighter exists in the computed-view (1)");
-  ok(!hs.promises[TYPE],
-    "No highlighter is being created in the computed-view (1)");
 
   info("Faking a mousemove on a non-transform property");
   ({valueSpan} = getComputedViewProperty(cView, "color"));
   hs._onMouseMove({target: valueSpan});
   ok(!hs.highlighters[TYPE], "No highlighter exists in the computed-view (2)");
-  ok(!hs.promises[TYPE],
-    "No highlighter is being created in the computed-view (2)");
 
   info("Faking a mousemove on a transform property");
   ({valueSpan} = getComputedViewProperty(cView, "transform"));
   onHighlighterShown = hs.once("highlighter-shown");
   hs._onMouseMove({target: valueSpan});
   yield onHighlighterShown;
-  ok(hs.promises[TYPE], "The highlighter is being initialized");
-  h = yield hs.promises[TYPE];
-  is(h, hs.highlighters[TYPE], "The initialized highlighter is the right one");
 });

devtools/client/styleinspector/test/browser_styleinspector_transform-highlighter-03.js

@@ -44,12 +44,13 @@ add_task(function*() {
       this.nodeFront = null;
       this.isShown = false;
       return promise.resolve();
-    }
+    },
+    finalize: function() {}
   };
 
   // Inject the mock highlighter in the rule-view
   let hs = view.highlighters;
-  hs.promises[TYPE] = promise.resolve(HighlighterFront);
+  hs.highlighters[TYPE] = HighlighterFront;
 
   let {valueSpan} = getRuleViewProperty(view, "body", "transform");
 

devtools/client/styleinspector/test/browser_styleinspector_transform-highlighter-04.js

@@ -39,7 +39,6 @@ add_task(function*() {
   hs._onMouseMove({target: valueSpan});
   ok(!hs.highlighters[TYPE],
     "No highlighter was created for the overriden property");
-  ok(!hs.promises[TYPE], "And no highlighter is being initialized either");
 
   info("Disabling the applied property");
   let classRuleEditor = getRuleViewRuleEditor(view, 1);
@@ -52,14 +51,10 @@ add_task(function*() {
   hs._onMouseMove({target: valueSpan});
   ok(!hs.highlighters[TYPE],
     "No highlighter was created for the disabled property");
-  ok(!hs.promises[TYPE], "And no highlighter is being initialized either");
 
   info("Faking a mousemove on the now unoverriden property");
   ({valueSpan} = getRuleViewProperty(view, "div", "transform"));
   let onHighlighterShown = hs.once("highlighter-shown");
   hs._onMouseMove({target: valueSpan});
   yield onHighlighterShown;
-  ok(hs.promises[TYPE], "The highlighter is being initialized now");
-  let h = yield hs.promises[TYPE];
-  is(h, hs.highlighters[TYPE], "The initialized highlighter is the right one");
 });