Bug 200419 - Web Inspector: Styles: show @supports CSS groupings
Summary: Web Inspector: Styles: show @supports CSS groupings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
: 200676 (view as bug list)
Depends on: 200488
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-03 11:30 PDT by Devin Rousso
Modified: 2019-08-13 11:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (56.08 KB, patch)
2019-08-03 19:17 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (122.31 KB, image/png)
2019-08-03 19:17 PDT, Devin Rousso
no flags Details
Patch (56.08 KB, patch)
2019-08-05 21:21 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (59.60 KB, patch)
2019-08-08 20:34 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (59.60 KB, patch)
2019-08-08 23:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (59.49 KB, patch)
2019-08-13 09:30 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-08-03 11:30:18 PDT
We show `@media`, but not `@supports`, even though they can be used in a similar way (change styles based on some "capability").
Comment 1 Devin Rousso 2019-08-03 19:17:05 PDT
Created attachment 375494 [details]
Patch
Comment 2 Devin Rousso 2019-08-03 19:17:17 PDT
Created attachment 375495 [details]
[Image] After Patch is applied
Comment 3 Joseph Pecoraro 2019-08-05 19:42:37 PDT
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.
Comment 4 Devin Rousso 2019-08-05 21:15:14 PDT
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.
Comment 5 Devin Rousso 2019-08-05 21:21:47 PDT
Created attachment 375599 [details]
Patch
Comment 6 WebKit Commit Bot 2019-08-05 22:04:22 PDT
Comment on attachment 375599 [details]
Patch

Clearing flags on attachment: 375599

Committed r248289: <https://trac.webkit.org/changeset/248289>
Comment 7 WebKit Commit Bot 2019-08-05 22:04:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-08-05 22:05:17 PDT
<rdar://problem/53971948>
Comment 9 WebKit Commit Bot 2019-08-06 16:33:34 PDT
Re-opened since this is blocked by bug 200488
Comment 10 Devin Rousso 2019-08-08 20:34:38 PDT
Created attachment 375886 [details]
Patch
Comment 11 Joseph Pecoraro 2019-08-08 22:46:22 PDT
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)`.
Comment 12 Devin Rousso 2019-08-08 23:53:12 PDT
Created attachment 375896 [details]
Patch
Comment 13 Devin Rousso 2019-08-08 23:53:32 PDT
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
Comment 14 Devin Rousso 2019-08-13 09:28:07 PDT
*** Bug 200676 has been marked as a duplicate of this bug. ***
Comment 15 Devin Rousso 2019-08-13 09:30:34 PDT
Created attachment 376173 [details]
Patch
Comment 16 WebKit Commit Bot 2019-08-13 11:28:21 PDT
Comment on attachment 376173 [details]
Patch

Clearing flags on attachment: 376173

Committed r248602: <https://trac.webkit.org/changeset/248602>
Comment 17 WebKit Commit Bot 2019-08-13 11:28:23 PDT
All reviewed patches have been landed.  Closing bug.