Bug 176289 – Web Inspector: Styles Redesign: display @media section headers

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

[Nikita Vasilyev]

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}

[Radar WebKit Bug Importer]

<rdar://problem/34228389>

[Nikita Vasilyev]

@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).

[Nikita Vasilyev]

Created attachment 320323 [details]
Patch

[Nikita Vasilyev]

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).

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

Created attachment 320523 [details]
Patch

[Devin Rousso]

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

[Devin Rousso]

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

[Nikita Vasilyev]

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.

[Devin Rousso]

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.

[Nikita Vasilyev]

Created attachment 320698 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 320698 [details]
Patch

Clearing flags on attachment: 320698

Committed r221998: <http://trac.webkit.org/changeset/221998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.