Web Inspector: Styles: show @supports CSS groupings

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

We show `@media`, but not `@supports`, even though they can be used in a similar way (change styles based on some "capability").

[Devin Rousso]

Created attachment 375494 [details]
Patch

[Devin Rousso]

Created attachment 375495 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

Comment on attachment 375494 [details]
Patch

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

r=me! Nice!

> Source/JavaScriptCore/inspector/protocol/CSS.json:224
> -            "id": "CSSMedia",
> +            "id": "Grouping",

I'm not sure `Grouping` is clear but I'm warming up to it.

I considered `AtRuleQuery`. Since these primarily match a media query or feature query features, with <link media="..."> / <style media="..."> special cases.
https://developer.mozilla.org/en-US/docs/Web/CSS/At-rule

I suspect other at-rule features like `@page {...}`, `@font-face {...}` and `@keyframes {...}` will require their own types.

> Source/JavaScriptCore/inspector/protocol/CSS.json:226
> +            "description": "CSS @media/@supports descriptor.",

This says "@media/@supports" but has a longer list of supported things (@media, @supports, @import, and synthesized from <link media="..."> / <style media="...">).

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:458
> +        let groupings = this.groupings.filter((grouping) => grouping.text !== "all");

Does the frontend filter out media queries of `all`? Maybe then the backend shouldn't filter them itself?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:109
> +            this._groupingElements = groupings.map((grouping, i) => {

Nit: This doesn't make use of `i`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:118
> +                if (groupingTypeElement.children.length)
> +                    groupingTypeElement.append(", ");

This just blew my mind. I did not know that `d = document.createElement("div"); d.textContent = "foo"; d.children.length` is zero. I guess `children` is not `childNodes`.

Maybe using `groupingTypeElement.childElementCount` might be better? I'd probably have carried a boolean around to say whether or not we're appending more to the currentGroupingType.

[Devin Rousso]

Comment on attachment 375494 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/CSS.json:224
>> +            "id": "Grouping",
> 
> I'm not sure `Grouping` is clear but I'm warming up to it.
> 
> I considered `AtRuleQuery`. Since these primarily match a media query or feature query features, with <link media="..."> / <style media="..."> special cases.
> https://developer.mozilla.org/en-US/docs/Web/CSS/At-rule
> 
> I suspect other at-rule features like `@page {...}`, `@font-face {...}` and `@keyframes {...}` will require their own types.

I like grouping because it's "so general" that it can be reused for other things.  If we wanted to add `@page`, this could also be used, although we'd need to do something else for how to represent a rule that doesn't have a selector (the `@page` is the selector).  I'd imagine a `GroupedProperties` object that had a `Grouping` and `Array<CSSProperty>`.

>> Source/JavaScriptCore/inspector/protocol/CSS.json:226
>> +            "description": "CSS @media/@supports descriptor.",
> 
> This says "@media/@supports" but has a longer list of supported things (@media, @supports, @import, and synthesized from <link media="..."> / <style media="...">).

The only case that `@imports` and the `<... media="...">` apply is when they're effectively like a media query, so I considered them to be "part" of `@media`, but I can clarify that.

>> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:458
>> +        let groupings = this.groupings.filter((grouping) => grouping.text !== "all");
> 
> Does the frontend filter out media queries of `all`? Maybe then the backend shouldn't filter them itself?

Yes, this is primarily to handler older backends that don't already filter out "all".  Every rule automatically has an "all" media, so there's no reason to include them (they also don't really mean anything, as they say "this applies everywhere").

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:109
>> +            this._groupingElements = groupings.map((grouping, i) => {
> 
> Nit: This doesn't make use of `i`

Ah, oops :P

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:118
>> +                    groupingTypeElement.append(", ");
> 
> This just blew my mind. I did not know that `d = document.createElement("div"); d.textContent = "foo"; d.children.length` is zero. I guess `children` is not `childNodes`.
> 
> Maybe using `groupingTypeElement.childElementCount` might be better? I'd probably have carried a boolean around to say whether or not we're appending more to the currentGroupingType.

`children` only counts `Element`s, whereas `childNodes` counts `Node`s.  It was actually my intention to use `children`, as I want to add ", " only after one of the below `<span>` has been added, but not after the `grouping.prefix`.  I'll add a comment.

[Devin Rousso]

Created attachment 375599 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375599 [details]
Patch

Clearing flags on attachment: 375599

Committed r248289: <https://trac.webkit.org/changeset/248289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53971948>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 200488

[Devin Rousso]

Created attachment 375886 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 375886 [details]
Patch

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

> Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_header.py:74
> +        headerPostlude_args = {
> +            'includes': '\n'.join(['#import ' + header for header in sorted(headerPreludeHeaders)]),
>          }

This should use `sorted(headerPostludeHeaders)` not `sorted(headerPreludeHeaders)`.

[Devin Rousso]

Created attachment 375896 [details]
Patch

[Devin Rousso]

Comment on attachment 375886 [details]
Patch

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

>> Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_header.py:74
>>          }
> 
> This should use `sorted(headerPostludeHeaders)` not `sorted(headerPreludeHeaders)`.

Oops :P

[Devin Rousso]

*** Bug 200676 has been marked as a duplicate of this bug. ***

[Devin Rousso]

Created attachment 376173 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376173 [details]
Patch

Clearing flags on attachment: 376173

Committed r248602: <https://trac.webkit.org/changeset/248602>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.