Web Inspector: Elements Tab: Computed: variables section looks broken…

… when the selected element has no CSS variables

https://bugs.webkit.org/show_bug.cgi?id=242704

Reviewed by Devin Rousso.

The original implementation made some wrong assumptions about the layout lifecycle
for CSS variable groups. This patch introduces a refactoring to correct that and
fix a bunch issues at once.

Context:

The properties section of the Computed panel is updated in response to the event
`WI.CSSStyleDeclaration.Event.PropertiesChanged` fired by
`WI.DOMNodeStyles.computedStyle` which is a `WI.CSSDeclaration` instance.
This allows it to dynamically reflect changes to any styles of the selected node.

There's an optimization in `WI.DOMNodeStyles.refresh() -> fetchedComputedStyle()`
where a change to styles of existing CSS rules isn't considered a `significantChange`.
This prevents a lot of needless re-layout in components which observe style refreshes.

The Properties section of the Computed panel listens for the event on `WI.DOMNodeStyles.computedStyle`
to address this optimization.

When grouping CSS variables, the groups were distinct `WI.CSSDeclaration` instances for clarity.
But they were cached and never updated in response to style changes on a node.
Because of this, the sections for variable groups in the Computed panel did not reflect
any changes: adding/removing variables, changing their type, etc.

The changes:

1) The logic for grouping variables by type is moved from `WI.DOMNodeStyles` to
`WI.CSSDeclaration`. This way, all sections in the Computed panel rely on a single
data source: `WI.DOMNodeStyles.computedStyle`.
It's the view's job to pick the subset of properties to render.

2) A new getter is added, `WI.CSSDeclaration.variablesForType`, to do this extra work
for all variable groups when consumers ask for them.

There can be a growing number of groups, so having separate iterations on every style
change over a potentially long list of properties is wasteful.
Plus, anything that doesn't match a type checker needs to go in the "Other" group.

3) `WI.ComputedStyleSection` is modified to obtain a list of properties to render
depending on the variables group type set on `WI.ComputedStyleSection.variablesGroupType`.
This subset of properties is then used in `WI.ComputedStyleSection.layout()`.

Introduced debouncing for layout in respose to `WI.DOMNodeStyles.Events.PropertiesChanged`
because it is expensive; throwing away all elements for properties and rebuilding from scratch.

4) Filtering is implemented. Sections that have no matches are hidden.

5) The test that checks CSS variables grouping is renamed and updated.

6) Modify `WI.View` to call `WI.View.didSubtreeLayout()` on the immediate parent
of the first dirty view that did layout on itself and all of its descendant views.
This allows a parent view to react in response to layout under its subtree.

This behavior is used to call `WI.ComputedStyleDetailsPanel.didSubtreeLayout()` where we can
toggle the visibility of the "Variables" section when the nested sections for variable groups are empty.

* LayoutTests/inspector/css/variablesForType-expected.txt: Renamed from LayoutTests/inspector/css/variableStylesByType-expected.txt.
* LayoutTests/inspector/css/variablesForType.html: Renamed from LayoutTests/inspector/css/variableStylesByType.html.
* LayoutTests/inspector/view/asynchronous-layout-expected.txt:
* LayoutTests/inspector/view/asynchronous-layout.html:
* LayoutTests/inspector/view/resources/test-view.js:
(TestPage.registerInitializer.WI.TestView):
(TestPage.registerInitializer.WI.TestView.prototype.get didLayoutSubtreeCount):
(TestPage.registerInitializer.WI.TestView.prototype.didLayoutSubtree):
(TestPage.registerInitializer):
* LayoutTests/inspector/view/synchronous-layout-expected.txt:
* LayoutTests/inspector/view/synchronous-layout.html:
* Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:
(WI.CSSStyleDeclaration):
(WI.CSSStyleDeclaration.prototype.variablesForType):
* Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:
(WI.DOMNodeStyles):
(WI.DOMNodeStyles.prototype.refresh.fetchedComputedStyle):
(WI.DOMNodeStyles.prototype.get variableStylesByType): Deleted.
* Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:
(WI.ComputedStyleDetailsPanel.displayNameForVariablesGroupType):
(WI.ComputedStyleDetailsPanel.prototype.refresh):
(WI.ComputedStyleDetailsPanel.prototype.layout):
(WI.ComputedStyleDetailsPanel.prototype.didLayoutSubtree):
(WI.ComputedStyleDetailsPanel.prototype._createVariablesStyleSection):
(WI.ComputedStyleDetailsPanel.prototype._removeVariablesStyleSection):
(WI.ComputedStyleDetailsPanel.prototype._renderVariablesStyleSectionGroup): Deleted.
* Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:
(WI.ComputedStyleSection.prototype.get variablesGroupType):
(WI.ComputedStyleSection.prototype.get isEmpty):
(WI.ComputedStyleSection.prototype.get properties):
(WI.ComputedStyleSection.prototype.layout):
(WI.ComputedStyleSection.prototype.detached):
(WI.ComputedStyleSection.prototype.applyFilter):
(WI.ComputedStyleSection.prototype._handlePropertiesChanged):
(WI.ComputedStyleSection):
* Source/WebInspectorUI/UserInterface/Views/View.js:
(WI.View.prototype.updateLayout):
(WI.View.prototype._layoutSubtree):
(WI.View._visitViewTreeForLayout):
(WI.View):

Canonical link: https://commits.webkit.org/253562@main

LayoutTests/inspector/css/variableStylesByType-expected.txt → LayoutTests/inspector/css/variablesForType-expected.txt

@@ -1,24 +1,24 @@
-Test for DOMNodeStyles.variableStylesByType
+Test for CSSStyleDeclaration.variableStylesByType
 
 
-== Running test suite: DOMNodeStyles.variableStylesByType
--- Running test case: DOMNodeStyles.variableStylesByType.Colors
+== Running test suite: CSSStyleDeclaration.variableStylesByType
+-- Running test case: CSSStyleDeclaration.variableStylesByType.Colors
 PASS: Should have "colors" variables group.
 PASS: Group should include all expected variables.
 
--- Running test case: DOMNodeStyles.variableStylesByType.Dimensions
+-- Running test case: CSSStyleDeclaration.variableStylesByType.Dimensions
 PASS: Should have "dimensions" variables group.
 PASS: Group should include all expected variables.
 
--- Running test case: DOMNodeStyles.variableStylesByType.Numbers
+-- Running test case: CSSStyleDeclaration.variableStylesByType.Numbers
 PASS: Should have "numbers" variables group.
 PASS: Group should include all expected variables.
 
--- Running test case: DOMNodeStyles.variableStylesByType.Other
+-- Running test case: CSSStyleDeclaration.variableStylesByType.Other
 PASS: Should have "other" variables group.
 PASS: Group should include all expected variables.
 
--- Running test case: DOMNodeStyles.variableStylesByType.Mixed
+-- Running test case: CSSStyleDeclaration.variableStylesByType.Mixed
 PASS: Should have "colors" variables group.
 PASS: Group should include all expected variables.
 PASS: Should have "dimensions" variables group.

LayoutTests/inspector/css/variableStylesByType.html → LayoutTests/inspector/css/variablesForType.html

@@ -6,7 +6,7 @@
 
 function test()
 {
-    let suite = InspectorTest.createAsyncSuite("DOMNodeStyles.variableStylesByType");
+    let suite = InspectorTest.createAsyncSuite("CSSStyleDeclaration.variableStylesByType");
 
     function addTestCase({name, description, selector, expected})
     {
@@ -19,86 +19,87 @@
                 let domNode = await WI.domManager.nodeForId(nodeId);
                 InspectorTest.assert(domNode, `Should find DOM Node for selector '${selector}'.`);
 
-                let cssStyles = WI.cssManager.stylesForNode(domNode);
-                await cssStyles.refreshIfNeeded();
+                let domNodeStyles = WI.cssManager.stylesForNode(domNode);
+                await domNodeStyles.refreshIfNeeded();
+
+                domNodeStyles.computedStyle.useVariablesGrouping = true;
 
-                let styleGroups = cssStyles.variableStylesByType;
                 for (let {groupType, variables} of expected) {
-                    let propertyNames = styleGroups.get(groupType).properties.map(property => property.name);
-                    InspectorTest.expectTrue(styleGroups.has(groupType), `Should have "${groupType}" variables group.`);
+                    let propertyNames = domNodeStyles.computedStyle.variablesForType(groupType).map(property => property.name);
+                    InspectorTest.expectTrue(!!propertyNames.length, `Should have "${groupType}" variables group.`);
                     InspectorTest.expectShallowEqual(propertyNames.sort(), variables.sort(), "Group should include all expected variables.");
                 }
             },
         });
     }
 
     addTestCase({
-        name: "DOMNodeStyles.variableStylesByType.Colors",
+        name: "CSSStyleDeclaration.variableStylesByType.Colors",
         description: "Check that variables with color values are correctly identified.",
         selector: "#colors",
         expected: [
             {
-                groupType: WI.DOMNodeStyles.VariablesGroupType.Colors,
+                groupType: WI.CSSStyleDeclaration.VariablesGroupType.Colors,
                 variables: ["--colorName", "--colorHEX", "--colorRGB", "--colorRGBA", "--colorHSL", "--colorHSLA"],
             },
         ],
     });
 
     addTestCase({
-        name: "DOMNodeStyles.variableStylesByType.Dimensions",
+        name: "CSSStyleDeclaration.variableStylesByType.Dimensions",
         description: "Check that variables with dimension values are correctly identified.",
         selector: "#dimensions",
         expected: [
             {
-                groupType: WI.DOMNodeStyles.VariablesGroupType.Dimensions,
+                groupType: WI.CSSStyleDeclaration.VariablesGroupType.Dimensions,
                 variables: ["--dimensionPX", "--dimensionPercent", "--dimensionFloat", "--dimensionNegative", "--dimensionNegativeFloat"],
             },
         ],
     });
 
     addTestCase({
-        name: "DOMNodeStyles.variableStylesByType.Numbers",
+        name: "CSSStyleDeclaration.variableStylesByType.Numbers",
         description: "Check that variables with number values are correctly identified.",
         selector: "#numbers",
         expected: [
             {
-                groupType: WI.DOMNodeStyles.VariablesGroupType.Numbers,
+                groupType: WI.CSSStyleDeclaration.VariablesGroupType.Numbers,
                 variables: ["--number", "--numberFloat", "--numberNegative", "--numberNegativeFloat"],
             },
         ],
     });
 
     addTestCase({
-        name: "DOMNodeStyles.variableStylesByType.Other",
+        name: "CSSStyleDeclaration.variableStylesByType.Other",
         description: "Check that variables with values with unmatched types are placed in the fallback group.",
         selector: "#other",
         expected: [
             {
-                groupType: WI.DOMNodeStyles.VariablesGroupType.Other,
+                groupType: WI.CSSStyleDeclaration.VariablesGroupType.Other,
                 variables: ["--shadow", "--font-family", "--font-shorthand", "--keyword"],
             },
         ],
     });
 
     addTestCase({
-        name: "DOMNodeStyles.variableStylesByType.Mixed",
+        name: "CSSStyleDeclaration.variableStylesByType.Mixed",
         description: "Check that all type groups have variables allocated.",
         selector: "#mixed",
         expected: [
             {
-                groupType: WI.DOMNodeStyles.VariablesGroupType.Colors,
+                groupType: WI.CSSStyleDeclaration.VariablesGroupType.Colors,
                 variables: ["--color"],
             },
             {
-                groupType: WI.DOMNodeStyles.VariablesGroupType.Dimensions,
+                groupType: WI.CSSStyleDeclaration.VariablesGroupType.Dimensions,
                 variables: ["--dimension"],
             },
             {
-                groupType: WI.DOMNodeStyles.VariablesGroupType.Numbers,
+                groupType: WI.CSSStyleDeclaration.VariablesGroupType.Numbers,
                 variables: ["--number"],
             },
             {
-                groupType: WI.DOMNodeStyles.VariablesGroupType.Other,
+                groupType: WI.CSSStyleDeclaration.VariablesGroupType.Other,
                 variables: ["--font-family"],
             },
         ],
@@ -169,7 +170,7 @@
 </style>
 </head>
 <body onload="runTest();">
-<p>Test for DOMNodeStyles.variableStylesByType</p>
+<p>Test for CSSStyleDeclaration.variableStylesByType</p>
 <div>
     <div id="colors"></div>
     <div id="dimensions"></div>

LayoutTests/inspector/view/asynchronous-layout-expected.txt

@@ -31,6 +31,12 @@ PASS: Parent view should have 0 dirty descendants.
 PASS: Child view should have 0 dirty descendants.
 PASS: Parent view should have started a layout.
 PASS: Child view should have completed 1 layout.
+Parent view completed a layout.
+PASS: Root view should have 0 dirty descendants.
+PASS: Root view should have 0 dirty descendants.
+PASS: Root view should have 0 dirty descendants.
+PASS: Parent view should have completed 1 layout.
+PASS: Parent view should not have a pending layout.
 Child view completed a layout.
 PASS: Root view should have 0 dirty descendants.
 PASS: Parent view should have 0 dirty descendants.
@@ -50,3 +56,12 @@ Layout complete.
 PASS: Chlid view should do an initial layout.
 PASS: Child view should update its layout.
 
+-- Running test case: View.didLayoutSubtree.parentView
+Flush pending layout on parent view.
+Schedule child view update.
+Layout complete.
+PASS: Child view should update its layout twice.
+PASS: Parent view should update its layout once.
+PASS: Child view should call didLayoutSubtree twice.
+PASS: Parent view should call didLayoutSubtree twice.
+

LayoutTests/inspector/view/asynchronous-layout.html

@@ -115,7 +115,18 @@
                 InspectorTest.expectEqual(parentView.layoutCount, 1, "Parent view should have completed 1 layout.");
                 InspectorTest.expectFalse(parentView.layoutPending, "Parent view should not have a pending layout.");
 
-                resolve();
+                parentView.evaluateAfterLayout(() => {
+                    InspectorTest.log("Parent view completed a layout.");
+
+                    InspectorTest.expectEqual(rootView._dirtyDescendantsCount, 0, "Root view should have 0 dirty descendants.");
+                    InspectorTest.expectEqual(parentView._dirtyDescendantsCount, 0, "Root view should have 0 dirty descendants.");
+                    InspectorTest.expectEqual(childView._dirtyDescendantsCount, 0, "Root view should have 0 dirty descendants.");
+
+                    InspectorTest.expectEqual(parentView.layoutCount, 1, "Parent view should have completed 1 layout.");
+                    InspectorTest.expectFalse(parentView.layoutPending, "Parent view should not have a pending layout.");
+
+                    resolve();
+                });
             });
         }
     });
@@ -139,6 +150,31 @@
         }
     });
 
+    suite.addTestCase({
+        name: "View.didLayoutSubtree.parentView",
+        test(resolve, reject) {
+            let parent = new WI.TestView;
+            let child = new WI.TestView;
+            WI.View.rootView().addSubview(parent);
+            parent.addSubview(child);
+
+            InspectorTest.log("Flush pending layout on parent view.");
+            parent.updateLayout();
+
+            InspectorTest.log("Schedule child view update.");
+            child.needsLayout();
+
+            parent.evaluateAfterLayout(() => {
+                InspectorTest.log("Layout complete.");
+                InspectorTest.expectEqual(child.layoutCount, 2, "Child view should update its layout twice.");
+                InspectorTest.expectEqual(parent.layoutCount, 1, "Parent view should update its layout once.");
+                InspectorTest.expectEqual(child.didLayoutSubtreeCount, 2, "Child view should call didLayoutSubtree twice.");
+                InspectorTest.expectEqual(parent.didLayoutSubtreeCount, 2, "Parent view should call didLayoutSubtree twice.");
+                resolve();
+            });
+        }
+    });
+
     suite.runTestCasesAndFinish();
 }
 </script>

LayoutTests/inspector/view/resources/test-view.js

@@ -8,12 +8,14 @@ TestPage.registerInitializer(() => {
             this._layoutCallbacks = [];
             this._initialLayoutCount = 0;
             this._layoutCount = 0;
+            this._didLayoutSubtreeCount = 0;
         }
 
         // Public
 
         get initialLayoutCount() { return this._initialLayoutCount; }
         get layoutCount() { return this._layoutCount; }
+        get didLayoutSubtreeCount() { return this._didLayoutSubtreeCount; }
 
         evaluateAfterLayout(callback)
         {
@@ -34,6 +36,8 @@ TestPage.registerInitializer(() => {
 
         didLayoutSubtree()
         {
+            this._didLayoutSubtreeCount++;
+
             let callbacks = this._layoutCallbacks;
             this._layoutCallbacks = [];
             for (let callback of callbacks)

LayoutTests/inspector/view/synchronous-layout-expected.txt

@@ -22,3 +22,10 @@ PASS: View should update if an initial layout never happened.
 PASS: View should update if a layout is pending.
 PASS: View should not update if no layout is pending.
 
+-- Running test case: View.didLayoutSubtree.parentView
+Update child view layout.
+PASS: Child view should update its layout once.
+PASS: Parent view should not update its layout.
+PASS: Child view should call didLayoutSubtree once.
+PASS: Parent view should call didLayoutSubtree once.
+

LayoutTests/inspector/view/synchronous-layout.html

@@ -71,6 +71,25 @@
         }
     });
 
+    suite.addTestCase({
+        name: "View.didLayoutSubtree.parentView",
+        test() {
+            let parent = new WI.TestView;
+            let child = new WI.TestView;
+            parent.addSubview(child);
+
+            InspectorTest.log("Update child view layout.");
+            child.updateLayout();
+
+            InspectorTest.expectEqual(child.layoutCount, 1, "Child view should update its layout once.");
+            InspectorTest.expectEqual(parent.layoutCount, 0, "Parent view should not update its layout.");
+            InspectorTest.expectEqual(child.didLayoutSubtreeCount, 1, "Child view should call didLayoutSubtree once.");
+            InspectorTest.expectEqual(parent.didLayoutSubtreeCount, 1, "Parent view should call didLayoutSubtree once.");
+
+            return true;
+        }
+    });
+
     suite.runTestCasesAndFinish();
 }
 </script>

Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js

@@ -50,6 +50,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
         this._properties = [];
         this._enabledProperties = null;
         this._visibleProperties = null;
+        this._variablesForType = new Map;
 
         this.update(text, properties, styleSheetTextRange, {dontFireEvents: true});
     }
@@ -113,6 +114,54 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
     get locked() { return this._locked; }
     set locked(value) { this._locked = value; }
 
+    variablesForType(type)
+    {
+        console.assert(Object.values(WI.CSSStyleDeclaration.VariablesGroupType).includes(type), type);
+
+        let variables = this._variablesForType.get(type);
+        if (variables)
+            return variables;
+
+        // Will iterate in order through type checkers for each CSS variable to identify its type.
+        // The catch-all "other" must always be last.
+        const typeCheckFunctions = [
+            {
+                type: WI.CSSStyleDeclaration.VariablesGroupType.Colors,
+                checker: (property) => WI.Color.fromString(property.value),
+            },
+            {
+                type: WI.CSSStyleDeclaration.VariablesGroupType.Dimensions,
+                checker: (property) => /^-?\d+(\.\d+)?\D+$/.test(property.value),
+            },
+            {
+                type: WI.CSSStyleDeclaration.VariablesGroupType.Numbers,
+                checker: (property) => /^-?\d+(\.\d+)?$/.test(property.value),
+            },
+            {
+                type: WI.CSSStyleDeclaration.VariablesGroupType.Other,
+                checker: (property) => true,
+            },
+        ];
+
+        // Ensure all types have a list. Empty lists are a signal to views to skip rendering.
+        for (let {type} of typeCheckFunctions)
+            this._variablesForType.set(type, []);
+
+        for (let property of this._properties) {
+            if (!property.isVariable)
+                continue;
+
+            for (let {type, checker} of typeCheckFunctions) {
+                if (checker(property)) {
+                    this._variablesForType.get(type).push(property);
+                    break;
+                }
+            }
+        }
+
+        return this._variablesForType.get(type);
+    }
+
     update(text, properties, styleSheetTextRange, options = {})
     {
         let dontFireEvents = options.dontFireEvents || false;
@@ -145,6 +194,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
 
         this._styleSheetTextRange = styleSheetTextRange;
         this._propertyNameMap = {};
+        this._variablesForType.clear();
 
         this._enabledProperties = null;
         this._visibleProperties = null;
@@ -565,3 +615,11 @@ WI.CSSStyleDeclaration.Type = {
     Attribute: "css-style-declaration-type-attribute",
     Computed: "css-style-declaration-type-computed"
 };
+
+WI.CSSStyleDeclaration.VariablesGroupType = {
+    Ungrouped: "ungrouped",
+    Colors: "colors",
+    Dimensions: "dimensions",
+    Numbers: "numbers",
+    Other: "other",
+};