Bug 233208 – Web Inspector: Support Cascade Layers in the Styles sidebar

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

Currently styles inside a @layer rule will not be shown in the Styles sidebar, which makes it difficult to debug and modify these styles. We should at least show these styles and their corresponding layer name like we do for other nested rules like @media and @supports. Styles that are in a layer as a result of an @import rule current are displayed, but will not be attributed to a layer, which could be misleading.

[Radar WebKit Bug Importer]

<rdar://problem/85469910>

[Patrick Angle]

Created attachment 444421 [details]
WIP v0.1 - Needs tests

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Patrick Angle]

Created attachment 444474 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 444474 [details]
Patch v1.0

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

Neat stuff!  Mostly style comments :)

Can you add a screenshot of what this would look like?  I have a mental picture, and I'm curious if it matches my expectation :P

> Source/WebCore/css/CSSImportRule.h:40
> +    WEBCORE_EXPORT const std::optional<Vector<AtomString>>& cascadeLayerName() const;

You should use `std::optional<CascadeLayerName>&` to match other `cascadeLayerName` methods (including the implementation of this).

> Source/WebCore/css/CSSLayerRule.cpp:66
> +    result.append("@layer ", layerName(), " {\n");

Based on the above, the space is only added if the layer has a name.  Not sure if that matters, but it's a slight difference.

> Source/WebCore/css/CSSLayerRule.cpp:76
> +    if (!layer.isStatement() && !layer.name().isEmpty())

Are you sure that the `isStatement` check is necessary?

> Source/WebCore/css/StyleRule.cpp:520
> +String StyleRuleLayer::stringFromCascadeLayerName(CascadeLayerName name)

Can we `const CascadeLayerName&`?

> Source/WebCore/css/StyleRule.h:306
> +    static String stringFromCascadeLayerName(CascadeLayerName);

It feels a bit odd for this to be on `StyleRuleLayer` instead of `CSSLayerRule` since that's primarily where it's used.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:112
> +            || data->type == WebCore::StyleRuleType::Supports
> +            || data->type == WebCore::StyleRuleType::Layer) {

NIT: It may be correct according to the styleguide to do this, but I kinda feel like it's not _that_ long of a line to have on one line, and IMO would read better that way.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:444
> +        Vector<RefPtr<Protocol::CSS::Grouping>> ruleGroupingPayloads;

I think this can be `Vector<Ref<Inspector::Protocol::CSS::Grouping>>` now that we don't have to worry about default-initializing a `Ref`.  You'd also probably wanna add some `WTFMove` in the `for` below too.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:453
> +                );

I can only find one other case kinda like this, but I feel like our "normal" style would be to have this on the previous line.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:480
> +                .setText(downcast<CSSLayerRule>(parentRule)->layerName())

What do/should we do if this is an empty string?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:486
> +        for (auto ruleGroupingPayload : ruleGroupingPayloads) {

`auto&` or `auto&&`

> Source/WebInspectorUI/UserInterface/Models/CSSGrouping.js:79
> +    LayerRule: "layer-rule",
> +    LayerImportRule: "layer-import-rule",

NIT: While I am in favor of making this alphabetical, it currently isn't, so I'd either make it alphabetical or move these to the end since it's the most recently added thing.  The latter would also match the order in the protocol too.

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:24
> +        InspectorTest.expectEqual(rule.groupings.length, groupings.length, `Rule should have ${groupings.length} grouping${groupings.length != 1 ? "s" : ""}.`);

`!==`

also i applaud you trying to get the right singular vs plural, but i dont think it _really_ matters that much for a test :P

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:30
> +            if (groupings[i].text != null)

`!==`

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:47
> +                InspectorTest.assert(domNodeStyles, `Should find CSS Styles for DOM Node.`);

This assert is kinda pointless since `stylesForNode` is guaranteed to return a `WI.DOMNodeStyles` 😅

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:59
> +        description: "A list item should have both the User Agent and authored `::marker` rules.",

oops?

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:74
> +                    {type: WI.CSSGrouping.Type.SupportsRule},

Shouldn't this have `text: "color: red"`?

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:105
> +                    {type: WI.CSSGrouping.Type.SupportsRule},

ditto (:74)

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:122
> +                    {type: WI.CSSGrouping.Type.SupportsRule},

ditto (:74)

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:154
> +                    {type: WI.CSSGrouping.Type.LayerRule, text: ""},

I think this will cause issues in `WI.CSSGrouping` as it assumes that the `text` must not be empty.  It may also be worth adjusting the protocol so that `text` is `"optional": true` so that we can just omit it entirely if there's no value for it.

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:163
> +                    {type: WI.CSSGrouping.Type.SupportsRule},

ditto (:74)

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:176
> +                colorPropertyValue: "lemonchiffon",

For my own knowledge, why is this last when it was the first one declared?  Is it because it's before the `@layer imported, base, special`?

[Patrick Angle]

Comment on attachment 444474 [details]
Patch v1.0

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

I'll upload a screenshot as well - Keep in mind while viewing the screenshot that I've opened bug 233238 to deal with the fact that the order of groupings per rule is backwards of what one would expect to see (outermost @layer/@supports/@media rule first).

>> Source/WebCore/css/CSSImportRule.h:40
>> +    WEBCORE_EXPORT const std::optional<Vector<AtomString>>& cascadeLayerName() const;
> 
> You should use `std::optional<CascadeLayerName>&` to match other `cascadeLayerName` methods (including the implementation of this).

Oops, yeah. I'm pretty sure I did this so I could keep working because importing StyleRule.h here to get `CascadeLayerName` caused build issues with WebKitLegacy. I'll revisit that.

>> Source/WebCore/css/CSSLayerRule.cpp:66
>> +    result.append("@layer ", layerName(), " {\n");
> 
> Based on the above, the space is only added if the layer has a name.  Not sure if that matters, but it's a slight difference.

Good catch - Looks like a WPT test also caught this, so this is clearly not correct.

>> Source/WebCore/css/CSSLayerRule.cpp:76
>> +    if (!layer.isStatement() && !layer.name().isEmpty())
> 
> Are you sure that the `isStatement` check is necessary?

The layer name is actually a variant which can be either a `Vector<CascadeLayerName>` or just a `CascadeLayerName` - checking `!isStatement` lets us know if it's actually a `CascadeLayerName` or not.

>> Source/WebCore/css/StyleRule.h:306
>> +    static String stringFromCascadeLayerName(CascadeLayerName);
> 
> It feels a bit odd for this to be on `StyleRuleLayer` instead of `CSSLayerRule` since that's primarily where it's used.

I put this here to be in the same file as the definition of `CascadeLayerName`, but I'm happy to move it.

>> Source/WebCore/inspector/InspectorStyleSheet.cpp:480
>> +                .setText(downcast<CSSLayerRule>(parentRule)->layerName())
> 
> What do/should we do if this is an empty string?

In my view an empty string is a valid value here - it means that the layer is it's own anonymous, unnamed, layer. It means the frontend will display a grouping of `@layer` with no name.

>> Source/WebInspectorUI/UserInterface/Models/CSSGrouping.js:79
>> +    LayerImportRule: "layer-import-rule",
> 
> NIT: While I am in favor of making this alphabetical, it currently isn't, so I'd either make it alphabetical or move these to the end since it's the most recently added thing.  The latter would also match the order in the protocol too.

Oops, though it was alphabetical, but clearly I didn't look deeper than the first character of any of the existing values.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:30
>> +            if (groupings[i].text != null)
> 
> `!==`

I actually purposefully used abstract equality here (and tried to note in the comment above that we are checking for nullish values) in place of a more complex check like `typeof(groupings[i].text) !== "undefined" && groupings[i].text !== null`. I suppose given that defining the text and making it null in the provided expectations isn't done anywhere, I could just do the first half of the more verbose check. This logic will need to change anyways to accommodate expecting an undefined/null `text` value on the rule's grouping anyways.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:74
>> +                    {type: WI.CSSGrouping.Type.SupportsRule},
> 
> Shouldn't this have `text: "color: red"`?

I skipped checking that since it was kinda tangential to the actual subject of these tests, but I'm not opposed to checking it either.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:154
>> +                    {type: WI.CSSGrouping.Type.LayerRule, text: ""},
> 
> I think this will cause issues in `WI.CSSGrouping` as it assumes that the `text` must not be empty.  It may also be worth adjusting the protocol so that `text` is `"optional": true` so that we can just omit it entirely if there's no value for it.

I think you are right that the correct thing to do is make text optional, since an anonymous layer will not have any text. An optional is probably cleaner than using an empty string to represent this. I'm not sure how I managed to avoid hitting that assertion in `WI.CSSGrouping` during testing...

>> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:176
>> +                colorPropertyValue: "lemonchiffon",
> 
> For my own knowledge, why is this last when it was the first one declared?  Is it because it's before the `@layer imported, base, special`?

Yeah, so layers are stacked in the order of first declaration, with each new layer being placed on top of previously declared layers (reusing a layer name just adds to an already declared layer), so in this case the first anonymous layer (lemonchiffon) is declared before base or special, so it is the lowest layer, followed by base, then special, then the very last anonymous layer declared (darkviolet), and then the final special layer that contains all the styles in the implicit outer layer, which get applied on top of any named layer. (https://www.w3.org/TR/css-cascade-5/#layer-ordering)

[Patrick Angle]

Created attachment 444526 [details]
Screenshot of Patch v1.0

[Patrick Angle]

Created attachment 444587 [details]
Patch v1.1 - Address Devin's review comments

[Patrick Angle]

The new test cases will continue to fail under debug builds until Bug 233283 ([CSS Cascade Layers] [Debug] ASSERTION FAILED: m_childRules.isEmpty() when using @import with layer name) is addressed.

[Devin Rousso]

Comment on attachment 444587 [details]
Patch v1.1 - Address Devin's review comments

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

r=me, with a few suggestions/changes.  Nice work!  :)

> Source/JavaScriptCore/inspector/protocol/CSS.json:231
> +                { "name": "text", "type": "string", "optional": true, "description": "Media query text." },

NIT: I dunno if this is actually a requirement/rule, but I think non-optional things should be before optional things, so I'd move this below `type`.

Aside: the `description` for this is also wrong 😅

> Source/WebCore/css/CSSLayerRule.cpp:80
> +        return std::optional<String>(stringFromCascadeLayerName(layer.name()));

Is the `std::optional<String>` actually needed?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:484
> +        for (auto&& ruleGroupingPayload : ruleGroupingPayloads) {

Does this also need a `WTFMove(ruleGroupingPayloads)`?

> Source/WebInspectorUI/UserInterface/Models/CSSGrouping.js:35
> +        this._text = text || null;

We probably need to add checks anywhere this is used so that we don't suddenly end up with a "null" somewhere in the UI 😅

AFAICT there's only two usages:
- `WI.CSSStyleDeclaration.prototype.generateFormattedText`
- `WI.SpreadsheetCSSStyleDeclarationSection.prototype.initialLayout`

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:32
> +                InspectorTest.expectFalse(rule.groupings[i].text, `Grouping ${i} should not have any text.`);

NIT: I think `expectNull` would be more technically correct.

[Patrick Angle]

Looks like I need to rebaseline some tests to deal with the order of properties of a CSS.Group changing. Also, I've identified some changes that need to be made in `SpreadsheetCSSStyleDeclarationSection.prototype.initialLayout` to make sure that @layer's with a name of `all` are not ignored and that we use a `.` to separate back-to-back layer names. Since I'll already be nearby, we should also reverse the order of groups in this patch so that the layer name strings are correctly concatenated.

[Patrick Angle]

Created attachment 445883 [details]
Patch v1.2

[Patrick Angle]

Comment on attachment 445883 [details]
Patch v1.2

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

GTK build continues to fail - looks like there is still a discrepancy between what are marked as private headers for Cocoa platforms and what is marked as private for other platforms.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:484
> +                styleText +=- " {";

Oops. `+=-`

[Patrick Angle]

Created attachment 445909 [details]
Patch v1.2.1 - GTK build fix

[Patrick Angle]

Created attachment 445927 [details]
Patch v1.2.2 - GTK build fix

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Patrick Angle]

Created attachment 446047 [details]
Patch v1.2.3 - Fix missing reviewer in changelog

[EWS]

Committed r286558 (244888@main): <https://commits.webkit.org/244888@main>

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