Bug 911678 - Inspector - inline style rules do not populate the CSSRuleView immediately. r=miker

browser/devtools/inspector/inspector-panel.js

@@ -743,6 +743,15 @@ InspectorPanel.prototype = {
     }
   },
 
+  /**
+  * 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.

browser/devtools/inspector/test/browser_inspector_changes.js

@@ -17,7 +17,7 @@ function test() {
   }
 
 
-  function getInspectorProp(aName)
+  function getInspectorComputedProp(aName)
   {
     let computedview = inspector.sidebar.getWindowForTab("computedview").computedview.view;
     for each (let view in computedview.propertyViews) {
@@ -28,6 +28,19 @@ function test() {
     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;
@@ -40,59 +53,97 @@ function test() {
       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.cssText = "font-size: 15px; color: red;";
 
-    testDiv.style.fontSize = "15px";
-
-    // 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";
 
-      // 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");
-    });
+    inspector.once("computed-view-refreshed", computedStylePanelAfterSwitch);
+    inspector.sidebar.select("computedview");
   }
 
-  function stylePanelAfterSwitch()
+  function computedStylePanelAfterSwitch()
   {
-    let propView = getInspectorProp("font-size");
-    is(propView.value, "20px", "Style inspector should be showing the newest font size.");
+    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 rulePanelTests()
+  {
+    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();
   }

browser/devtools/markupview/markup-view.js

@@ -387,6 +387,7 @@ MarkupView.prototype = {
    * 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;
@@ -409,6 +410,11 @@ MarkupView.prototype = {
       }
       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
@@ -417,6 +423,10 @@ MarkupView.prototype = {
         this._updateChildren(container, {flash: true});
       }
     }
+
+    if (requiresLayoutChange) {
+      this._inspector.immediateLayoutChange();
+    }
     this._waitForChildren().then(() => {
       this._flashMutatedNodes(aMutations);
       this._inspector.emit("markupmutation");

browser/devtools/styleinspector/rule-view.js

@@ -1283,13 +1283,13 @@ CssRuleView.prototype = {
   {
     // 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() {

browser/devtools/styleinspector/test/browser_ruleview_update.js

@@ -7,6 +7,7 @@ let doc;
 let inspector;
 let ruleView;
 let testElement;
+let rule;
 
 function startTest(aInspector, aRuleView)
 {
@@ -29,9 +30,7 @@ function startTest(aInspector, aRuleView)
   testElement.setAttribute("style", elementStyle);
 
   inspector.selection.setNode(testElement);
-  inspector.once("inspector-updated", () => {
-    testRuleChanges();
-  }, true);
+  inspector.once("rule-view-refreshed", testRuleChanges);
 }
 
 function testRuleChanges()
@@ -43,25 +42,29 @@ function testRuleChanges()
   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.");
+}
 
-    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.");
+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.");
 
-    testPropertyChanges();
-  }));
+  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();
 }
 
 function validateTextProp(aProp, aEnabled, aName, aValue, aDesc)
@@ -77,65 +80,86 @@ function validateTextProp(aProp, aEnabled, aName, aValue, aDesc)
 
 function testPropertyChanges()
 {
-  // Add a second margin-top value, just to make things interesting.
-  let rule = ruleView._elementStyle.rules[0];
+  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.
   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");
+}
 
-    // 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();
+function testPropertyChange0()
+{
+  validateTextProp(rule.textProps[0], false, "margin-top", "1px", "Original margin property active");
 
-  }).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");
+  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");
 
-    // 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");
+  inspector.once("rule-view-refreshed", testPropertyChange2);
 
-    // 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");
+  // 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");
 
-    // 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", testPropertyChange3);
 
-    // 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");
+  // 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");
 
-    finishTest();
-  }));
+  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");
+
+  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");
+
+  inspector.once("rule-view-refreshed", testPropertyChange6);
+
+  // 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();