Bug 176289 - Web Inspector: Styles Redesign: display @media section headers
Summary: Web Inspector: Styles Redesign: display @media section headers
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on: 175343
Blocks: 176640 176641
  Show dependency treegraph
 
Reported: 2017-09-02 19:58 PDT by Nikita Vasilyev
Modified: 2017-09-13 16:20 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.74 KB, patch)
2017-09-08 17:59 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Screenshot] With patch applied (223.68 KB, image/png)
2017-09-08 18:14 PDT, Nikita Vasilyev
no flags Details
Patch (3.72 KB, patch)
2017-09-11 23:40 PDT, Nikita Vasilyev
hi: review+
Details | Formatted Diff | Diff
[Image] Before/after (310.52 KB, image/png)
2017-09-12 20:36 PDT, Nikita Vasilyev
no flags Details
Patch (3.72 KB, patch)
2017-09-13 15:33 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-09-02 19:58:18 PDT
Bug 175343 does not implement at-rule section headers.


To manually test this:

1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/media.html
2. Inspect h3#title

Expected:

@media screen and (max-height: 999999px)
* {color: black}
* {color: orange}
* {color: orangered}
h3 {...}

Inherited from div.in
@media screen and (max-height: 999999px)
.in {font-size: 14px}
.in {font-size: 15px}
.in {font-size: 16px}
@media screen and (max-height: 999999px)
* {color: black}
* {color: orange}
* {color: orangered}

Inherited from body.out
@media screen and (max-height: 999999px)
* {color: black}
* {color: orange}
* {color: orangered}

Inherited from html
@media screen and (max-height: 999999px)
* {color: black}
* {color: orange}
* {color: orangered}
Comment 1 Radar WebKit Bug Importer 2017-09-02 19:58:34 PDT
<rdar://problem/34228389>
Comment 2 Nikita Vasilyev 2017-09-06 17:03:06 PDT
@keyframes aren't shown at all in the current Styles sidebar. Making it a separate issue: Bug 176484 - Web Inspector: Styles Redesign: Show CSS animations (@keyframes).
Comment 3 Nikita Vasilyev 2017-09-08 17:59:36 PDT
Created attachment 320323 [details]
Patch
Comment 4 Nikita Vasilyev 2017-09-08 18:14:03 PDT
Created attachment 320325 [details]
[Screenshot] With patch applied

Another test page: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/media2.html

Although this patch doesn't implement it, the plan is to make media query selectors editable (bug 176641) and gray out selectors that don't match (176640).
Comment 5 Joseph Pecoraro 2017-09-11 19:36:05 PDT
Comment on attachment 320323 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:248
> +        mediaLabel.append(WI.unlocalizedString("@media "), mediaText);

No need for the unlocalized here. Its clear that the string is code. We use unlocalized when a string looks like it might be something we would localized, but known that we don't want localized. For example strings in the UserInterface/Debug directory.
Comment 6 Nikita Vasilyev 2017-09-11 23:40:56 PDT
Created attachment 320523 [details]
Patch
Comment 7 Devin Rousso 2017-09-12 14:41:10 PDT
Comment on attachment 320523 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:18
> +        Don't display "all" or "screen" media query selectors. One of the goals of the
> +        Styles sidebar redesign is to fit more data.

This patch means that each rule would list all media queries, meaning that if every rule has the same query, we're repeating it over and over.  I feel like that would take up more space than our previous approach.  Do you have any data as to whether this is true/false?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:239
> +

Style: no newline necessary
Comment 8 Devin Rousso 2017-09-12 14:41:11 PDT
Comment on attachment 320523 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:18
> +        Don't display "all" or "screen" media query selectors. One of the goals of the
> +        Styles sidebar redesign is to fit more data.

This patch means that each rule would list all media queries, meaning that if every rule has the same query, we're repeating it over and over.  I feel like that would take up more space than our previous approach.  Do you have any data as to whether this is true/false?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:239
> +

Style: no newline necessary
Comment 9 Nikita Vasilyev 2017-09-12 20:36:18 PDT
Created attachment 320612 [details]
[Image] Before/after

(In reply to Devin Rousso from comment #8)
> Comment on attachment 320523 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320523&action=review
> 
> > Source/WebInspectorUI/ChangeLog:18
> > +        Don't display "all" or "screen" media query selectors. One of the goals of the
> > +        Styles sidebar redesign is to fit more data.
> 
> This patch means that each rule would list all media queries, meaning that
> if every rule has the same query, we're repeating it over and over.  I feel
> like that would take up more space than our previous approach.  Do you have
> any data as to whether this is true/false?

I haven't done an extensive analysis, but most websites that I've seen have the majority of their CSS rules without any media queries. Media queries are mostly used to modify layout for smaller screens and display high resolution media for retina screen.

This is what I often see in the current design:

    Inherited from: body
    @media screen and (max-width: 600px)
    body {
      ...
    }
    @media all
    body {
      ...
    }

In this example, we have to include `@media all` to explicitly tell that this rule doesn't share previous rule's media query.

We wouldn't have to do that if we didn't group media query headers.

The attached image is from twitter.com.
Comment 10 Devin Rousso 2017-09-13 15:16:20 PDT
Comment on attachment 320523 [details]
Patch

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

r=me

>>>> Source/WebInspectorUI/ChangeLog:18
>>>> +        Styles sidebar redesign is to fit more data.
>>> 
>>> This patch means that each rule would list all media queries, meaning that if every rule has the same query, we're repeating it over and over.  I feel like that would take up more space than our previous approach.  Do you have any data as to whether this is true/false?
>> 
>> This patch means that each rule would list all media queries, meaning that if every rule has the same query, we're repeating it over and over.  I feel like that would take up more space than our previous approach.  Do you have any data as to whether this is true/false?
> 
> I haven't done an extensive analysis, but most websites that I've seen have the majority of their CSS rules without any media queries. Media queries are mostly used to modify layout for smaller screens and display high resolution media for retina screen.
> 
> This is what I often see in the current design:
> 
>     Inherited from: body
>     @media screen and (max-width: 600px)
>     body {
>       ...
>     }
>     @media all
>     body {
>       ...
>     }
> 
> In this example, we have to include `@media all` to explicitly tell that this rule doesn't share previous rule's media query.
> 
> We wouldn't have to do that if we didn't group media query headers.
> 
> The attached image is from twitter.com.

Ah ok.  I guess for most cases where media queries aren't used its a massive benefit.
Comment 11 Nikita Vasilyev 2017-09-13 15:33:33 PDT
Created attachment 320698 [details]
Patch
Comment 12 WebKit Commit Bot 2017-09-13 16:20:40 PDT
Comment on attachment 320698 [details]
Patch

Clearing flags on attachment: 320698

Committed r221998: <http://trac.webkit.org/changeset/221998>
Comment 13 WebKit Commit Bot 2017-09-13 16:20:51 PDT
All reviewed patches have been landed.  Closing bug.