Last Comment Bug 722196 - Rule view not showing media query information
: Rule view not showing media query information
Status: RESOLVED FIXED
[computedview][ruleview][fixed-in-fx-...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 10 Branch
: All All
: P1 major (vote)
: Firefox 13
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
: Patrick Brosset <:pbro>
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-29 15:01 PST by sys.sgx
Modified: 2012-03-08 06:42 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test Case (353 bytes, text/html)
2012-02-29 07:22 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details
Patch (11.21 KB, patch)
2012-02-29 07:24 PST, Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: review+
Details | Diff | Splinter Review
[in-fx-team] Patch 2 (11.25 KB, patch)
2012-03-01 11:28 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review

Description sys.sgx 2012-01-29 15:01:46 PST
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:10.0) Gecko/20100101 Firefox/10.0
Build ID: 20120123235200

Steps to reproduce:

1. Browse here: http://markdotto.com/bs2/docs/scaffolding.html
2. Inspect the element span12, it's the "12" that shows "1170px" on hover.
3. The editor does not load the correct css file, and shows 940px as the css rule applied.



Expected results:

There are two css files on this page. The responsive css most likely overwrites the rules for span class, and makes the span12 equal to 1170px. This should be shown in the style editor when inspecting the div.
Comment 1 Dave Camp (:dcamp) 2012-01-29 15:28:56 PST
I can confirm this.
Comment 2 Dave Camp (:dcamp) 2012-01-30 11:23:34 PST
The 1170px width rule in bootstrap-responsive.css is wrapped in a min-width media query.  It looks to me like (depending on window size) opening the sidebar is changing the size of the window, which adds in that rule.  So there are two problems that need to be fixed:

a) The rule view should give more information about rules that match a media query.
b) The style sidebar causes a resize, which mucks with the page you're inspecting.

sys.sgx, can you confirm that observation?
Comment 3 sys.sgx 2012-01-30 13:04:24 PST
Yes, I can confirm this. Based on your message:

a) The rule view *must* give information about events that led to the current style being applied. (ie media queries)
b) I think this is important. Maybe chrome doesnt have this issue because the editor is not changing the size of this element. The style inspector should not split the current window, thus leading to a resize. It has to be overlayed so that the actual page is shown as intended by the css rules.
Comment 4 sys.sgx 2012-01-30 13:04:50 PST
New one found: https://bugzilla.mozilla.org/show_bug.cgi?id=722446
Comment 5 sys.sgx 2012-01-30 13:11:40 PST
It might be good to start a new issue if you agree with the idea of having the style inspector in overlay mode.

Thanks
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-29 07:22:39 PST
Created attachment 601603 [details]
Test Case

This is a simple test case that makes it possible to easily reproduce the problem:
1. Open the test case
2. Resize the browser window width to roughly the same size as the red div.
3. Right-click and inspect the div.

The side panel changes the content width, which brings in the max-width media query, which changes the div's width.
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-29 07:24:49 PST
Created attachment 601604 [details] [diff] [review]
Patch

We still have no way to avoid reflow when the side panel is opened but that issue is something we are aware of and we have been actively seeking a solution.

The issue of not displaying any information identifying media query styles is simple to solve by slightly changing the title we display above the rule (as this patch does).
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-03-01 07:01:23 PST
Comment on attachment 601604 [details] [diff] [review]
Patch

Review of attachment 601604 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +382,5 @@
>        this._title = CssLogic._strings.formatStringFromName("rule.inheritedSource",
>                                                             args, args.length);
>      }
>  
> +    return this.mediaText ? this._title + " @media " + this.mediaText : this._title;

Hey! You can shave 7 whole bytes of this. Should dramatically improve load times...
    return this._title + (this.mediaText ? " @media " + this.mediaText : "");

(Yes, I'm short of things to comment on in this patch)

::: browser/themes/gnomestripe/devtools/csshtmltree.css
@@ +141,4 @@
>    cursor: pointer;
>  }
>  
>  /* This rule is necessary because Templater.jsm breaks LTR TDs in RTL docs */

I know this isn't new, but is there something that Templater should do that it's not here?
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2012-03-01 11:28:09 PST
Created attachment 602045 [details] [diff] [review]
[in-fx-team] Patch 2

(In reply to Joe Walker from comment #8)
> Comment on attachment 601604 [details] [diff] [review]
> Patch
> 
> Review of attachment 601604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/CssRuleView.jsm
> @@ +382,5 @@
> >        this._title = CssLogic._strings.formatStringFromName("rule.inheritedSource",
> >                                                             args, args.length);
> >      }
> >  
> > +    return this.mediaText ? this._title + " @media " + this.mediaText : this._title;
> 
> Hey! You can shave 7 whole bytes of this. Should dramatically improve load
> times...
>     return this._title + (this.mediaText ? " @media " + this.mediaText : "");
> 
> (Yes, I'm short of things to comment on in this patch)
> 

I don't care about the 7 bytes but now the line is under 80 characters so I have a warm fuzzy feeling.

> ::: browser/themes/gnomestripe/devtools/csshtmltree.css
> @@ +141,4 @@
> >    cursor: pointer;
> >  }
> >  
> >  /* This rule is necessary because Templater.jsm breaks LTR TDs in RTL docs */
> 
> I know this isn't new, but is there something that Templater should do that
> it's not here?

This is neither the time or the place but I don't care ;o)

I have never been able to work out exactly why they break. If I remember rightly, whenever anything in the xhtml template is invalid it breaks rtl for it's parent element and everything further down in the file. Adding "invalid" attributes breaks the RTLness because it invalidates the xhtml doc. Because the doc is already broken, when the template is processed it will have some rtl and some ltr. There was also something specific to td elements but I am not certain what.
Comment 10 Panos Astithas [:past] 2012-03-06 03:44:27 PST
https://hg.mozilla.org/integration/fx-team/rev/da2e0e0fbf07
Comment 11 Rob Campbell [:rc] (:robcee) 2012-03-08 06:42:59 PST
https://hg.mozilla.org/mozilla-central/rev/da2e0e0fbf07

Note You need to log in before you can comment on or make changes to this bug.