Bug 65626 - Web Inspector: Show media queries associated with specific CSS rules
Summary: Web Inspector: Show media queries associated with specific CSS rules
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-03 10:20 PDT by Zach Leatherman
Modified: 2011-11-10 06:56 PST (History)
15 users (show)

See Also:


Attachments
Patch (17.73 KB, patch)
2011-11-08 08:47 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (30.93 KB, patch)
2011-11-09 08:30 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[IMAGE] Screenshot of the Styles sidebar pane with media queries displayed (57.59 KB, image/png)
2011-11-09 08:44 PST, Alexander Pavlov (apavlov)
no flags Details
Patch (31.06 KB, patch)
2011-11-10 01:59 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (31.06 KB, patch)
2011-11-10 02:30 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (31.00 KB, patch)
2011-11-10 06:05 PST, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zach Leatherman 2011-08-03 10:20:41 PDT
This is probably best explained with an example.

Go to http://jquerymobile.com/test/
Inspect div.content-secondary
When the window >= 650px, the width of this element is set to 45%.
When I resize the window < 650px, there is no width set.

There currently is no visual indicator that exists when the width is set to 45% that it is conditional on a media query.  I would like this to be explicit in the web inspector styles.

Maybe something like this in the Styles pane:

@media all and (min-width: 650px){              jqm-docs.css:150
  .content-secondary {                          jqm-docs.css:178
    text-align: left;
    float: left;
    width: 45%;
    background: none;
    border-top: 0;
  }
}
Comment 1 Alexander Pavlov (apavlov) 2011-11-08 08:47:11 PST
Created attachment 114082 [details]
Patch
Comment 2 Pavel Feldman 2011-11-08 10:08:49 PST
Comment on attachment 114082 [details]
Patch

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

Could we get a screenshot for this? r- for a number of small problems.

> Source/WebCore/inspector/Inspector.json:-446
> -                "description": "Enables console domain, sends the messages collected so far to the client by means of the <code>messageAdded</code> notification." 

This change will confuse the version control annotation. Could you not do it please?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:750
> +        for (size_t i = mediaVector.size(); i > 0; --i)

We usually do = size - 1; i >=0; --i

So the order is self media followed by a reversed list (i.e. starting from root?) This is confusing, the list should be either from root to current or from current to root.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:843
> +            this._mediaTextElement = this.titleElement.createChild("div", "media");

So this section property will iterate over all of the elements and will not be accessed from anywhere else? Can we use local variable instead?

> LayoutTests/inspector/styles/styles-iframe-expected.txt:9
> +[expanded] media="screen"@media screen#main { (styles-iframe.html:6)

A test involving nested medias would be great.
Comment 3 Alexander Pavlov (apavlov) 2011-11-09 08:30:30 PST
Created attachment 114289 [details]
Patch
Comment 4 Alexander Pavlov (apavlov) 2011-11-09 08:31:55 PST
A more complete solution has been attached, which enables navigation to media lists in the chain, when possible.

(In reply to comment #2)
> (From update of attachment 114082 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114082&action=review
> 
> Could we get a screenshot for this? r- for a number of small problems.
> 
> > Source/WebCore/inspector/Inspector.json:-446
> > -                "description": "Enables console domain, sends the messages collected so far to the client by means of the <code>messageAdded</code> notification." 
> 
> This change will confuse the version control annotation. Could you not do it please?

Fixed

> > Source/WebCore/inspector/InspectorStyleSheet.cpp:750
> > +        for (size_t i = mediaVector.size(); i > 0; --i)
> 
> We usually do = size - 1; i >=0; --i

Heh. It's a size_t which is unsigned. Resorted to building an outward-traversal list and reversing it while rendering.

> So the order is self media followed by a reversed list (i.e. starting from root?) This is confusing, the list should be either from root to current or from current to root.

It was not SELF MEDIA but media query of the parent stylesheet, the root one. But I've found out that @import rules can also have media queries, so the traversal is slightly more involved (see the extracted method).

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:843
> > +            this._mediaTextElement = this.titleElement.createChild("div", "media");
> 
> So this section property will iterate over all of the elements and will not be accessed from anywhere else? Can we use local variable instead?

Fixed.

> > LayoutTests/inspector/styles/styles-iframe-expected.txt:9
> > +[expanded] media="screen"@media screen#main { (styles-iframe.html:6)
> 
> A test involving nested medias would be great.

Done.
Comment 5 Pavel Feldman 2011-11-09 08:43:21 PST
Comment on attachment 114289 [details]
Patch

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

Inspector looks good. Please make sure CSS folks are file with the m_lastLine. It looks sane to me.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:136
> +    default:

Removing default will convert it from runtime to compile time error.
Comment 6 Alexander Pavlov (apavlov) 2011-11-09 08:44:43 PST
Created attachment 114292 [details]
[IMAGE] Screenshot of the Styles sidebar pane with media queries displayed
Comment 7 Alexander Pavlov (apavlov) 2011-11-10 01:59:55 PST
Created attachment 114455 [details]
Patch

[PATCH] Fix winbot compilability, adjust linked URLs display
Comment 8 Alexander Pavlov (apavlov) 2011-11-10 02:30:16 PST
Created attachment 114458 [details]
Patch

[PATCH] Re-submit for Windows EWS bot (some weird bot issue occurred)
Comment 9 Antti Koivisto 2011-11-10 05:54:52 PST
Comment on attachment 114458 [details]
Patch

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

> Source/WebCore/css/MediaList.h:89
> +    int lastLine() const { return m_lastLine; }
> +    void updateLastLine(int lastLine) { m_lastLine = lastLine; }

Can lastLine be negative? If not, why signed type?

WebKit coding style is to usually name setters as "setFoo".
Comment 10 Antti Koivisto 2011-11-10 05:56:06 PST
Looks ok otherwise.
Comment 11 Alexander Pavlov (apavlov) 2011-11-10 06:02:56 PST
(In reply to comment #9)
> (From update of attachment 114458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114458&action=review
> 
> > Source/WebCore/css/MediaList.h:89
> > +    int lastLine() const { return m_lastLine; }
> > +    void updateLastLine(int lastLine) { m_lastLine = lastLine; }
> 
> Can lastLine be negative? If not, why signed type?

CSS classes (CSSParser, CSSStyleRule) use "int" to that end, so I decided to stick to the same type.

> WebKit coding style is to usually name setters as "setFoo".

Will fix.
Comment 12 Alexander Pavlov (apavlov) 2011-11-10 06:05:33 PST
Created attachment 114480 [details]
Patch

Comment from anttik fixed
Comment 13 Alexander Pavlov (apavlov) 2011-11-10 06:56:47 PST
Committed r99849: <http://trac.webkit.org/changeset/99849>