From 35719bbbf7cdb54952e069aa9c7159764a24584e Mon Sep 17 00:00:00 2001 From: Brian Grinstead Date: Tue, 15 Oct 2013 07:09:49 -0500 Subject: [PATCH] Bug 911678 - Inspector - inline style rules do not populate the CSSRuleView immediately. r=miker --- browser/devtools/inspector/inspector-panel.js | 9 + .../test/browser_inspector_changes.js | 105 +++++++++--- browser/devtools/markupview/markup-view.js | 10 ++ browser/devtools/styleinspector/rule-view.js | 4 +- .../test/browser_ruleview_update.js | 154 ++++++++++-------- 5 files changed, 188 insertions(+), 94 deletions(-) diff --git a/browser/devtools/inspector/inspector-panel.js b/browser/devtools/inspector/inspector-panel.js index f16a9e0403b..0858d29397b 100644 --- a/browser/devtools/inspector/inspector-panel.js +++ b/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. diff --git a/browser/devtools/inspector/test/browser_inspector_changes.js b/browser/devtools/inspector/test/browser_inspector_changes.js index 703801f50a2..1338f9e5d08 100644 --- a/browser/devtools/inspector/test/browser_inspector_changes.js +++ b/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(); } diff --git a/browser/devtools/markupview/markup-view.js b/browser/devtools/markupview/markup-view.js index 84a508a2cdb..93201570125 100644 --- a/browser/devtools/markupview/markup-view.js +++ b/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"); diff --git a/browser/devtools/styleinspector/rule-view.js b/browser/devtools/styleinspector/rule-view.js index 1fb8e0bf1f7..8da6dcc0abe 100644 --- a/browser/devtools/styleinspector/rule-view.js +++ b/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() { diff --git a/browser/devtools/styleinspector/test/browser_ruleview_update.js b/browser/devtools/styleinspector/test/browser_ruleview_update.js index 92a3d557cba..30861fc2ee7 100644 --- a/browser/devtools/styleinspector/test/browser_ruleview_update.js +++ b/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();