Bug 225972 – Web Inspector: Styles panel slow to render when inspecting node with many inherited CSS variables

• Component: Web Inspector
• Status: RESOLVED FIXED

[Razvan Caliman]

Steps to reproduce:
- Go to https://www.youtube.com
- Open Web Inspector > Elements Tab
- Inspect any node

Observe the substantial delay between selecting the node and the Styles details sidebar updating.

Scroll the list of matched styles and notice a large number of CSS variables inherited from ancestors, most of which are not used on the selected node.

Sites like these whose design system places most CSS variables on the root node causes all nodes on the page to inherit them. Inspecting any node forces Web Inspector to do a lot of work to render declarations of inherited CSS variables which do not participate in the styles of the selected node. This adds visual noise and impacts performance by doing a lot of unnecessary work, like trying to annotate variable values with color swatches.

Unused inherited variables should not be shown in the Styles details sidebar to reduce visual clutter and focus the user's attention when debugging styles.

The list of all matching CSS variables can always be found in the Computed details sidebar, in the Variables section.

[Radar WebKit Bug Importer]

<rdar://problem/78211185>

[Razvan Caliman]

Created attachment 429067 [details]
Patch

[Razvan Caliman]

Created attachment 429069 [details]
Video with patch applied

[Razvan Caliman]

Created attachment 429070 [details]
Screenshot with patch applied

[Razvan Caliman]

Created attachment 429072 [details]
Sample file: unused inherited CSS variables

Inspect .target: None of the inherited CSS Variables should be visible

[Razvan Caliman]

Created attachment 429073 [details]
Sample file: referenced inherited CSS variables

Inspect .target: All inherited CSS Variables should be visible

[Razvan Caliman]

Created attachment 429074 [details]
Sample file: overwritten inherited CSS variables

Inspect .target: All inherited CSS Variables should be visible

[Razvan Caliman]

Created attachment 429075 [details]
Sample file: inherited CSS variables in inheritable properties

Inspect .target: Only inherited variables of inheritable properties should be visible

[Razvan Caliman]

Created attachment 429076 [details]
Screenshot of inspecting youtube.com with patch applied

Hundreds of unused CSS variables now hidden.
Easier to scan the list of matching styles.

[Patrick Angle]

Comment on attachment 429067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429067&action=review

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1031
> +localizedStrings["Option-click to show unused CSS variables from all rules"] = "Option-click to show unused CSS variables from all rules";

We generally format new strings that are used in specific places with keys of `Text @ Location` e.g. `Option-click to show unused CSS variables from all rules @ Styles Sidebar Panel Tooltip` and use the three-argument WI.UIString format and include a description for localization as well.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1324
> +localizedStrings["Show %d unused CSS variable (singular)"] = "Show %d unused CSS variable";
> +localizedStrings["Show %d unused CSS variables (plural)"] = "Show %d unused CSS variables";

Ditto :1031

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:52
> +    static getUsedVariableNames(string)

We should write some tests for this utility method, or make sure it is being exercised in tests for `DOMNodeStyles::_collectUsedCSSVariables`.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:948
> +    _collectUsedCSSVariables(styles)

We should also test the resulting set here. While it won't test the UI, it can make sure that the information the UI uses matches our expectations. See `getComputedPrimaryFontForNode.html` for a test I added last fall to test another new property on `DOMNodeStyles`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:337
> +            this.updateLayout();

Does this have to be done synchronously? In general, it's better to schedule a layout with `needsLayout(WI.View.LayoutReason.Dirty)` so we can batch any other layouts together.

[Nikita Vasilyev]

Comment on attachment 429067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429067&action=review

(Not a complete review.)

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:61
> +            return char === ")" || char === "," || char === " ";

"\n" and "\t"?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:969
> +                    // Leverage the fact that styles are already sorted in cascade order to support inherited variables referencing other variables.
> +                    // If the variable was used before (ex: in a CSS rule of higher specificity), collect any variables used in its declaration value
> +                    // (if any variables are found, this isn't the end of the variable reference chain in the inheritance stack).

Neat!

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:727
> +        this?._delegate?.spreadsheetCSSStyleDeclarationEditorSetAllPropertyVisibilityMode(propertyVisibilityMode);

Nit: `this` cannot be falsy here. Should be `this._delegate?.` instead of `this?._delegate?.`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:290
> +        if (this._delegate && this._delegate.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode)
> +            this._delegate.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode(propertyVisibilityMode);

Nit: could be shortened to `this._delegate?.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode?.(propertyVisibilityMode)`

[Devin Rousso]

Comment on attachment 429067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429067&action=review

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:52
>> +    static getUsedVariableNames(string)
> 
> We should write some tests for this utility method, or make sure it is being exercised in tests for `DOMNodeStyles::_collectUsedCSSVariables`.

I also wonder if this might be easier done somewhere else, like when tokenizing the CSS property as right now this won't handle cases like `content: "var(--test)"`.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:953
> +        for (var i = 0; i < styles.length; ++i) {
> +            var style = styles[i];

```
    for (let style of styles) {
```

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:957
> +            for (var j = 0; j < properties.length; ++j) {
> +                var property = properties[j];

```
    for (let property of style.enabledProperties) {
```

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:962
> +                else {

NIT: Instead of an `else` can you just add a `continue` to the `if` above?

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:969
>> +                    // (if any variables are found, this isn't the end of the variable reference chain in the inheritance stack).
> 
> Neat!

This isn't necessarily true.  CSS `var` supports a fallback value if the variable isn't defined, so there's no guarantee that the variable usage is higher specificity than it's declaration.  e.g.
```
    div#higher-specificity {
        --color: red;
    }

    div {
        color: var(--color, blue);
    }
```

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:239
> +    margin-block-start: 4px;

NIT: there's really no reason to use `-block-` properties since Web Inspector is always in a horizontal writing mode

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:245
> +    color: hsl(0, 0%, 50%);

Can we use a variable instead?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:249
> +    text-decoration: none;

This seems like the wrong behavior.  I think we should match what we do with the source code location link and adjust the color.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:250
> +    color: hsl(0, 0%, 33%);

ditto (:245)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:256
> +    filter: opacity(0.5);

Why not just `opacity: 0.5`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:260
> +    vertical-align: middle;

I don't think this is actually vertically aligned :/

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:108
> +        if (this._hiddenUnusedVariables.size > 0) {

Style: drop the `> 0` since `size` can never be negative

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:112
> +            showHiddenVariablesButtonElement.role = "button";

NIT: Why don't we use an actual `<button>` instead?  That's normally what we do for "Show # More" things (e.g. if an array is more than 100 items long in the console).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:116
> +            let label = this._hiddenUnusedVariables.size > 1
> +                ? WI.UIString("Show %d unused CSS variables", "Show %d unused CSS variables (plural)", "")
> +                : WI.UIString("Show %d unused CSS variable", "Show %d unused CSS variable (singular)", "");

Style: we don't usually do this kind of multi-line ternary

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125
> +                this.propertyVisibilityMode = WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode.ShowAll;

What happens if a developer enables `ShowAll`, selects another element to hide the section, and then re-selects the first element to show this section again?  Does `ShowAll` persist or does it get reset each time the section is shown?  IMO it should be the former.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:128
> +            return;

Do we really want to `return` here?  What about the code below for filtering or editing or selection?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:272
> +            if (property.isVariable && hideUnusedInheritedVariables && this._style.inherited && !this._style.nodeStyles.usedCSSVariables.has(property.name)) {

NIT: I feel like we should restructure this function a bit so that we don't check `property.isVariable` three times

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:337
>> +            this.updateLayout();
> 
> Does this have to be done synchronously? In general, it's better to schedule a layout with `needsLayout(WI.View.LayoutReason.Dirty)` so we can batch any other layouts together.

(or really just `needsLayout()` as `WI.View.LayoutReason.Dirty` is the default)

Also, `set propertyVisibilityMode` will call `needsLayout` so you might not even need to do anything.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:338
> +            return this.highlightProperty(property);

If the reason that you're doing `updateLayout` is so that this can work, the pattern we'd prefer instead is to have a `this._pendingPropertyToHighlight = property;` and then use it (and reset it) inside `layout`.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:727
>> +        this?._delegate?.spreadsheetCSSStyleDeclarationEditorSetAllPropertyVisibilityMode(propertyVisibilityMode);
> 
> Nit: `this` cannot be falsy here. Should be `this._delegate?.` instead of `this?._delegate?.`.

+1

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:290
>> +            this._delegate.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode(propertyVisibilityMode);
> 
> Nit: could be shortened to `this._delegate?.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode?.(propertyVisibilityMode)`

+1

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:293
> +    setPropertyVisibilityMode(propertyVisibilityMode)

NIT: Why not just `set propertyVisibilityMode`?

[Razvan Caliman]

Comment on attachment 429067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429067&action=review

>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:52
>>> +    static getUsedVariableNames(string)
>> 
>> We should write some tests for this utility method, or make sure it is being exercised in tests for `DOMNodeStyles::_collectUsedCSSVariables`.
> 
> I also wonder if this might be easier done somewhere else, like when tokenizing the CSS property as right now this won't handle cases like `content: "var(--test)"`.

Renamed to getFoundVariableNames() and added a test.

@Devin, this called by `WI.DOMNodeStyles._collectUsedCSSVariables()` with CSS property values, so it does pick up `--test` in that example. It picks-up variable-like names. It's naive, but it works reasonably well for this use case: creating an allow-list of variable names found in values against which to check actual CSS variable names from CSS property declarations.

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:61
>> +            return char === ")" || char === "," || char === " ";
> 
> "\n" and "\t"?

Good catch!

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:112
>> +            showHiddenVariablesButtonElement.role = "button";
> 
> NIT: Why don't we use an actual `<button>` instead?  That's normally what we do for "Show # More" things (e.g. if an array is more than 100 items long in the console).

I avoided the plain button because the repetition looked noisy on egregious use case with many rules with unused variables. 
I tried it again with the latest patch. Let me know how it feels. 
I'm warming up to the idea, particularly if there's precedent in the UI.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125
>> +                this.propertyVisibilityMode = WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode.ShowAll;
> 
> What happens if a developer enables `ShowAll`, selects another element to hide the section, and then re-selects the first element to show this section again?  Does `ShowAll` persist or does it get reset each time the section is shown?  IMO it should be the former.

Yes, the shown properties persist when returning to the element.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:128
>> +            return;
> 
> Do we really want to `return` here?  What about the code below for filtering or editing or selection?

Oops. Leftover from other code.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:338
>> +            return this.highlightProperty(property);
> 
> If the reason that you're doing `updateLayout` is so that this can work, the pattern we'd prefer instead is to have a `this._pendingPropertyToHighlight = property;` and then use it (and reset it) inside `layout`.

That's clever and it works. Thanks!
I was doing synchronous layout before because without this, it entered into an infinite refresh loop.

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:727
>>> +        this?._delegate?.spreadsheetCSSStyleDeclarationEditorSetAllPropertyVisibilityMode(propertyVisibilityMode);
>> 
>> Nit: `this` cannot be falsy here. Should be `this._delegate?.` instead of `this?._delegate?.`.
> 
> +1

Done!

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:290
>>> +            this._delegate.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode(propertyVisibilityMode);
>> 
>> Nit: could be shortened to `this._delegate?.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode?.(propertyVisibilityMode)`
> 
> +1

Done!

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:293
>> +    setPropertyVisibilityMode(propertyVisibilityMode)
> 
> NIT: Why not just `set propertyVisibilityMode`?

Yes, that works too.

[Razvan Caliman]

Created attachment 430277 [details]
Patch

Add tests. Address code review feedback

[Devin Rousso]

Comment on attachment 429067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429067&action=review

>>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:52
>>>> +    static getUsedVariableNames(string)
>>> 
>>> We should write some tests for this utility method, or make sure it is being exercised in tests for `DOMNodeStyles::_collectUsedCSSVariables`.
>> 
>> I also wonder if this might be easier done somewhere else, like when tokenizing the CSS property as right now this won't handle cases like `content: "var(--test)"`.
> 
> Renamed to getFoundVariableNames() and added a test.
> 
> @Devin, this called by `WI.DOMNodeStyles._collectUsedCSSVariables()` with CSS property values, so it does pick up `--test` in that example. It picks-up variable-like names. It's naive, but it works reasonably well for this use case: creating an allow-list of variable names found in values against which to check actual CSS variable names from CSS property declarations.

Right my point was more that we do NOT want it to pick up `--` inside strings.  `content: "var(--test)"` shouldn't be considered as a valid usage of the CSS variable `--test`.  Perhaps not the biggest issue since it's not really a bad result (and probably exceedingly rare), but definitely worth a followup bug and maybe even a FIXME comment.

>>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:969
>>> +                    // (if any variables are found, this isn't the end of the variable reference chain in the inheritance stack).
>> 
>> Neat!
> 
> This isn't necessarily true.  CSS `var` supports a fallback value if the variable isn't defined, so there's no guarantee that the variable usage is higher specificity than it's declaration.  e.g.
> ```
>     div#higher-specificity {
>         --color: red;
>     }
> 
>     div {
>         color: var(--color, blue);
>     }
> ```

As mentioned offline this does appear to work already, so I'd suggest having a test explicitly for this scenario and maybe tweaking this comment to also allow for that.

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125
>>> +                this.propertyVisibilityMode = WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode.ShowAll;
>> 
>> What happens if a developer enables `ShowAll`, selects another element to hide the section, and then re-selects the first element to show this section again?  Does `ShowAll` persist or does it get reset each time the section is shown?  IMO it should be the former.
> 
> Yes, the shown properties persist when returning to the element.

What about other elements that share this rule?  e.g. if there's a style for `div` I wouldn't want every child of every `<div>` to have to click to show inherited properties over and over (though perhaps in that case we may want a "Hide" button so that if the developer just wants to see it quickly without permanently altering everything they can).

[Patrick Angle]

Comment on attachment 430277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430277&action=review

Great tests! I agree with Devin in your offline discussion regarding adding a few more test cases for more specific CSS variables being used be less specific CSS rules. A few notes throughout.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:52
> +    static getFoundVariableNames(string)

Nit: `get...` to me invokes just getting a value we already know, instead of doing more complex work. Perhaps `parseFoundVariableNames(string)` or `collectFoundVariableNames(string)`?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:958
> +                else {

Instead of a nesting even deeper here, can we `continue` on the above conditional?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:961
> +                        this._usedCSSVariables.addAll(variables);

Should we `continue` after this as well, since the next condition does the exact same thing? I do appreciate not combining these conditions for clarity, though!

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:237
> +    margin-left: calc(var(--css-declaration-horizontal-padding) + 17px);

If this is to match `SpreadsheetCSSStyleDeclarationEditor.css:44`, can we turn either the entire value into a variable or at least turn the `17px` into a variable?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:112
> +            // showHiddenVariablesButtonElement.role = "button";

Oops?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:116
> +            let labelSingular = WI.UIString("Show %d unused CSS variable", "Show %d unused CSS variable (singular) @ Styles Sidebar Panel", "Text label for button to reveal one unused CSS variable");
> +            let labelPlural = WI.UIString("Show %d unused CSS variables", "Show %d unused CSS variables (plural) @ Styles Sidebar Panel", "Text label for button to reveal multiple unused CSS variables");
> +            let label = this._hiddenUnusedVariables.size > 1 ? labelPlural : labelSingular;

Can we do this without loading both strings every time and instead only call `WI.UIString` for the string we need?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:266
> +        this._hiddenUnusedVariables = new Set;

Could we just `this._hiddenUnusedVariables.clear()`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:730
> +        this._delegate?.spreadsheetCSSStyleDeclarationEditorSetAllPropertyVisibilityMode(propertyVisibilityMode);

Are all delegates guaranteed to have this function, or should this match SpreadsheetCSSStyleDeclartionSection.js:294 where the function isn't assumed to be present. e.g.
```
this._delegate?.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode?.(propertyVisibilityMode);
```

> LayoutTests/inspector/css/getFoundVariableNames-expected.txt:29
> +PASS: var(
> +--one
> +) should contain these CSS variable names: ["--one"].

Can you surround the tested expression in `"..."` to make it clearer that it is the input, particularly in this cases where the input spans multiple lines?.

> LayoutTests/inspector/css/getFoundVariableNames.html:15
> +            async test() {

Nit: This doesn't need to be `async`, since there is no `await`ing in this test.

> LayoutTests/inspector/css/getFoundVariableNames.html:17
> +                InspectorTest.expectShallowEqual(variables, expected, `${expression} should contain these CSS variable names: ${JSON.stringify(expected)}.`);

See getFoundVariableNames-expected.txt:27-29
```
InspectorTest.expectShallowEqual(variables, expected, `"${expression}" should contain these CSS variable names: ${JSON.stringify(expected)}.`);
```

[Devin Rousso]

Comment on attachment 430277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430277&action=review

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:52
>> +    static getFoundVariableNames(string)
> 
> Nit: `get...` to me invokes just getting a value we already know, instead of doing more complex work. Perhaps `parseFoundVariableNames(string)` or `collectFoundVariableNames(string)`?

Or maybe just `findVariableNames`?

>> LayoutTests/inspector/css/getFoundVariableNames.html:15
>> +            async test() {
> 
> Nit: This doesn't need to be `async`, since there is no `await`ing in this test.

This actually does need to be `async` because it's using an async suite.  Without the `async` you'd have to have at least a `resolve` parameter that gets called.

Along these lines tho, why not just use a sync suite?

[Razvan Caliman]

Comment on attachment 430277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430277&action=review

>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:52
>>> +    static getFoundVariableNames(string)
>> 
>> Nit: `get...` to me invokes just getting a value we already know, instead of doing more complex work. Perhaps `parseFoundVariableNames(string)` or `collectFoundVariableNames(string)`?
> 
> Or maybe just `findVariableNames`?

Renamed to `findVariableNames` and updated the corresponding test

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:958
>> +                else {
> 
> Instead of a nesting even deeper here, can we `continue` on the above conditional?

Yes, that works fine too. 
I generally prefer the if/else approach because it clarifies which code path applies to which type of styles (inherited vs matching), but I don't have strong feelings about it. `continue` it is.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:237
>> +    margin-left: calc(var(--css-declaration-horizontal-padding) + 17px);
> 
> If this is to match `SpreadsheetCSSStyleDeclarationEditor.css:44`, can we turn either the entire value into a variable or at least turn the `17px` into a variable?

Added a new variable, `--css-declaration-indentation`,  to `Source/WebInspectorUI/UserInterface/Views/Variables.css`. 
Yet another unused variable inherited by all nodes :) Thankfully we're getting a feature to hide those now :)

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:116
>> +            let label = this._hiddenUnusedVariables.size > 1 ? labelPlural : labelSingular;
> 
> Can we do this without loading both strings every time and instead only call `WI.UIString` for the string we need?

A previous review comment advised against multi-level ternary operator. Putting it all on one line makes very long.
I opted for the temp variables instead. This is similar to `source/webinspectorui/userinterface/views/AuditTestGroupContentView.js:167`

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:266
>> +        this._hiddenUnusedVariables = new Set;
> 
> Could we just `this._hiddenUnusedVariables.clear()`?

Indeed.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:730
>> +        this._delegate?.spreadsheetCSSStyleDeclarationEditorSetAllPropertyVisibilityMode(propertyVisibilityMode);
> 
> Are all delegates guaranteed to have this function, or should this match SpreadsheetCSSStyleDeclartionSection.js:294 where the function isn't assumed to be present. e.g.
> ```
> this._delegate?.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode?.(propertyVisibilityMode);
> ```

The check is better. Thanks!

>> LayoutTests/inspector/css/getFoundVariableNames-expected.txt:29
>> +) should contain these CSS variable names: ["--one"].
> 
> Can you surround the tested expression in `"..."` to make it clearer that it is the input, particularly in this cases where the input spans multiple lines?.

Good idea. Done!

>>> LayoutTests/inspector/css/getFoundVariableNames.html:15
>>> +            async test() {
>> 
>> Nit: This doesn't need to be `async`, since there is no `await`ing in this test.
> 
> This actually does need to be `async` because it's using an async suite.  Without the `async` you'd have to have at least a `resolve` parameter that gets called.
> 
> Along these lines tho, why not just use a sync suite?

Indeed, no need for an async suite. I duplicated another test and modified it. These are leftovers I forgot to change.

>> LayoutTests/inspector/css/getFoundVariableNames.html:17
>> +                InspectorTest.expectShallowEqual(variables, expected, `${expression} should contain these CSS variable names: ${JSON.stringify(expected)}.`);
> 
> See getFoundVariableNames-expected.txt:27-29
> ```
> InspectorTest.expectShallowEqual(variables, expected, `"${expression}" should contain these CSS variable names: ${JSON.stringify(expected)}.`);
> ```

Added quotes as requested above.

[Razvan Caliman]

Created attachment 430492 [details]
Patch

[Patrick Angle]

Comment on attachment 430492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430492&action=review

Looks great, just a few nits, provision r+ from me.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:52
> +    // FIXME: This naively collects variable-like names used in values.

Generally preferred to open a bugzilla bug and link it here.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:53
> +    // It's sufficient for addressing <https://webkit.org/b/225972>, but should be hardened if used for something that requires accuracy.

I wouldn't mention this current bug, as blaming this code in an editor will lead back to this patch already. I would just open the bug for the fixme, which should make it apparent if someone else comes along to use this that there are things to look out for.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:950
> +        this._usedCSSVariables = new Set;

Can this be `.clear()` like you changed SpreadsheetCSSStyleDeclarationEditor.js:265 to? I think that makes it clearer the intention here is to literally clear out this set every time.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:958
> +                    // FIXME: Support the case of variables declared on matching styles but not used anywhere.
> +                    // For <https://webkit.org/b/225972>, it was sufficient to determine used variables just from inherited styles.

DITTO CSSProperty.js:52-53

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:972
> +                // FIXME: Support the edge-case where a variable is declared in a higher specificity rule but used in a lower specificity one.

DITTO CSSProperty.js:52

> LayoutTests/inspector/css/usedCSSVariables.html:39
> +        }

STYLE trailing comma

> LayoutTests/inspector/css/usedCSSVariables.html:50
> +        }

DITTO :39

> LayoutTests/inspector/css/usedCSSVariables.html:61
> +        }

DITTO :39

> LayoutTests/inspector/css/usedCSSVariables.html:71
> +        }

DITTO :39

> LayoutTests/inspector/css/usedCSSVariables.html:81
> +        }

DITTO :39

> LayoutTests/inspector/css/usedCSSVariables.html:90
> +        }

DITTO :39

[Devin Rousso]

Comment on attachment 430492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430492&action=review

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:950
>> +        this._usedCSSVariables = new Set;
> 
> Can this be `.clear()` like you changed SpreadsheetCSSStyleDeclarationEditor.js:265 to? I think that makes it clearer the intention here is to literally clear out this set every time.

One major thing worth considering (especially when the object is exposed as "API") when deciding between `.clear()` and `new Set`(or `Map` or whatever) is that the former is still the same object, meaning that any other holders of the object elsewhere will also be affected by this operation, whereas with the latter that other object is left alone.  This has bitten us in the past (e.g. <https://webkit.org/b/198276>) and as such we've sorta defaulted to creating a new object instead of `.clear()` (which should ideally be similar performance) in order to avoid this effectively invisible footgun.  While this patch doesn't appear to have that problem, I'd still suggest going the route of `new Set` (or `Map` or whatever) just in case it becomes this way in the future.  Generally speaking, if it's important for other code to know about modifications to this object we should be dispatching an event and having the other code fetch the new value instead of relying on this implicit connection/behavior.

As such, I'd keep it as `new Set` here.

The `.clear()` you have below is fine because it's not exposed as "API", but you also could change that to `new Set` too if you want.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:972
>> +                // FIXME: Support the edge-case where a variable is declared in a higher specificity rule but used in a lower specificity one.
> 
> DITTO CSSProperty.js:52

Also I thought you said this already works?  If it doesn't then it definitely needs an actual bug like @Patrick Angle suggested, but if it does then I'd drop the FIXME and rework the first sentence (or the whole paragraph) to explain how higher specificity variables are also accounted for in this system.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:111
> +            showHiddenVariablesButtonElement.title = WI.UIString("Option-click to show unused CSS variables from all rules", "Option-click to show unused CSS variables from all rules @ Styles Sidebar Panel Tooltip", "Tooltip with instructions on how to show all hidden CSS variables");

NIT: it seems a little odd to have the `title` describe different functionality than what regular clicking would do, but I suppose that since the regular click is described in the `textContent` it's fine

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:114
> +            let labelSingular = WI.UIString("Show %d unused CSS variable", "Show %d unused CSS variable (singular) @ Styles Sidebar Panel", "Text label for button to reveal one unused CSS variable");
> +            let labelPlural = WI.UIString("Show %d unused CSS variables", "Show %d unused CSS variables (plural) @ Styles Sidebar Panel", "Text label for button to reveal multiple unused CSS variables");

Style: these can be `const` since they never change between Web Inspector sessions

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:337
> +        if (this._hiddenUnusedVariables.has(property)) {

I think this means that the first instance of the variable is matched instead of the instance of the variable that's actually applied being matched.  Is this true?  If so, you may need to check `!property.overridden`  too.

NIT: I'd move this above both of the functions so that if we're dealing with a variable we don't need to even create those functions :)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:729
> +        this._delegate?.spreadsheetCSSStyleDeclarationEditorSetAllPropertyVisibilityMode?.(propertyVisibilityMode);

Style: this should really provide `this` as the first argument as is normal/proper delegate convention

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:294
> +        this._delegate?.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode?.(propertyVisibilityMode);

Style: this should really provide `this` as the first argument as is normal/proper delegate convention

> Source/WebInspectorUI/UserInterface/Views/Variables.css:204
> +    --css-declaration-indentation: calc(var(--css-declaration-horizontal-padding) + 17px);

Personally, I don't really see any benefit for having these at the top-level.  I think this would be better being on the lowest common ancestor to all styles that need it (e.g. `.spreadsheet-style-declaration-editor`).

> LayoutTests/inspector/css/findVariableNames.html:109
> +        expression: "var(--var(one))",
> +        expected: ["--var(one"],

This seems surprising.  I wouldn't have expected "(" to be a valid character in a variable name o_0

> LayoutTests/inspector/css/usedCSSVariables-expected.txt:9
> +Uses inherited variable
> +Uses referenced inherited variable
> +Does not use inherited variable
> +Uses inherited variable in function value
> +Uses inherited variable declared in higher specificity rule but used in lower specificity rule
> +
> +Does not use inherited variable declared in higher specificity rule but used in lower specificity rule

NIT: This kind of output in the expectations file isn't super helpful and can sometimes confuse other engineers who are unfamiliar with the test (e.g. bot watchers trying to determine the nature of a test failure).  As such, I'd get rid of it (i.e. remove the text content from the nodes inside the `<body>` and just let the `id`/`class` indicate the purpose).

[Razvan Caliman]

Created attachment 430585 [details]
Patch

Address code review comments; Updated patch

[Razvan Caliman]

Comment on attachment 430492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430492&action=review

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:53
>> +    // It's sufficient for addressing <https://webkit.org/b/225972>, but should be hardened if used for something that requires accuracy.
> 
> I wouldn't mention this current bug, as blaming this code in an editor will lead back to this patch already. I would just open the bug for the fixme, which should make it apparent if someone else comes along to use this that there are things to look out for.

That's a good point. Git blame is enough to identify the Bugzilla bug that introduced these changes. 
I filed new bugs and added them to the FIXMEs.

>>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:950
>>> +        this._usedCSSVariables = new Set;
>> 
>> Can this be `.clear()` like you changed SpreadsheetCSSStyleDeclarationEditor.js:265 to? I think that makes it clearer the intention here is to literally clear out this set every time.
> 
> One major thing worth considering (especially when the object is exposed as "API") when deciding between `.clear()` and `new Set`(or `Map` or whatever) is that the former is still the same object, meaning that any other holders of the object elsewhere will also be affected by this operation, whereas with the latter that other object is left alone.  This has bitten us in the past (e.g. <https://webkit.org/b/198276>) and as such we've sorta defaulted to creating a new object instead of `.clear()` (which should ideally be similar performance) in order to avoid this effectively invisible footgun.  While this patch doesn't appear to have that problem, I'd still suggest going the route of `new Set` (or `Map` or whatever) just in case it becomes this way in the future.  Generally speaking, if it's important for other code to know about modifications to this object we should be dispatching an event and having the other code fetch the new value instead of relying on this implicit connection/behavior.
> 
> As such, I'd keep it as `new Set` here.
> 
> The `.clear()` you have below is fine because it's not exposed as "API", but you also could change that to `new Set` too if you want.

Thank you for the detailed explanation. That makes a lot of sense.

I'll change the other instance back to `new Set` as well. For consistency if nothing else. 

I picked up this habit of clearing by redeclaring a new Set, Map, Array, etc. a longer time ago but without a clear reason why. Now I have one. Thank you! :)

>>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:972
>>> +                // FIXME: Support the edge-case where a variable is declared in a higher specificity rule but used in a lower specificity one.
>> 
>> DITTO CSSProperty.js:52
> 
> Also I thought you said this already works?  If it doesn't then it definitely needs an actual bug like @Patrick Angle suggested, but if it does then I'd drop the FIXME and rework the first sentence (or the whole paragraph) to explain how higher specificity variables are also accounted for in this system.

Yes, it already works. I got confused. I had even wrote the tests to sanity check myself (`LayoutTests/inspector/css/usedCSSVariables.html`) and still got confused. This FIXME doesn't apply at all.

I think the naming keeps throwing me off. This works, but not for the reasons you may expect. Let me explain:

This method collects all variable names found to be used in CSS property values. It is not a list of variables declared AND used; just used. 
There can be undeclared variables found in this list. Perhaps a better name is in order: `foundCSSVariables`?

For example:
```
color: var(--never-declared) // --never-declared is collected into the list of, *ahem*, usedCSSVariables.
```

This list is not a source of truth after having parsed and resolved variables. It is essentially an allowlist used later, during layout. CSS variable *declarations* are hidden if they are not found in this allowlist (i.e.: declared but unused). That's why specificity doesn't matter at this stage. If the variable name is found in a value, regardless of order or specificity, it is collected.

---

```
if (property.isVariable && this._usedCSSVariables.has(property.name))
   this._usedCSSVariables.addAll(variables);
``` 

The reason for this condition was a bit clearer before we replaced the `else` branch with the `continue`. 
It is for collecting chains of variable references within inherited styles. It's supposed to handle cases like these:

```
    html {
        --deep: green;
    }
    
    body {
        --middle: var(--deep);
    }

    div {
        color: var(--middle);
    }
```

Here, we want to show all inherited variables. Without that check, we'd lose the connection between `--middle` and `--deep`. 

We want to guard this and run only on inherited variables which were found/used on matching styles (The reference chain starts at `var(--middle)`  on `div` in the above example). The list of used variables on matching styles will have been already populated by the time inherited styles are processed (remember, styles are already ordered by cascade). My original comments about higher/lower specificity were probably misleading. This case is what I was referring to. 

That check is important because we want to avoid picking up a reference chain for the unused variables here when inspecting `<div>`:

```
    html {
        --deep-not-used: green;
    }
    
    body {
        --middle-not-used: var(--deep-not-used);
    }

    div {
        color: green;
    }
```

The YouTube.com case has plenty of inherited variable reference chains which ultimately don't get used on matching styles. Without this guard, we'd still render a lot of noise. 

---

In conclusion, I guess a lot of the confusion is coming from the expectation that this list of usedCSSVariables is authoritative when it is merely an optimistic allowlist.

Of course, I'd like to create an authoritative list of declared AND used CSS variables. But I encountered enough edge cases already to understand that this isn't trivial. And that goal is orthogonal to this one of making the Styles rendering faster by trimming the inherited properties list.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:111
>> +            showHiddenVariablesButtonElement.title = WI.UIString("Option-click to show unused CSS variables from all rules", "Option-click to show unused CSS variables from all rules @ Styles Sidebar Panel Tooltip", "Tooltip with instructions on how to show all hidden CSS variables");
> 
> NIT: it seems a little odd to have the `title` describe different functionality than what regular clicking would do, but I suppose that since the regular click is described in the `textContent` it's fine

This only slightly better than the Shift+Click on color swatches to cycle color formats :) 
Still one of the least discoverable features of developer tools in all browsers.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:337
>> +        if (this._hiddenUnusedVariables.has(property)) {
> 
> I think this means that the first instance of the variable is matched instead of the instance of the variable that's actually applied being matched.  Is this true?  If so, you may need to check `!property.overridden`  too.
> 
> NIT: I'd move this above both of the functions so that if we're dealing with a variable we don't need to even create those functions :)

> If so, you may need to check `!property.overridden` too.
That's a very good catch! Thanks!

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:204
>> +    --css-declaration-indentation: calc(var(--css-declaration-horizontal-padding) + 17px);
> 
> Personally, I don't really see any benefit for having these at the top-level.  I think this would be better being on the lowest common ancestor to all styles that need it (e.g. `.spreadsheet-style-declaration-editor`).

Ok, I'll move it in context.

>> LayoutTests/inspector/css/findVariableNames.html:109
>> +        expected: ["--var(one"],
> 
> This seems surprising.  I wouldn't have expected "(" to be a valid character in a variable name o_0

Hence the fast but naive parsing in `CSSProperty.findVariableNames()`. I can follow-up in an another patch to make this better. 

While it's still reasonable, I'd like to avoid involving the tokenizer from CodeMirror. That's more accurate, but also slower than this quick check on the hot path for every property value of every node selection.

>> LayoutTests/inspector/css/usedCSSVariables-expected.txt:9
>> +Does not use inherited variable declared in higher specificity rule but used in lower specificity rule
> 
> NIT: This kind of output in the expectations file isn't super helpful and can sometimes confuse other engineers who are unfamiliar with the test (e.g. bot watchers trying to determine the nature of a test failure).  As such, I'd get rid of it (i.e. remove the text content from the nodes inside the `<body>` and just let the `id`/`class` indicate the purpose).

Done.

>> LayoutTests/inspector/css/usedCSSVariables.html:39
>> +        }
> 
> STYLE trailing comma

Fixed in all instances. Thanks for spotting!

[Devin Rousso]

Comment on attachment 430585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430585&action=review

r=me, nice fix, thanks for iterating :)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:36
> +    --css-declaration-indentation: calc(var(--css-declaration-horizontal-padding) + 17px);

NIT: it'd be nice to also pull out the `17px` and give it a name (e.g. `--property-checkbox-width`) so that it's less of a magic number
NIT: Since this is declared on a class with "declaration" already in the name, it's a bit redundant.  How about just `--property-indentation`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:314
> +        if (!property.overridden && this._hiddenUnusedVariables.has(property)) {

Is it possible that somehow we have two instances of `WI.CSSProperty` that both refer to the same underlying CSS?  I think that's why the check for `canonicalName` is done below.  Might be worth investigating as a followup.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:729
> +        this._delegate?.spreadsheetCSSStyleDeclarationEditorSetAllPropertyVisibilityMode?.(propertyVisibilityMode);

Style: this should really provide `this` as the first argument as is normal/proper delegate convention

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:294
> +        this._delegate?.spreadsheetCSSStyleDeclarationSectionSetAllPropertyVisibilityMode?.(propertyVisibilityMode);

Style: this should really provide `this` as the first argument as is normal/proper delegate convention

[Razvan Caliman]

Created attachment 430839 [details]
Patch

Carry over R+; Address nits

[Razvan Caliman]

Comment on attachment 430839 [details]
Patch

Carry over R+; Remove review request.

[EWS]

Committed r278607 (238596@main): <https://commits.webkit.org/238596@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430839 [details].