Bug 911678 - Inspector - inline style rules do not populate the CSSRuleView immediately. r=miker
authorBrian Grinstead <bgrinstead@mozilla.com>
Tue, 15 Oct 2013 07:09:49 -0500
changeset 165658 a8177c0528e68a47598a42981dfa18713e82a257
parent 165657 a352097e570fa9157aecd2a6b0cdf66f9bb2015e
child 165659 f12bb231ae374f70a5e08ee86e7cb45e56a06687
push id428
push userbbajaj@mozilla.com
push dateTue, 28 Jan 2014 00:16:25 +0000
treeherdermozilla-release@cd72a7ff3a75 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmiker
bugs911678
milestone27.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 911678 - Inspector - inline style rules do not populate the CSSRuleView immediately. r=miker
browser/devtools/inspector/inspector-panel.js
browser/devtools/inspector/test/browser_inspector_changes.js
browser/devtools/markupview/markup-view.js
browser/devtools/styleinspector/rule-view.js
browser/devtools/styleinspector/test/browser_ruleview_update.js
--- a/browser/devtools/inspector/inspector-panel.js
+++ b/browser/devtools/inspector/inspector-panel.js
@@ -739,16 +739,25 @@ InspectorPanel.prototype = {
       this.markup.deleteNode(this.selection.nodeFront);
     } else {
       // remove the node from content
       this.walker.removeNode(this.selection.nodeFront);
     }
   },
 
   /**
+  * Trigger a high-priority layout change for things that need to be
+  * updated immediately
+  */
+  immediateLayoutChange: function Inspector_immediateLayoutChange()
+  {
+    this.emit("layout-change");
+  },
+
+  /**
    * Schedule a low-priority change event for things like paint
    * and resize.
    */
   scheduleLayoutChange: function Inspector_scheduleLayoutChange(event)
   {
     // Filter out non browser window resize events (i.e. triggered by iframes)
     if (this.browser.contentWindow === event.target) {
       if (this._timer) {
--- a/browser/devtools/inspector/test/browser_inspector_changes.js
+++ b/browser/devtools/inspector/test/browser_inspector_changes.js
@@ -12,92 +12,143 @@ function test() {
   function createDocument()
   {
     doc.body.innerHTML = '<div id="testdiv">Test div!</div>';
     doc.title = "Inspector Change Test";
     openInspector(runInspectorTests);
   }
 
 
-  function getInspectorProp(aName)
+  function getInspectorComputedProp(aName)
   {
     let computedview = inspector.sidebar.getWindowForTab("computedview").computedview.view;
     for each (let view in computedview.propertyViews) {
       if (view.name == aName) {
         return view;
       }
     }
     return null;
   }
 
+  function getInspectorRuleProp(aName)
+  {
+    let ruleview = inspector.sidebar.getWindowForTab("ruleview").ruleview.view;
+    let inlineStyles = ruleview._elementStyle.rules[0];
+
+    for each (let prop in inlineStyles.textProps) {
+      if (prop.name == aName) {
+        return prop;
+      }
+    }
+    return null;
+  }
+
   function runInspectorTests(aInspector)
   {
     inspector = aInspector;
     inspector.sidebar.once("computedview-ready", function() {
       info("Computed View ready");
       inspector.sidebar.select("computedview");
 
       testDiv = doc.getElementById("testdiv");
 
       testDiv.style.fontSize = "10px";
 
       // Start up the style inspector panel...
-      inspector.once("computed-view-refreshed", stylePanelTests);
+      inspector.once("computed-view-refreshed", computedStylePanelTests);
 
       inspector.selection.setNode(testDiv);
     });
   }
 
-  function stylePanelTests()
+  function computedStylePanelTests()
   {
     let computedview = inspector.sidebar.getWindowForTab("computedview").computedview;
     ok(computedview, "Style Panel has a cssHtmlTree");
 
-    let propView = getInspectorProp("font-size");
+    let propView = getInspectorComputedProp("font-size");
     is(propView.value, "10px", "Style inspector should be showing the correct font size.");
 
-    inspector.once("computed-view-refreshed", stylePanelAfterChange);
-
-    testDiv.style.fontSize = "15px";
+    testDiv.style.cssText = "font-size: 15px; color: red;";
 
-    // FIXME: This shouldn't be needed but as long as we don't fix the bug
-    // where the rule/computed views are not updated when the selected node's
-    // styles change, it has to stay here
-    inspector.emit("layout-change");
+    // Wait until layout-change fires from mutation to skip earlier refresh event
+    inspector.once("layout-change", () => {
+      inspector.once("computed-view-refreshed", computedStylePanelAfterChange);
+    });
   }
 
-  function stylePanelAfterChange()
+  function computedStylePanelAfterChange()
   {
-    let propView = getInspectorProp("font-size");
+    let propView = getInspectorComputedProp("font-size");
     is(propView.value, "15px", "Style inspector should be showing the new font size.");
 
-    stylePanelNotActive();
+    let propView = getInspectorComputedProp("color");
+    is(propView.value, "#F00", "Style inspector should be showing the new color.");
+
+    computedStylePanelNotActive();
   }
 
-  function stylePanelNotActive()
+  function computedStylePanelNotActive()
   {
     // Tests changes made while the style panel is not active.
     inspector.sidebar.select("ruleview");
 
-    executeSoon(function() {
-      inspector.once("computed-view-refreshed", stylePanelAfterSwitch);
-      testDiv.style.fontSize = "20px";
-      inspector.sidebar.select("computedview");
+    testDiv.style.fontSize = "20px";
+    testDiv.style.color = "blue";
+    testDiv.style.textAlign = "center";
+
+    inspector.once("computed-view-refreshed", computedStylePanelAfterSwitch);
+    inspector.sidebar.select("computedview");
+  }
 
-      // FIXME: This shouldn't be needed but as long as we don't fix the bug
-      // where the rule/computed views are not updated when the selected node's
-      // styles change, it has to stay here
-      inspector.emit("layout-change");
-    });
+  function computedStylePanelAfterSwitch()
+  {
+    let propView = getInspectorComputedProp("font-size");
+    is(propView.value, "20px", "Style inspector should be showing the new font size.");
+
+    let propView = getInspectorComputedProp("color");
+    is(propView.value, "#00F", "Style inspector should be showing the new color.");
+
+    let propView = getInspectorComputedProp("text-align");
+    is(propView.value, "center", "Style inspector should be showing the new text align.");
+
+    rulePanelTests();
   }
 
-  function stylePanelAfterSwitch()
+  function rulePanelTests()
   {
-    let propView = getInspectorProp("font-size");
-    is(propView.value, "20px", "Style inspector should be showing the newest font size.");
+    inspector.sidebar.select("ruleview");
+    let ruleview = inspector.sidebar.getWindowForTab("ruleview").ruleview;
+    ok(ruleview, "Style Panel has a ruleview");
+
+    let propView = getInspectorRuleProp("text-align");
+    is(propView.value, "center", "Style inspector should be showing the new text align.");
+
+    testDiv.style.textAlign = "right";
+    testDiv.style.color = "lightgoldenrodyellow";
+    testDiv.style.fontSize = "3em";
+    testDiv.style.textTransform = "uppercase";
+
+
+    inspector.once("rule-view-refreshed", rulePanelAfterChange);
+  }
+
+  function rulePanelAfterChange()
+  {
+    let propView = getInspectorRuleProp("text-align");
+    is(propView.value, "right", "Style inspector should be showing the new text align.");
+
+    let propView = getInspectorRuleProp("color");
+    is(propView.value, "#FAFAD2", "Style inspector should be showing the new color.")
+
+    let propView = getInspectorRuleProp("font-size");
+    is(propView.value, "3em", "Style inspector should be showing the new font size.");
+
+    let propView = getInspectorRuleProp("text-transform");
+    is(propView.value, "uppercase", "Style inspector should be showing the new text transform.");
 
     finishTest();
   }
 
   function finishTest()
   {
     gBrowser.removeCurrentTab();
     finish();
--- a/browser/devtools/markupview/markup-view.js
+++ b/browser/devtools/markupview/markup-view.js
@@ -382,16 +382,17 @@ MarkupView.prototype = {
 
     return container;
   },
 
   /**
    * Mutation observer used for included nodes.
    */
   _mutationObserver: function(aMutations) {
+    let requiresLayoutChange = false;
     for (let mutation of aMutations) {
       let type = mutation.type;
       let target = mutation.target;
 
       if (mutation.type === "documentUnload") {
         // Treat this as a childList change of the child (maybe the protocol
         // should do this).
         type = "childList";
@@ -404,24 +405,33 @@ MarkupView.prototype = {
       let container = this._containers.get(target);
       if (!container) {
         // Container might not exist if this came from a load event for a node
         // we're not viewing.
         continue;
       }
       if (type === "attributes" || type === "characterData") {
         container.update();
+
+        // Auto refresh style properties on selected node when they change.
+        if (type === "attributes" && container.selected) {
+          requiresLayoutChange = true;
+        }
       } else if (type === "childList") {
         container.childrenDirty = true;
         // Update the children to take care of changes in the DOM
         // Passing true as the last parameter asks for mutation flashing of the
         // new nodes
         this._updateChildren(container, {flash: true});
       }
     }
+
+    if (requiresLayoutChange) {
+      this._inspector.immediateLayoutChange();
+    }
     this._waitForChildren().then(() => {
       this._flashMutatedNodes(aMutations);
       this._inspector.emit("markupmutation");
     });
   },
 
   /**
    * Given a list of mutations returned by the mutation observer, flash the
--- a/browser/devtools/styleinspector/rule-view.js
+++ b/browser/devtools/styleinspector/rule-view.js
@@ -1278,23 +1278,23 @@ CssRuleView.prototype = {
 
   /**
    * Update the rules for the currently highlighted element.
    */
   nodeChanged: function CssRuleView_nodeChanged()
   {
     // Ignore refreshes during editing or when no element is selected.
     if (this.isEditing || !this._elementStyle) {
-      return promise.resolve(null);
+      return;
     }
 
     this._clearRules();
 
     // Repopulate the element style.
-    return this._populate();
+    this._populate();
   },
 
   _populate: function() {
     let elementStyle = this._elementStyle;
     return this._elementStyle.populate().then(() => {
       if (this._elementStyle != elementStyle) {
         return promise.reject("element changed");
       }
--- a/browser/devtools/styleinspector/test/browser_ruleview_update.js
+++ b/browser/devtools/styleinspector/test/browser_ruleview_update.js
@@ -2,16 +2,17 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 let doc;
 
 let inspector;
 let ruleView;
 let testElement;
+let rule;
 
 function startTest(aInspector, aRuleView)
 {
   inspector = aInspector;
   ruleView = aRuleView;
   let style = '' +
     '#testid {' +
     '  background-color: blue;' +
@@ -24,123 +25,146 @@ function startTest(aInspector, aRuleView
   doc.body.innerHTML = '<div id="testid" class="testclass">Styled Node</div>';
 
   testElement = doc.getElementById("testid");
 
   let elementStyle = 'margin-top: 1px; padding-top: 5px;'
   testElement.setAttribute("style", elementStyle);
 
   inspector.selection.setNode(testElement);
-  inspector.once("inspector-updated", () => {
-    testRuleChanges();
-  }, true);
+  inspector.once("rule-view-refreshed", testRuleChanges);
 }
 
 function testRuleChanges()
 {
   let selectors = ruleView.doc.querySelectorAll(".ruleview-selector");
   is(selectors.length, 3, "Three rules visible.");
   is(selectors[0].textContent.indexOf("element"), 0, "First item is inline style.");
   is(selectors[1].textContent.indexOf("#testid"), 0, "Second item is id rule.");
   is(selectors[2].textContent.indexOf(".testclass"), 0, "Third item is class rule.");
 
   // Change the id and refresh.
+  inspector.once("rule-view-refreshed", testRuleChange1);
   testElement.setAttribute("id", "differentid");
-  promiseDone(ruleView.nodeChanged().then(() => {
-    let selectors = ruleView.doc.querySelectorAll(".ruleview-selector");
-    is(selectors.length, 2, "Two rules visible.");
-    is(selectors[0].textContent.indexOf("element"), 0, "First item is inline style.");
-    is(selectors[1].textContent.indexOf(".testclass"), 0, "Second item is class rule.");
+}
+
+function testRuleChange1()
+{
+  let selectors = ruleView.doc.querySelectorAll(".ruleview-selector");
+  is(selectors.length, 2, "Two rules visible.");
+  is(selectors[0].textContent.indexOf("element"), 0, "First item is inline style.");
+  is(selectors[1].textContent.indexOf(".testclass"), 0, "Second item is class rule.");
 
-    testElement.setAttribute("id", "testid");
-    return ruleView.nodeChanged();
-  }).then(() => {
-    // Put the id back.
-    let selectors = ruleView.doc.querySelectorAll(".ruleview-selector");
-    is(selectors.length, 3, "Three rules visible.");
-    is(selectors[0].textContent.indexOf("element"), 0, "First item is inline style.");
-    is(selectors[1].textContent.indexOf("#testid"), 0, "Second item is id rule.");
-    is(selectors[2].textContent.indexOf(".testclass"), 0, "Third item is class rule.");
+  inspector.once("rule-view-refreshed", testRuleChange2);
+  testElement.setAttribute("id", "testid");
+}
+function testRuleChange2()
+{
+  let selectors = ruleView.doc.querySelectorAll(".ruleview-selector");
+  is(selectors.length, 3, "Three rules visible.");
+  is(selectors[0].textContent.indexOf("element"), 0, "First item is inline style.");
+  is(selectors[1].textContent.indexOf("#testid"), 0, "Second item is id rule.");
+  is(selectors[2].textContent.indexOf(".testclass"), 0, "Third item is class rule.");
 
-    testPropertyChanges();
-  }));
+  testPropertyChanges();
 }
 
 function validateTextProp(aProp, aEnabled, aName, aValue, aDesc)
 {
   is(aProp.enabled, aEnabled, aDesc + ": enabled.");
   is(aProp.name, aName, aDesc + ": name.");
   is(aProp.value, aValue, aDesc + ": value.");
 
   is(aProp.editor.enable.hasAttribute("checked"), aEnabled, aDesc + ": enabled checkbox.");
   is(aProp.editor.nameSpan.textContent, aName, aDesc + ": name span.");
   is(aProp.editor.valueSpan.textContent, aValue, aDesc + ": value span.");
 }
 
 function testPropertyChanges()
 {
+  rule = ruleView._elementStyle.rules[0];
+  let ruleEditor = ruleView._elementStyle.rules[0].editor;
+  inspector.once("rule-view-refreshed", testPropertyChange0);
+
   // Add a second margin-top value, just to make things interesting.
-  let rule = ruleView._elementStyle.rules[0];
-  let ruleEditor = ruleView._elementStyle.rules[0].editor;
   ruleEditor.addProperty("margin-top", "5px", "");
-  promiseDone(expectRuleChange(rule).then(() => {
-    // Set the element style back to a 1px margin-top.
-    testElement.setAttribute("style", "margin-top: 1px; padding-top: 5px");
-    return ruleView.nodeChanged();
-  }).then(() => {
-    is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
-    validateTextProp(rule.textProps[0], true, "margin-top", "1px", "First margin property re-enabled");
-    validateTextProp(rule.textProps[2], false, "margin-top", "5px", "Second margin property disabled");
+}
+
+function testPropertyChange0()
+{
+  validateTextProp(rule.textProps[0], false, "margin-top", "1px", "Original margin property active");
 
-    // Now set it back to 5px, the 5px value should be re-enabled.
-    testElement.setAttribute("style", "margin-top: 5px; padding-top: 5px;");
-    return ruleView.nodeChanged();
+  inspector.once("rule-view-refreshed", testPropertyChange1);
+  testElement.setAttribute("style", "margin-top: 1px; padding-top: 5px");
+}
+function testPropertyChange1()
+{
+  is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
+  validateTextProp(rule.textProps[0], true, "margin-top", "1px", "First margin property re-enabled");
+  validateTextProp(rule.textProps[2], false, "margin-top", "5px", "Second margin property disabled");
+
+  inspector.once("rule-view-refreshed", testPropertyChange2);
 
-  }).then(() => {
-    is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
-    validateTextProp(rule.textProps[0], false, "margin-top", "1px", "First margin property re-enabled");
-    validateTextProp(rule.textProps[2], true, "margin-top", "5px", "Second margin property disabled");
+  // Now set it back to 5px, the 5px value should be re-enabled.
+  testElement.setAttribute("style", "margin-top: 5px; padding-top: 5px;");
+}
+function testPropertyChange2()
+{
+  is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
+  validateTextProp(rule.textProps[0], false, "margin-top", "1px", "First margin property re-enabled");
+  validateTextProp(rule.textProps[2], true, "margin-top", "5px", "Second margin property disabled");
+
+  inspector.once("rule-view-refreshed", testPropertyChange3);
 
-    // Set the margin property to a value that doesn't exist in the editor.
-    // Should reuse the currently-enabled element (the second one.)
-    testElement.setAttribute("style", "margin-top: 15px; padding-top: 5px;");
-    return ruleView.nodeChanged();
-  }).then(() => {
-    is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
-    validateTextProp(rule.textProps[0], false, "margin-top", "1px", "First margin property re-enabled");
-    validateTextProp(rule.textProps[2], true, "margin-top", "15px", "Second margin property disabled");
+  // Set the margin property to a value that doesn't exist in the editor.
+  // Should reuse the currently-enabled element (the second one.)
+  testElement.setAttribute("style", "margin-top: 15px; padding-top: 5px;");
+}
+function testPropertyChange3()
+{
+  is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
+  validateTextProp(rule.textProps[0], false, "margin-top", "1px", "First margin property re-enabled");
+  validateTextProp(rule.textProps[2], true, "margin-top", "15px", "Second margin property disabled");
 
-    // Remove the padding-top attribute.  Should disable the padding property but not remove it.
-    testElement.setAttribute("style", "margin-top: 5px;");
-    return ruleView.nodeChanged();
-  }).then(() => {
-    is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
-    validateTextProp(rule.textProps[1], false, "padding-top", "5px", "Padding property disabled");
+  inspector.once("rule-view-refreshed", testPropertyChange4);
+
+  // Remove the padding-top attribute.  Should disable the padding property but not remove it.
+  testElement.setAttribute("style", "margin-top: 5px;");
+}
+function testPropertyChange4()
+{
+  is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
+  validateTextProp(rule.textProps[1], false, "padding-top", "5px", "Padding property disabled");
 
-    // Put the padding-top attribute back in, should re-enable the padding property.
-    testElement.setAttribute("style", "margin-top: 5px; padding-top: 25px");
-    return ruleView.nodeChanged();
-  }).then(() => {
-    is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
-    validateTextProp(rule.textProps[1], true, "padding-top", "25px", "Padding property enabled");
+  inspector.once("rule-view-refreshed", testPropertyChange5);
+
+  // Put the padding-top attribute back in, should re-enable the padding property.
+  testElement.setAttribute("style", "margin-top: 5px; padding-top: 25px");
+}
+function testPropertyChange5()
+{
+  is(rule.editor.element.querySelectorAll(".ruleview-property").length, 3, "Correct number of properties");
+  validateTextProp(rule.textProps[1], true, "padding-top", "25px", "Padding property enabled");
 
-    // Add an entirely new property.
-    testElement.setAttribute("style", "margin-top: 5px; padding-top: 25px; padding-left: 20px;");
-    return ruleView.nodeChanged();
-  }).then(() => {
-    is(rule.editor.element.querySelectorAll(".ruleview-property").length, 4, "Added a property");
-    validateTextProp(rule.textProps[3], true, "padding-left", "20px", "Padding property enabled");
+  inspector.once("rule-view-refreshed", testPropertyChange6);
 
-    finishTest();
-  }));
+  // Add an entirely new property
+  testElement.setAttribute("style", "margin-top: 5px; padding-top: 25px; padding-left: 20px;");
+}
+function testPropertyChange6()
+{
+  is(rule.editor.element.querySelectorAll(".ruleview-property").length, 4, "Added a property");
+  validateTextProp(rule.textProps[3], true, "padding-left", "20px", "Padding property enabled");
+
+  finishTest();
 }
 
 function finishTest()
 {
-  inspector = ruleView = null;
+  inspector = ruleView = rule = null;
   doc = null;
   gBrowser.removeCurrentTab();
   finish();
 }
 
 function test()
 {
   waitForExplicitFinish();