Bug 233563 – Web Inspector: Computed Panel: Group CSS variables by value type

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Razvan Caliman]

<rdar://82978905>

[Razvan Caliman]

Created attachment 445304 [details]
Patch 1.0

Implement CSS variable grouping by type in Computed details sidebar panel

[Razvan Caliman]

Created attachment 445305 [details]
Screenshot with patch applied

Available CSS variable type groups when inspecting webkit.org

[Patrick Angle]

Comment on attachment 445304 [details]
Patch 1.0

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

Looking good (and thank you for the detailed changelog as well). Biggest question mark here remains the UI for actually choosing a group - I don't think this is quite it, yet - but I'll let that discussion continue offline where it started.

> Source/WebInspectorUI/ChangeLog:44
> +        We need to remove and rebuilt just the sections for CSS variables because

s/rebuilt/rebuild

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:148
> +    getVariablesStyleGroupedByType() {

Can we write tests for this? We should make sure variables are split into the expected groups, and also make sure that changes to specific properties as well as adding/removing rules correctly updates the results here.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:1049
> +    Ungrouped: "Ungrouped",
> +    Colors: "Colors",
> +    Dimensions: "Dimensions",
> +    Numbers: "Numbers",
> +    Other: "Other",

NIT: Values for enum-like keys are typically all lower-cased. e.g. `Ungrouped: "ungrouped",`

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:44
> +            [WI.DOMNodeStyles.VariablesGroupType.Ungrouped]: WI.UIString("Ungrouped"),
> +            [WI.DOMNodeStyles.VariablesGroupType.Colors]: WI.UIString("Colors"),
> +            [WI.DOMNodeStyles.VariablesGroupType.Dimensions]: WI.UIString("Dimensions"),
> +            [WI.DOMNodeStyles.VariablesGroupType.Numbers]: WI.UIString("Numbers"),
> +            [WI.DOMNodeStyles.VariablesGroupType.Other]: WI.UIString("Other"),

These strings should probably have localization keys and descriptions, since they are rather generic bits of text with a very specific purpose in the UI.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:80
> +                return;

Does this need to be a `return`, or could it be a `break` instead so that we still reach the below call to `super.refresh()`? (Currently super.refresh here is an empty function, but still better to not cause problems down the road.)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:156
> +        let variablesNavigationRow = new WI.DetailsSectionRow;

Noting here that there is some discussion offline about wether or not a navigation/scope bar is the correct UI for this. My primary concern currently is that another inline scope bar carries quite a bit of visual weight and is somewhat confusing since an almost/completely identical scope bar is at the top of the sidebar to choose which Details Panel is visible.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:168
> +        createVariableGroupingModeScopeBarItem(WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped, "Ungrouped");
> +        createVariableGroupingModeScopeBarItem(WI.ComputedStyleDetailsPanel.VariablesGroupingMode.ByType, "By Type");

"Ungrouped" and "By Type" should be localized strings (also like on :40-44 I'd prefer they include a description and key for localizers, particularly since "Ungrouped" appears in both places, but with a potentially different meaning.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:232
> +        // variablesStyleSection.sortPropertiesByName = true;

Oops? For what it's worth I'd lean towards keeping the sorting within each section to match the currently implementation.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:247
> +        if (!label) {

I wonder if this would all read more clearly (including at the call sites) if instead of passing a label in, you used a `showGroupLabel` bool as a parameter (defaults to false) and looked up the group label text by type below instead? Up to you, though.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:285
> +        if (event && event.data && !event.data.collapsed) {

NIT: Can we rework this to be an early return to reduce the number of levels deep we end up nested below? e.g.
```
if (!event || !event.data || event.data.collapsed)
    return;
```

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:289
> +                    continue;

Is it possible for the event's target to match multiple `detailsSection`, or could this be an early return instead?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:328
> +    Ungrouped: "Ungrouped",
> +    ByType: "ByType",

NIT: DOMNodeStyles.js:1045-1049 e.g. `ByType: "by-type"`

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:329
> +    ByUsage: "ByUsage",

Unused?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:37
> -        this._styleTraces = [];
> +        this._styleTraces = null;

I'm not sure I understand why this needs to change based on the rest of the patch? Was this previously just incorrect? If so, can you add a note that this is a drive-by change to the changelog (although not really a drive-by since there are no other changes in this file currently.

[Razvan Caliman]

Comment on attachment 445304 [details]
Patch 1.0

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

>> Source/WebInspectorUI/ChangeLog:44
>> +        We need to remove and rebuilt just the sections for CSS variables because
> 
> s/rebuilt/rebuild

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:148
>> +    getVariablesStyleGroupedByType() {
> 
> Can we write tests for this? We should make sure variables are split into the expected groups, and also make sure that changes to specific properties as well as adding/removing rules correctly updates the results here.

Agreed with the need to add a test. With your permission, I'd like to do it in a follow-up patch so this one can progress to landing. It is a blocker for implementing grouping variables by consumer.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:1049
>> +    Other: "Other",
> 
> NIT: Values for enum-like keys are typically all lower-cased. e.g. `Ungrouped: "ungrouped",`

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:44
>> +            [WI.DOMNodeStyles.VariablesGroupType.Other]: WI.UIString("Other"),
> 
> These strings should probably have localization keys and descriptions, since they are rather generic bits of text with a very specific purpose in the UI.

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:80
>> +                return;
> 
> Does this need to be a `return`, or could it be a `break` instead so that we still reach the below call to `super.refresh()`? (Currently super.refresh here is an empty function, but still better to not cause problems down the road.)

The superclass expects `refresh()` to be implemented by the subclass. Calling `super.refresh()` isn't needed here at all. 

If we hit the return, it means a section is missing so we need to redo layout. Letting the rest of this method run filtering on sections that will be destroyed by layout isn't useful. But there's no harm in it either. 

I'll replace the `return` with a `break` for future-proofing.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:156
>> +        let variablesNavigationRow = new WI.DetailsSectionRow;
> 
> Noting here that there is some discussion offline about wether or not a navigation/scope bar is the correct UI for this. My primary concern currently is that another inline scope bar carries quite a bit of visual weight and is somewhat confusing since an almost/completely identical scope bar is at the top of the sidebar to choose which Details Panel is visible.

Replaced with a visual treatment similar to the one for the context execution selector in the Console.
Moved the scope bar to the Variables section header so it's sticky as the list scrolls.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:168
>> +        createVariableGroupingModeScopeBarItem(WI.ComputedStyleDetailsPanel.VariablesGroupingMode.ByType, "By Type");
> 
> "Ungrouped" and "By Type" should be localized strings (also like on :40-44 I'd prefer they include a description and key for localizers, particularly since "Ungrouped" appears in both places, but with a potentially different meaning.

Done. Added descriptions and keys.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:232
>> +        // variablesStyleSection.sortPropertiesByName = true;
> 
> Oops? For what it's worth I'd lean towards keeping the sorting within each section to match the currently implementation.

This was a stray commented-out property. Removed.

Sorting was opt-on when using `WI.SpreadsheetCSSStyleDeclarationEditor`, but `WI.ComputedStyleSection` does sorting by name by default. 
https://sourcegraph.com/github.com/WebKit/WebKit-http/-/blob/Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js?L189

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:247
>> +        if (!label) {
> 
> I wonder if this would all read more clearly (including at the call sites) if instead of passing a label in, you used a `showGroupLabel` bool as a parameter (defaults to false) and looked up the group label text by type below instead? Up to you, though.

I'm hesitant to add label matching inside. When adding grouping by consumer this could lead to collisions if we have identical group types (ex "other"). The labels may be similar in English, but I can't be sure there aren't differences in other locales. Feeding the labels from outside feels more predictable.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:285
>> +        if (event && event.data && !event.data.collapsed) {
> 
> NIT: Can we rework this to be an early return to reduce the number of levels deep we end up nested below? e.g.
> ```
> if (!event || !event.data || event.data.collapsed)
>     return;
> ```

Good idea. Better still:

```
if (event?.data?.collapsed)
    return
```

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:289
>> +                    continue;
> 
> Is it possible for the event's target to match multiple `detailsSection`, or could this be an early return instead?

One `detailsSection` per target. Early return is fine. Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:328
>> +    ByType: "ByType",
> 
> NIT: DOMNodeStyles.js:1045-1049 e.g. `ByType: "by-type"`

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:329
>> +    ByUsage: "ByUsage",
> 
> Unused?

Yup, unused. This belongs with a different patch. Removed.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:37
>> +        this._styleTraces = null;
> 
> I'm not sure I understand why this needs to change based on the rest of the patch? Was this previously just incorrect? If so, can you add a note that this is a drive-by change to the changelog (although not really a drive-by since there are no other changes in this file currently.

The default array value is never used. It throws an error in `WI.ComputedStyleSection.layout()` because it is expected to be a `Map`:

```
let propertyTrace = this._styleTraces ? this._styleTraces.get(property.name) : null;
```

For the section with computed CSS properties, `styleTraces` is set to a `Map` by `WI.ComputedStyleDetailsPanel._computePropertyTraces()`
It is never used for the sections with CSS variables, hence the need to default to a falsy value.

[Razvan Caliman]

Created attachment 446240 [details]
Patch 1.2

Address code review feedback:
- moved scope bar navigation to section header
- fix nits
- clarified CHANGELOG
- tweaks to styles to account for nested details sections

[Razvan Caliman]

Created attachment 446241 [details]
Screenshot with patch applied

[Devin Rousso]

Comment on attachment 446240 [details]
Patch 1.2

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

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:148
> +    getVariablesStyleGroupedByType() {

Style: `{` on next line

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:152
> +        let typeCheckFunctions = {

Style: this can be `const` since nothing about it changes between Web Inspector sessions

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:153
> +            [WI.DOMNodeStyles.VariablesGroupType.Colors]: (property) => {

this doesn't need to be an arrow function

NIT: you could inline the body

ditto (below)

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:154
> +                return WI.Color.fromString(property.value) !== null;

Style: we normally prefer just a `!` instead of directly checking against a falsy value (and we don't like to have a function return different falsy values)

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:158
> +                return /-?^\d+(\.\d+)?[a-z]{1,5}$/.test(property.value);

Did you mean to put the `^` at the start?  Or is this some new regex syntax/feature im not familiar with?

Also, why only `{1,5}`?  What about `%`?  Since we're just doing a basic `test`, I think it'd be easier (and more future-proof) to just check for digits followed by non-digits with no space (e.g. `/^-?\d+(\.\d+)?\D+$/`).

ditto (below)

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:163
> +            // Catch-all for the rest of the variables. Keep last

If these are ordered, I'd move the comment to the top above `let typeCheckFunctions = {` so that it's clear from the start.  

Style: missing "." at the end

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:167
> +        let properties = this?._computedStyle?.properties;

`this` shouldn't ever be falsy

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:172
> +        let variables = new Set(properties.filter(property => property.isVariable));

Why does this need to be a `Set`?  I'd hope that the `properties` of a computed style are already unique.

Also, this means that we're iterating over `properties` twice.  Can we restructure this so that we only do it once?
```
    let variablesForType = {};
    for (let property of properties) {
        if (!property.isVariable)
            continue;

        for (let [type, checker] of typeCheckFunctions) {
            if (checker(variable)) {
                variablesForType[type] ||= [];
                variablesForType[type].push(variable);
                break;
            }
        }
    }
    for (let [type] of typeCheckerFunctions) {
        if (variablesForType[type]?.length) {
            ...
            this._variablesStyleByType.set(type, style);
        }
    }
```

Style: we always add `(` `)` around the parameters of an arrow function

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:174
> +        for (let type of Object.keys(typeCheckFunctions)) {

Rather than use an object that we then have to `Object.keys`, can we just make `typeCheckFunctions` into an array right at the start?
```
    const typeCheckFunctions = [
        [WI.DOMNodeStyles.VariablesGroupType.Colors, (property) => WI.Color.fromString(property.value))],
        ...
    ];
```

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:178
> +                    variables.take(variable);

This is modification while iterating, which is usually not a good thing (tho in this case I think JS `Set` is safe from that).

ditto (:172)

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:184
> +                let style = new WI.CSSStyleDeclaration(this, null, null, WI.CSSStyleDeclaration.Type.Computed, this._node, false, null, properties);

Please create `const` variables for all the hardcoded values here so that we know what they represent.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:39
> +        this._variablesGroupLabelByGroupType = {

Please make this into a `static` method on `WI.DOMNodeStyles` instead of having it as a local variable.
```
    static displayNameForVariablesGroupType(variablesGroupType)
    {
        console.assert(Object.values(WI.DOMNodeStyles.VariablesGroupType).includes(variablesGroupType), variablesGroupType);

        switch (variablesGroupType) {
        case WI.DOMNodeStyles.VariablesGroupType.Ungrouped:
            return WI.UIString("Ungrouped", "Ungrouped @ CSS variable grouping", "Label for CSS variables that don't fit into any other known group");
        ...
        }

        console.assert(false, "not reached");
        return "";
    }
```

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:71
> +        if (this._variablesGroupingModeSetting.value === WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped)

NIT: I'd use a `switch` to avoid repeating code (and IMO it looks nicer)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:102
> +        for (let styleSection of this._detailsSectionByStyleSectionMap.keys()) {

Style: no `{` `}` for single-line control flow

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:158
> +            scopeBarItem[WI.ComputedStyleDetailsPanel.VariablesGroupingModeSymbol] = mode;

This isn't really ideal.  It works, but we generally prefer to not use expando properties where possible.  Why not just make the value of each `WI.ComputedStyleDetailsPanel.VariablesGroupingMode` more verbose (e.g. `"computed-style-variables-grouping-mode-ungrouped"`) and then have the `mode` be the identifier?  Alternatively, you could have a `this._scopeBarItemForVariablesGroupingMode = new Map`?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:170
> +        this._variablesGroupingModeScopeBar = new WI.ScopeBar("computed-style-variables-grouping-mode-scope-bar", variablesGroupingModeScopeBarItems, variablesGroupingModeScopeBarItems[0], shouldGroupNonExclusiveItems);

It's redundant to add `-scope-bar` to the identifier as `WI.ScopeBar` will already add a `.scope-bar` class (i.e. you can do `.scope-bar.computed-style-variables-grouping-mode` instead).

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:192
> +            // The details section for computed properties is updated in-place by WI.ComputedStyleDetailsPanel.refresh()

Style: missing ending "."

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:202
> +            detailsSection?.element.remove();

Why would `detailsSection` be falsy?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:205
> +        if (this._variablesGroupingModeSetting.value === WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped) {

Style: no `{` `}` for single-line control flow

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:209
> +        if (this._variablesGroupingModeSetting.value === WI.ComputedStyleDetailsPanel.VariablesGroupingMode.ByType) {

It shouldn't be possible for `this._variablesGroupingModeSetting.value` to be equal to both `WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped` and `WI.ComputedStyleDetailsPanel.VariablesGroupingMode.ByType` at the same time.

ditto (:71)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:211
> +            for (let [type, style] of styleGroups) {

ditto (:205)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:227
> +    _createVariablesStyleSection()

Why do we need a separate function for this?  Can we just inline it inside `_renderVariablesStyleSectionGroup`?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:234
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:238
> +    _renderVariablesStyleSectionGroup(style, groupType, label) {

Style: `{` on next line

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:252
> +            detailsSection = new WI.DetailsSection(`computed-style-variables-group-${groupType}`, label, [detailsSectionGroup]);

NIT: I don't usually use template strings where a single concatenation would suffice (i.e. `"computed-style-variables-group-" + groupType`).

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:284
> +        if (event?.data?.collapsed)

When would `event` be falsy?  Same with `event.data` (`WI.DetailsSection.Event.CollapsedStateChanged` always dispatches with `data`)?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:289
> +                styleSection.needsLayout();

This implies that no layouts happened while the `WI.DetailsSection` was collapsed.  Is that true?  I don't recall anything in `WI.DetailsSection` that would prevent layouts.

[Devin Rousso]

Comment on attachment 445304 [details]
Patch 1.0

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

>>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:148
>>> +    getVariablesStyleGroupedByType() {
>> 
>> Can we write tests for this? We should make sure variables are split into the expected groups, and also make sure that changes to specific properties as well as adding/removing rules correctly updates the results here.
> 
> Agreed with the need to add a test. With your permission, I'd like to do it in a follow-up patch so this one can progress to landing. It is a blocker for implementing grouping variables by consumer.

Seeing as how this is a new feature (and not a bugfix), I think having tests is necessary and should be added alongside the feature itself.

[Razvan Caliman]

Comment on attachment 446240 [details]
Patch 1.2

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:148
>> +    getVariablesStyleGroupedByType() {
> 
> Style: `{` on next line

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:152
>> +        let typeCheckFunctions = {
> 
> Style: this can be `const` since nothing about it changes between Web Inspector sessions

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:153
>> +            [WI.DOMNodeStyles.VariablesGroupType.Colors]: (property) => {
> 
> this doesn't need to be an arrow function
> 
> NIT: you could inline the body
> 
> ditto (below)

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:154
>> +                return WI.Color.fromString(property.value) !== null;
> 
> Style: we normally prefer just a `!` instead of directly checking against a falsy value (and we don't like to have a function return different falsy values)

Done.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:158
>> +                return /-?^\d+(\.\d+)?[a-z]{1,5}$/.test(property.value);
> 
> Did you mean to put the `^` at the start?  Or is this some new regex syntax/feature im not familiar with?
> 
> Also, why only `{1,5}`?  What about `%`?  Since we're just doing a basic `test`, I think it'd be easier (and more future-proof) to just check for digits followed by non-digits with no space (e.g. `/^-?\d+(\.\d+)?\D+$/`).
> 
> ditto (below)

> Did you mean to put the `^` at the start?
Yes, that's right. It was a mistake after changing the regex to fix a bug.
Fixed.

> Also, why only `{1,5}`?
1 is the shortest and 5 are the longest length units currently supported: `q` and `dvmax`, respectively. See `WI.CSSCompletions.lengthUnits`

> I think it'd be easier (and more future-proof) to just check for digits followed by non-digits with no space (e.g. `/^-?\d+(\.\d+)?\D+$/`).
Yeah, the current regex isn't very strict. Any 1–5 char string would pass, regardless if it's a valid length unit or not. And it's missing `%` indeed. I'll use your suggestion. 

I want to fix https://webkit.org/b/233576 so it's more strict. Now, it will put `90deg` or `60s` into the "Dimensions" group which is not right. They should fall into "Other".

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:163
>> +            // Catch-all for the rest of the variables. Keep last
> 
> If these are ordered, I'd move the comment to the top above `let typeCheckFunctions = {` so that it's clear from the start.  
> 
> Style: missing "." at the end

Done.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:167
>> +        let properties = this?._computedStyle?.properties;
> 
> `this` shouldn't ever be falsy

Fixed. I got confused by optional chaining operators many times before.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:172
>> +        let variables = new Set(properties.filter(property => property.isVariable));
> 
> Why does this need to be a `Set`?  I'd hope that the `properties` of a computed style are already unique.
> 
> Also, this means that we're iterating over `properties` twice.  Can we restructure this so that we only do it once?
> ```
>     let variablesForType = {};
>     for (let property of properties) {
>         if (!property.isVariable)
>             continue;
> 
>         for (let [type, checker] of typeCheckFunctions) {
>             if (checker(variable)) {
>                 variablesForType[type] ||= [];
>                 variablesForType[type].push(variable);
>                 break;
>             }
>         }
>     }
>     for (let [type] of typeCheckerFunctions) {
>         if (variablesForType[type]?.length) {
>             ...
>             this._variablesStyleByType.set(type, style);
>         }
>     }
> ```
> 
> Style: we always add `(` `)` around the parameters of an arrow function

> Why does this need to be a `Set`? I'd hope that the `properties` of a computed style are already unique.
Used a `Set` and `Set.take()` to successively reduce the list of variables to iterate over for each type.

> Also, this means that we're iterating over `properties` twice. Can we restructure this so that we only do it once?
I like your suggestion. Done.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:174
>> +        for (let type of Object.keys(typeCheckFunctions)) {
> 
> Rather than use an object that we then have to `Object.keys`, can we just make `typeCheckFunctions` into an array right at the start?
> ```
>     const typeCheckFunctions = [
>         [WI.DOMNodeStyles.VariablesGroupType.Colors, (property) => WI.Color.fromString(property.value))],
>         ...
>     ];
> ```

Done.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:178
>> +                    variables.take(variable);
> 
> This is modification while iterating, which is usually not a good thing (tho in this case I think JS `Set` is safe from that).
> 
> ditto (:172)

This was intentional to reduce the set size after each iteration. This is obsolete after implementing your suggestion above.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:184
>> +                let style = new WI.CSSStyleDeclaration(this, null, null, WI.CSSStyleDeclaration.Type.Computed, this._node, false, null, properties);
> 
> Please create `const` variables for all the hardcoded values here so that we know what they represent.

Done.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:39
>> +        this._variablesGroupLabelByGroupType = {
> 
> Please make this into a `static` method on `WI.DOMNodeStyles` instead of having it as a local variable.
> ```
>     static displayNameForVariablesGroupType(variablesGroupType)
>     {
>         console.assert(Object.values(WI.DOMNodeStyles.VariablesGroupType).includes(variablesGroupType), variablesGroupType);
> 
>         switch (variablesGroupType) {
>         case WI.DOMNodeStyles.VariablesGroupType.Ungrouped:
>             return WI.UIString("Ungrouped", "Ungrouped @ CSS variable grouping", "Label for CSS variables that don't fit into any other known group");
>         ...
>         }
> 
>         console.assert(false, "not reached");
>         return "";
>     }
> ```

Sure this works too. Is there any reason to do it like this aside from the assertions?

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:71
>> +        if (this._variablesGroupingModeSetting.value === WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped)
> 
> NIT: I'd use a `switch` to avoid repeating code (and IMO it looks nicer)

Done.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:102
>> +        for (let styleSection of this._detailsSectionByStyleSectionMap.keys()) {
> 
> Style: no `{` `}` for single-line control flow

Done.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:158
>> +            scopeBarItem[WI.ComputedStyleDetailsPanel.VariablesGroupingModeSymbol] = mode;
> 
> This isn't really ideal.  It works, but we generally prefer to not use expando properties where possible.  Why not just make the value of each `WI.ComputedStyleDetailsPanel.VariablesGroupingMode` more verbose (e.g. `"computed-style-variables-grouping-mode-ungrouped"`) and then have the `mode` be the identifier?  Alternatively, you could have a `this._scopeBarItemForVariablesGroupingMode = new Map`?

To be fair, I reused prior art from the Sources panel introduced ~2 years ago: https://trac.webkit.org/changeset/248916/webkit#file6
If this is no longer the way to do it, it'd be nice to know why.

I implemented your suggestion to use the `mode` for `ScopeBarItem.id`.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:170
>> +        this._variablesGroupingModeScopeBar = new WI.ScopeBar("computed-style-variables-grouping-mode-scope-bar", variablesGroupingModeScopeBarItems, variablesGroupingModeScopeBarItems[0], shouldGroupNonExclusiveItems);
> 
> It's redundant to add `-scope-bar` to the identifier as `WI.ScopeBar` will already add a `.scope-bar` class (i.e. you can do `.scope-bar.computed-style-variables-grouping-mode` instead).

Done.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:192
>> +            // The details section for computed properties is updated in-place by WI.ComputedStyleDetailsPanel.refresh()
> 
> Style: missing ending "."

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:202
>> +            detailsSection?.element.remove();
> 
> Why would `detailsSection` be falsy?

Misplaced optional chaining operator. Again.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:209
>> +        if (this._variablesGroupingModeSetting.value === WI.ComputedStyleDetailsPanel.VariablesGroupingMode.ByType) {
> 
> It shouldn't be possible for `this._variablesGroupingModeSetting.value` to be equal to both `WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped` and `WI.ComputedStyleDetailsPanel.VariablesGroupingMode.ByType` at the same time.
> 
> ditto (:71)

Done.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:227
>> +    _createVariablesStyleSection()
> 
> Why do we need a separate function for this?  Can we just inline it inside `_renderVariablesStyleSectionGroup`?

No particular reason. Just to avoid making the body of `_renderVariablesStyleSectionGroup()` too large.
There used to be more logic in these methods in a previous WIP. Inlining works. 

Done.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:238
>> +    _renderVariablesStyleSectionGroup(style, groupType, label) {
> 
> Style: `{` on next line

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:284
>> +        if (event?.data?.collapsed)
> 
> When would `event` be falsy?  Same with `event.data` (`WI.DetailsSection.Event.CollapsedStateChanged` always dispatches with `data`)?

Trigger happy with optional chaining operator to translate prior logic. Fixed.

[Razvan Caliman]

Created attachment 446393 [details]
Patch 1.3

Add test and address code review feedback

[Devin Rousso]

Comment on attachment 446240 [details]
Patch 1.2

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

>>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:39
>>> +        this._variablesGroupLabelByGroupType = {
>> 
>> Please make this into a `static` method on `WI.DOMNodeStyles` instead of having it as a local variable.
>> ```
>>     static displayNameForVariablesGroupType(variablesGroupType)
>>     {
>>         console.assert(Object.values(WI.DOMNodeStyles.VariablesGroupType).includes(variablesGroupType), variablesGroupType);
>> 
>>         switch (variablesGroupType) {
>>         case WI.DOMNodeStyles.VariablesGroupType.Ungrouped:
>>             return WI.UIString("Ungrouped", "Ungrouped @ CSS variable grouping", "Label for CSS variables that don't fit into any other known group");
>>         ...
>>         }
>> 
>>         console.assert(false, "not reached");
>>         return "";
>>     }
>> ```
> 
> Sure this works too. Is there any reason to do it like this aside from the assertions?

We usually do this for a few reasons.
- It makes the actual UI logic is a bit cleaner (i.e. don't have to do work to convert an enum to a UI string).
- It helps avoid repeated code if other places need to do this too.
- It puts it in the same place as the definition of the enum, meaning that if a new value is added to the enum then it's more likely for the engineer to add it to the `switch` too.

>>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:158
>>> +            scopeBarItem[WI.ComputedStyleDetailsPanel.VariablesGroupingModeSymbol] = mode;
>> 
>> This isn't really ideal.  It works, but we generally prefer to not use expando properties where possible.  Why not just make the value of each `WI.ComputedStyleDetailsPanel.VariablesGroupingMode` more verbose (e.g. `"computed-style-variables-grouping-mode-ungrouped"`) and then have the `mode` be the identifier?  Alternatively, you could have a `this._scopeBarItemForVariablesGroupingMode = new Map`?
> 
> To be fair, I reused prior art from the Sources panel introduced ~2 years ago: https://trac.webkit.org/changeset/248916/webkit#file6
> If this is no longer the way to do it, it'd be nice to know why.
> 
> I implemented your suggestion to use the `mode` for `ScopeBarItem.id`.

Ah I see.  I think a major difference is that `WI.ResourceGroupingMode` is something that's used in a few different places (including the "global" setting `WI.settings.resourceGroupingMode`) whereas `WI.ComputedStyleDetailsPanel.VariablesGroupingModeSymbol` is only used by this class.  If `WI.ComputedStyleDetailsPanel.VariablesGroupingModeSymbol` was used in more places then I'd be more OK with what you did.

[Devin Rousso]

Comment on attachment 446393 [details]
Patch 1.3

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

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:51
> +        this._variablesStyleByType = null;

NIT: "variables style by type" is awkward to read.  I think "variable style for type" would read nicer.
NIT: instead of having this default to `null`, can we have it always be a `Map` that you just `.clear()` inside `refresh()`?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:148
> +    getVariablesStyleGroupedByType()

Style: It's odd to have a function prefixed with `get` that has no parameters.  Can we make this into `get variablesStyleGroupedByType` instead?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:155
> +        const typeCheckFunctions = [

NIT: you can move this below the `if (!properties)` early-return since it's not used till after that

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:157
> +            // FIXME: <https://webkit.org/b/233576> build RegEx from `WI.CSSCompletions.lenghtUnits`.

NIT: it's actually `RegExp` 😅
NIT: s/lenght/length

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:160
> +            [WI.DOMNodeStyles.VariablesGroupType.Other, (property) => true]

Style: missing trailing comma

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:188
> +                let style = new WI.CSSStyleDeclaration(this, ownerStyleSheet, id, WI.CSSStyleDeclaration.Type.Computed, this._node, inherited, text, variablesForType[type]);

Aside: we should clean up `WI.CSSStyleDeclaration` as a followup, e.g.
```
    constructor(type, nodeStyles, {id, ownerStyleSheet, styleSheetTextRange, node, properties, text, inherited} = {})
```

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:-49
> -.sidebar > .panel.details.css-style > .content > .computed .property {
> -    position: relative;

Is this not needed anymore?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:34
> +        this._variablesGroupingModeSetting = new WI.Setting("computed-style-variables-grouping", WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped);

NIT: the `name` of the `WI.Setting` should match the variable its stored in, so `"computed-style-variables-grouping-mode"`

Thinking about this a bit more, I dunno if we actually even need a `WI.Setting` for this since `WI.ScopeBarItem` maintains it's own `WI.Setting`.  I'd try that instead.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:38
> +        this._variablesStyleSectionByGroupTypeMap = new Map;

ditto (Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:51)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:49
> +            return WI.UIString("Colors", "Colors @ Computed Style variables section", "Section header for the group of CSS variables with colors as values");

Style: I'd add a newline after this so there's a bit more space in this block of text

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:88
> +            break;

ditto (:49)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:217
> +            detailsSection.element?.remove();

It's not possible for `WI.DetailsSection` to have a falsy `element`.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:223
> +            break;

ditto (:49)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:248
> +        if (style)

When is `style` ever falsy?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:297
> +                styleSection.needsLayout();

This implies that no layouts happened while the `WI.DetailsSection` was collapsed. Is that true? I don't recall anything in `WI.DetailsSection` that would prevent layouts.

[Razvan Caliman]

Comment on attachment 446393 [details]
Patch 1.3

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:51
>> +        this._variablesStyleByType = null;
> 
> NIT: "variables style by type" is awkward to read.  I think "variable style for type" would read nicer.
> NIT: instead of having this default to `null`, can we have it always be a `Map` that you just `.clear()` inside `refresh()`?

> NIT: "variables style by type" is awkward to read. I think "variable style for type" would read nicer.
Done

> NIT: instead of having this default to `null`, can we have it always be a `Map` that you just `.clear()` inside `refresh()`?
We use the `null` to determine if the groups need to be recomputed. That's when a `Map` is created. 
If the `Map` is empty, it's ambiguous whether there are no variables or they weren't processed.
We'd need another flag to know that state. I'll keep this as-is.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:148
>> +    getVariablesStyleGroupedByType()
> 
> Style: It's odd to have a function prefixed with `get` that has no parameters.  Can we make this into `get variablesStyleGroupedByType` instead?

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:155
>> +        const typeCheckFunctions = [
> 
> NIT: you can move this below the `if (!properties)` early-return since it's not used till after that

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:157
>> +            // FIXME: <https://webkit.org/b/233576> build RegEx from `WI.CSSCompletions.lenghtUnits`.
> 
> NIT: it's actually `RegExp` 😅
> NIT: s/lenght/length

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:160
>> +            [WI.DOMNodeStyles.VariablesGroupType.Other, (property) => true]
> 
> Style: missing trailing comma

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:188
>> +                let style = new WI.CSSStyleDeclaration(this, ownerStyleSheet, id, WI.CSSStyleDeclaration.Type.Computed, this._node, inherited, text, variablesForType[type]);
> 
> Aside: we should clean up `WI.CSSStyleDeclaration` as a followup, e.g.
> ```
>     constructor(type, nodeStyles, {id, ownerStyleSheet, styleSheetTextRange, node, properties, text, inherited} = {})
> ```

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

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:-49
>> -    position: relative;
> 
> Is this not needed anymore?

I don't think it was necessary for a while now. I don't see any elements within which require it. All swatches and buttons are positioned correctly.

Swapping out `WI.SpreadsheetCSSStyleDeclarationEditor` for `WI.ComputedStyleSection` also changes the DOM structure. `.computed-property-item` is what we want to target.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:34
>> +        this._variablesGroupingModeSetting = new WI.Setting("computed-style-variables-grouping", WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped);
> 
> NIT: the `name` of the `WI.Setting` should match the variable its stored in, so `"computed-style-variables-grouping-mode"`
> 
> Thinking about this a bit more, I dunno if we actually even need a `WI.Setting` for this since `WI.ScopeBarItem` maintains it's own `WI.Setting`.  I'd try that instead.

Yes, this works too. Added a getter `get variablesGroupingMode()` which points to the setting value of the selected item.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:49
>> +            return WI.UIString("Colors", "Colors @ Computed Style variables section", "Section header for the group of CSS variables with colors as values");
> 
> Style: I'd add a newline after this so there's a bit more space in this block of text

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:88
>> +            break;
> 
> ditto (:49)

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:217
>> +            detailsSection.element?.remove();
> 
> It's not possible for `WI.DetailsSection` to have a falsy `element`.

Not yet. Perhaps when we turn it into a View that can get detached. Removed for now.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:223
>> +            break;
> 
> ditto (:49)

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:248
>> +        if (style)
> 
> When is `style` ever falsy?

Leftover from a previous WIP. Removed

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:297
>> +                styleSection.needsLayout();
> 
> This implies that no layouts happened while the `WI.DetailsSection` was collapsed. Is that true? I don't recall anything in `WI.DetailsSection` that would prevent layouts.

Indeed, that's true. The `WI.ComputedStyleSection` contents are updated even when its host `WI.DetailsSection` is collapsed. Setting `WI.ComputedStyleSection.style` triggers layout.

Seems like this was the case in the original source as well: 
https://sourcegraph.com/github.com/WebKit/WebKit-http/-/blob/Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js?L176-186

This could be addressed by finally turning `WI.DetailsSection` and `WI.DetailsSectionRow` into views that participate in the view tree. Right now they extend `WI.Object`.
See: https://bugs.webkit.org/show_bug.cgi?id=152269
We could detach nested views when collapsed and that would prevent layout of children. But this needs testing to ensure other consumers work well.

I'd like to handle this in a follow-up. Not a bug, but a needless perf waste.

I have to land the current patch this week.

[Razvan Caliman]

Created attachment 446558 [details]
Patch 1.4

Address code review feedback

[Razvan Caliman]

<rdar://82978905>

[Patrick Angle]

Comment on attachment 446558 [details]
Patch 1.4

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

> Source/WebInspectorUI/ChangeLog:71
> +        Generalize handling collpsed state change events for all sections. current and future.

NIT: s/collpsed/collapsed
NIT: s/sections. current/sections, current

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:151
> +    get variablesStyleGroupedByType()
> +    {
> +        if (this._variableStyleForType)
> +            return this._variableStyleForType;

Can we name the underlying variable to match the getter or vice-versa?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:190
> +            if (variablesForType[type]?.length) {
> +                const ownerStyleSheet = null;
> +                const id = null;
> +                const inherited = false;
> +                const text = null;
> +                let style = new WI.CSSStyleDeclaration(this, ownerStyleSheet, id, WI.CSSStyleDeclaration.Type.Computed, this._node, inherited, text, variablesForType[type]);
> +                this._variableStyleForType.set(type, style);
> +            }

NIT: This would be cleaner as an early-continue instead of nesting deeper here.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:44
> +        console.assert(Object.values(WI.DOMNodeStyles.VariablesGroupType).includes(variablesGroupType), variablesGroupType);

This assertion is almost entirely redundant with the one below on :60. I'd personally opt to keep that one and drop this assertion since the one on :60 catches both invalid input as well as forgetting to add a case for a new group type.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:60
> +        console.assert(false, "not reached");

NIT: s/not reached/Unknown group type

Also, we should include the `variablesGroupType` as a third parameter here to help someone determine later what value isn't valid.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:213
> +            // WI.ComputedStyleSection is a view but its element is attached to a WI.DetailsSection which isn't. Reparent it before removing.
> +            // FIXME: <https://webkit.org/b/152269>

NIT: including the bug title make it easier to understand what would need fixed here and would help explain the first line of comment here as well.

> LayoutTests/ChangeLog:12
> +        * inspector/css/getVariablesStyleGroupedByType-expected.txt: Added.
> +        * inspector/css/getVariablesStyleGroupedByType.html: Added.

These names (and the names within the test) should probably match whatever function name you end up using above for DOMNodeStyles.js:148-151

[Devin Rousso]

Comment on attachment 446558 [details]
Patch 1.4

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

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:64
> +    font-weight: normal;

I think we may also want to adjust some positioning, as in attachment 445305 [details] it looks like this isn't vertically centered.

Aside: I wonder if maybe we should adjust the color of the `WI.ScopeBar` so that it doesn't visually "pop" as much.  We definitely want it to be visible/noticeable, but not _too_ much.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:181
> +        let createVariablesGroupingModeScopeBarItem = (mode, label) => {

NIT: this doesn't need to be an arrow function
NIT: does this even need to exist anymore?
```
    const variablesGroupingModeScopeBarItems = [
        new WI.ScopeBarItem(WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped, WI.UIString("Ungrouped", "Ungrouped @ Computed Style variables grouping mode", "Label for button to show CSS variables ungrouped")),
        new WI.ScopeBarItem(WI.ComputedStyleDetailsPanel.VariablesGroupingMode.ByType, WI.UIString("By Type", "By Type @ Computed Style variables grouping mode", "Label for button to show CSS variables grouped by type")),
    ];
```

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:-183
> -            this._variablesTextEditor.needsLayout();

I need to apologize.  My last comment suggested that you should remove the `_handleDetailsSectionCollapsedStateChanged`.  I think there is still a need for it, as when the `WI.DetailsSection` is collapsed, any `WI.View` children will be unable to get sizing information (e.g. `this.element.realOffsetWidth`), meaning that when it is finally expanded we should do a `layout` so that we recalculate anything like that.  I don't think anything in `WI.ComputedStyleSection` actually uses that, but it may be worth doing the `needsLayout` just in case.  My comment was intended to be more of a "we should figure out a way to make sure we don't do any `layout` when `collapsed`.  again, my apologies for the confusion.

[Razvan Caliman]

Comment on attachment 446558 [details]
Patch 1.4

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

>> Source/WebInspectorUI/ChangeLog:71
>> +        Generalize handling collpsed state change events for all sections. current and future.
> 
> NIT: s/collpsed/collapsed
> NIT: s/sections. current/sections, current

Done

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:151
>> +            return this._variableStyleForType;
> 
> Can we name the underlying variable to match the getter or vice-versa?

Renamed this before following code review feedback. I agree it's not pretty. It's trying to convey many things about many things.

Can't use "*ForType" at the call site because it implies a type argument may be passed.

I renamed it to `_variableStylesByType` and `get variableStylesByType()` correspondingly. 
Hope this sticks. Updated test file names as well.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:190
>> +            }
> 
> NIT: This would be cleaner as an early-continue instead of nesting deeper here.

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:64
>> +    font-weight: normal;
> 
> I think we may also want to adjust some positioning, as in attachment 445305 [details] it looks like this isn't vertically centered.
> 
> Aside: I wonder if maybe we should adjust the color of the `WI.ScopeBar` so that it doesn't visually "pop" as much.  We definitely want it to be visible/noticeable, but not _too_ much.

> it looks like this isn't vertically centered.
Done.

> Aside: I wonder if maybe we should adjust the color of the `WI.ScopeBar` so that it doesn't visually "pop" as much.
The `WI.ScopeBar` is using the system accent color. This is nice for consistency. But I agree the default is strident in this context.

Taking this offline to discuss on chat and share some options. I think we can land it as-is and tweak it afterwards.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:44
>> +        console.assert(Object.values(WI.DOMNodeStyles.VariablesGroupType).includes(variablesGroupType), variablesGroupType);
> 
> This assertion is almost entirely redundant with the one below on :60. I'd personally opt to keep that one and drop this assertion since the one on :60 catches both invalid input as well as forgetting to add a case for a new group type.

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:60
>> +        console.assert(false, "not reached");
> 
> NIT: s/not reached/Unknown group type
> 
> Also, we should include the `variablesGroupType` as a third parameter here to help someone determine later what value isn't valid.

Done.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:181
>> +        let createVariablesGroupingModeScopeBarItem = (mode, label) => {
> 
> NIT: this doesn't need to be an arrow function
> NIT: does this even need to exist anymore?
> ```
>     const variablesGroupingModeScopeBarItems = [
>         new WI.ScopeBarItem(WI.ComputedStyleDetailsPanel.VariablesGroupingMode.Ungrouped, WI.UIString("Ungrouped", "Ungrouped @ Computed Style variables grouping mode", "Label for button to show CSS variables ungrouped")),
>         new WI.ScopeBarItem(WI.ComputedStyleDetailsPanel.VariablesGroupingMode.ByType, WI.UIString("By Type", "By Type @ Computed Style variables grouping mode", "Label for button to show CSS variables grouped by type")),
>     ];
> ```

Yes, this can be reduced. The method body used to do more.
Done.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:213
>> +            // FIXME: <https://webkit.org/b/152269>
> 
> NIT: including the bug title make it easier to understand what would need fixed here and would help explain the first line of comment here as well.

Done

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:-183
>> -            this._variablesTextEditor.needsLayout();
> 
> I need to apologize.  My last comment suggested that you should remove the `_handleDetailsSectionCollapsedStateChanged`.  I think there is still a need for it, as when the `WI.DetailsSection` is collapsed, any `WI.View` children will be unable to get sizing information (e.g. `this.element.realOffsetWidth`), meaning that when it is finally expanded we should do a `layout` so that we recalculate anything like that.  I don't think anything in `WI.ComputedStyleSection` actually uses that, but it may be worth doing the `needsLayout` just in case.  My comment was intended to be more of a "we should figure out a way to make sure we don't do any `layout` when `collapsed`.  again, my apologies for the confusion.

Ok. I readded the handler as it was.
The aim is still to handle needless layouts in a follow-up by turning `WI.DetailsSection` into a `WI.View` to solve this issue holistically.

>> LayoutTests/ChangeLog:12
>> +        * inspector/css/getVariablesStyleGroupedByType.html: Added.
> 
> These names (and the names within the test) should probably match whatever function name you end up using above for DOMNodeStyles.js:148-151

Done. Landed on `variableStylesByType`

[Razvan Caliman]

Created attachment 446718 [details]
Patch 1.5

Address code review feedback

[Patrick Angle]

Comment on attachment 446718 [details]
Patch 1.5

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

r=me

> LayoutTests/inspector/css/variableStylesByType.html:23
> +                InspectorTest.assert(cssStyles, `Should find CSS Styles for DOM Node.`);

As Devin mentioned on bug 233372, `styleForNode` is guaranteed to return a `WI.DOMNodeStyles` so this assertion is not necessary.

> LayoutTests/inspector/css/variableStylesByType.html:46
> +        ]

Style/NIT: Missing trailing comma

> LayoutTests/inspector/css/variableStylesByType.html:58
> +        ]

Ditto :46

> LayoutTests/inspector/css/variableStylesByType.html:70
> +        ]

Ditto :46

> LayoutTests/inspector/css/variableStylesByType.html:82
> +        ]

Ditto :46

> LayoutTests/inspector/css/variableStylesByType.html:106
> +        ]

Ditto :46

[Devin Rousso]

Comment on attachment 446718 [details]
Patch 1.5

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

r=me as well, nice work!

I agree, the color adjustments can be made in a followup :)

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:159
> +        const typeCheckFunctions = [

NIT: I just realized that this probably should be an array of objects, not an array of arrays.  e.g.
```
    const typeCheckFunctions = [
        {
            type: WI.DOMNodeStyles.VariablesGroupType.Colors,
            checker(property) {
                return WI.Color.fromString(property.value);
            },
        },
        ...
    ];
```

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:64
> +    margin-top: -1px;

Can we do `vertical-align: -1px;` or something like that?  We usually prefer that to negative positioning.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:209
> +            // WI.ComputedStyleSection is a view but its element is attached to a WI.DetailsSection which isn't. Reparent it before removing.

What actually removes the `WI.DetailsSection` from the DOM?  I thought `collapsed` just applies a CSS class with `display: none`?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:213
> +            styleSection.element.remove();

Is this actually needed?  I thought `WI.View.prototype.removeSubview` also handled `subview.element.remove()`?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:297
> +        if (section)
> +            section.element.classList.toggle("hidden", !event.data.matches);

```
    section?.element.classList.toggle("hidden", !event.data.matches);
```

>> LayoutTests/inspector/css/variableStylesByType.html:46
>> +        ]
> 
> Style/NIT: Missing trailing comma

and after the `}` on the line above :)

[Razvan Caliman]

Comment on attachment 446718 [details]
Patch 1.5

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:159
>> +        const typeCheckFunctions = [
> 
> NIT: I just realized that this probably should be an array of objects, not an array of arrays.  e.g.
> ```
>     const typeCheckFunctions = [
>         {
>             type: WI.DOMNodeStyles.VariablesGroupType.Colors,
>             checker(property) {
>                 return WI.Color.fromString(property.value);
>             },
>         },
>         ...
>     ];
> ```

DOne

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:64
>> +    margin-top: -1px;
> 
> Can we do `vertical-align: -1px;` or something like that?  We usually prefer that to negative positioning.

Has no effect in this context and I don't understand why.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:209
>> +            // WI.ComputedStyleSection is a view but its element is attached to a WI.DetailsSection which isn't. Reparent it before removing.
> 
> What actually removes the `WI.DetailsSection` from the DOM?  I thought `collapsed` just applies a CSS class with `display: none`?

See Line 219
```
detailsSection.element.remove();
```

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:213
>> +            styleSection.element.remove();
> 
> Is this actually needed?  I thought `WI.View.prototype.removeSubview` also handled `subview.element.remove()`?

I expected so too, but I see leftover elements in absence of manually cleaning-up, for example on page refresh.
Moving the element around doesn't play well with `addSubview()` / `removeSubview()`. Some assertions that should not be hit are, even after reparenting.
I gave up fighting it.

It's a hack because `WI.DetailsSection` isn't a view but we need the element to show up inside it.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:297
>> +            section.element.classList.toggle("hidden", !event.data.matches);
> 
> ```
>     section?.element.classList.toggle("hidden", !event.data.matches);
> ```

Done

>> LayoutTests/inspector/css/variableStylesByType.html:23
>> +                InspectorTest.assert(cssStyles, `Should find CSS Styles for DOM Node.`);
> 
> As Devin mentioned on bug 233372, `styleForNode` is guaranteed to return a `WI.DOMNodeStyles` so this assertion is not necessary.

Done

>>> LayoutTests/inspector/css/variableStylesByType.html:46
>>> +        ]
>> 
>> Style/NIT: Missing trailing comma
> 
> and after the `}` on the line above :)

Done 'em all

[Razvan Caliman]

Created attachment 446760 [details]
Patch 1.6

Carry over R+; Address last nits

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Razvan Caliman]

Created attachment 446800 [details]
Patch 1.6

Carry over R+; Forgot to add names of reviewers to Changelog in previous patch

[EWS]

Committed r286876 (245106@main): <https://commits.webkit.org/245106@main>

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