Bug 200419

Summary: Web Inspector: Styles: show @supports CSS groupings
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=199946
Bug Depends on: 200488    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Patch
none
Patch
none
Patch
none
Patch none

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.