Bug 238346 – Web Inspector: Support Container Queries in the Styles sidebar

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

[Patrick Angle]

After we fix bug 238338 we still need to actually show rules wrapped in a container query in the Styles sidebar. This is complicated by the lack of a CSSOM implementation currently for container queries, so I'm splitting this into its own bug to seperate the two issues.

[Radar WebKit Bug Importer]

<rdar://problem/90787978>

[Patrick Angle]

Created attachment 455976 [details]
Patch v1.0

[EWS Watchlist]

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

[Antti Koivisto]

Comment on attachment 455976 [details]
Patch v1.0

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

> Source/WebCore/css/CSSContainerRule.h:33
> +class CSSContainerRule final : public CSSConditionRule {

I'm adding CSSOM support in https://bugs.webkit.org/show_bug.cgi?id=238500. Lets land that first.

[Antti Koivisto]

Comment on attachment 455976 [details]
Patch v1.0

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

> Source/WebCore/css/ContainerQueryParser.cpp:41
> +    auto authoredCondition = range.serialize().stripWhiteSpace();

Serialization is expected to be canonical so this wouldn't work correctly.

[Patrick Angle]

Comment on attachment 455976 [details]
Patch v1.0

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

>> Source/WebCore/css/CSSContainerRule.h:33
>> +class CSSContainerRule final : public CSSConditionRule {
> 
> I'm adding CSSOM support in https://bugs.webkit.org/show_bug.cgi?id=238500. Lets land that first.

Oh cool! I’ll hold this until that lands and rebase on top of it. Thanks!

[Patrick Angle]

Comment on attachment 455976 [details]
Patch v1.0

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

>> Source/WebCore/css/ContainerQueryParser.cpp:41
>> +    auto authoredCondition = range.serialize().stripWhiteSpace();
> 
> Serialization is expected to be canonical so this wouldn't work correctly.

I think `authoredCondition` is a bad name for what I intended this to be. I needed a non-simplified form of the query to use as the `conditionText`, since the spec says logical substitutions aren't allowed, which I took to mean that `(max-width: 100px)` should not become `(width <= 100px)` which is what just serializing the existing computed `FilteredContainerQuery` would do as far as I can tell.

[Antti Koivisto]

(In reply to Patrick Angle from comment #7)
> Comment on attachment 455976 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455976&action=review
> 
> >> Source/WebCore/css/ContainerQueryParser.cpp:41
> >> +    auto authoredCondition = range.serialize().stripWhiteSpace();
> > 
> > Serialization is expected to be canonical so this wouldn't work correctly.
> 
> I think `authoredCondition` is a bad name for what I intended this to be. I
> needed a non-simplified form of the query to use as the `conditionText`,
> since the spec says logical substitutions aren't allowed, which I took to
> mean that `(max-width: 100px)` should not become `(width <= 100px)` which is
> what just serializing the existing computed `FilteredContainerQuery` would
> do as far as I can tell.

Whitespace needs to be normalized, etc, that's cover by a WPT too. It can't really be done without serialising from the parsed structure.

Good point about max-width: vs <=. We should probably remember the difference. Need to expand the WPT to cover that.

[Patrick Angle]

Created attachment 456050 [details]
Patch v1.1 - Rebase on CSSContainerRule implementation

[Devin Rousso]

Comment on attachment 456050 [details]
Patch v1.1 - Rebase on CSSContainerRule implementation

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

r=me, awesomeness =D

Could you please upload a screenshot of this change?  I'm 99% sure I know what this will look like, but it'd be nice to know for sure (plus we may wanna have some extra styling, e.g. maybe we emphasize the container name?).

> Source/WebCore/inspector/InspectorStyleSheet.cpp:134
> +            // These rule types do not contain child rules that are displayed in Web Inspector's Style Details Sidebar for elements.

NIT: this sentence is a bit awkward to read
```
// These rule types do not contain child rules, and therefore have nothing to display in the Styles panel in the details sidebar of the Elements Tab in Web Inspector.
```

> LayoutTests/inspector/css/getMatchedStylesForNodeContainerGrouping.html:155
> +    @container notUsedName (width >= 200px) {

NIT: Should we try some other conditions?  Maybe some style containers (assuming we support them)?

[Patrick Angle]

Comment on attachment 456050 [details]
Patch v1.1 - Rebase on CSSContainerRule implementation

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

Attaching a screenshot as well. Currently no special styling is applied to any part of the grouping text, which while consistent with other group types (e.g. `@media`, `@supports`, `@layer`) is not great. We should probably be tokenizing and coloring all of these different groupings, but would prefer to defer that to a followup to tackle soon™.

>> LayoutTests/inspector/css/getMatchedStylesForNodeContainerGrouping.html:155
>> +    @container notUsedName (width >= 200px) {
> 
> NIT: Should we try some other conditions?  Maybe some style containers (assuming we support them)?

We don't actually support style containers yet, so those rules aren't associated with any element yet (they are created internally as `WebCore::CQ::UnknownQuery`).

[Patrick Angle]

Created attachment 456064 [details]
Patch v1.2 - Review nit

[Patrick Angle]

Created attachment 456065 [details]
Screenshot of Patch 1.2

[Patrick Angle]

Created attachment 456253 [details]
[fast-cq] Patch v1.3 - Rebased

[Ryan Haddad]

Marked fast-cq due to to test results only showing pre-existing test failures.

[EWS]

Committed r292181 (249084@main): <https://commits.webkit.org/249084@main>

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