Closed Bug 1031351 Opened 6 years ago Closed 6 years ago

Style Editor sidebar not showing all active CSS files

Categories

(DevTools :: Style Editor, defect)

defect
Not set

Tracking

(firefox30 wontfix, firefox31+ verified, firefox32+ verified, firefox33 verified)

VERIFIED FIXED
Firefox 33
Tracking Status
firefox30 --- wontfix
firefox31 + verified
firefox32 + verified
firefox33 --- verified

People

(Reporter: ty.ferrie, Assigned: harth)

References

Details

(Keywords: regression, reproducible, testcase)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

Since upgrading to Firefox 30, the style editor does not list all active CSS files in the sidebar.


Actual results:

A limited list (approximately half) of active CSS files are displaying in sidebar in Firefox 30.  I downgraded my application to Firefox 29.0.1 and opened the style editor and I once again have all of my CSS files listed in the sidebar.

All CSS files are actively running on the page itself, they are simply not displaying in the sidebar list in the Style Editor.


Expected results:

All active CSS files should display in sidebar of Style Editor.  This works properly on FF29, but an incomplete list appears in FF30.
Can you provide a testcase?
Component: Untriaged → Developer Tools: Style Editor
Flags: needinfo?(ty.ferrie)
(In reply to Ty Ferrie from comment #2)
> Created attachment 8447223 [details]
> Style Editor Comparison FF29/FF30

Right, but like this we still can't see how the stylesheets are included on the page or debug the issue ourselves...
1) Navigate to https://hitech-demo.bigmachines.com
2) This will bring you to a login screen, however you do not need to login to view this issue.
3) Open Style Editor
4) In Firefox 29 you will see a full list of active style sheets for this page.  There should be 10 CSS files.  In Firefox 30 you will only see a list of 8 CSS files.  All files are still actively running on the page and it is displaying correctly in the browser, however the style editor list is now limited.
(In reply to Ty Ferrie from comment #4)
> 1) Navigate to https://hitech-demo.bigmachines.com
> 2) This will bring you to a login screen, however you do not need to login
> to view this issue.
> 3) Open Style Editor
> 4) In Firefox 29 you will see a full list of active style sheets for this
> page.  There should be 10 CSS files.  In Firefox 30 you will only see a list
> of 8 CSS files.  All files are still actively running on the page and it is
> displaying correctly in the browser, however the style editor list is now
> limited.

Thanks! That's very helpful. :-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ty.ferrie)
OS: Windows 7 → All
Hardware: x86_64 → All
<LINK REL="STYLESHEET" HREF="/bmfsweb/hitech-demo/hitech-demo.css" TYPE="text/css"/>
<LINK REL="STYLESHEET" HREF="/bmfsweb/hitech-demo/hitech-demoAlt.css" TYPE="text/css"/>

seem to be missing from the list.
Those 2 CSS files are still actively running on the page, however.  Their content is reflected in the browser display.
(In reply to Ty Ferrie from comment #7)
> Those 2 CSS files are still actively running on the page, however.  Their
> content is reflected in the browser display.

Yes.

Regression range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82c90c17fc95&tochange=e3b76b155ca4

Suspect: bug 711401.

Heather / Joe, can you have a look what's going on here?
Blocks: 711401
Flags: needinfo?(jwalker)
Bug 711401 does seem like the obvious cause, but I can't see a way that the regexp could throw. I'm going to plead ITSWEEKENDALREADY and see if Heather has more brain power left...
Flags: needinfo?(jwalker) → needinfo?(fayearthur)
Assignee: nobody → fayearthur
Flags: needinfo?(fayearthur)
Looks like the culprit for one of the sheets is:

voice-family: "\"}\"";

Luckily that shouldn't be too common, but still a problem. Another problem is that we try to prettify that sheet when we really don't need to.
The regression here is that we just became more aggressive in deciding what to prettify. This has caused other problems like: http://stackoverflow.com/questions/24252004/firefox-30-dev-tool-style-editor-formatting-differs-from-my-own.

I figured a better metric would be to compare the number of lines in the file vs. the number of rules, so if there's more than one line per rule, we minify it, otherwise we don't do anything.

The other problem (voice-family: "\"}\"";) should be fixed when we overhaul the prettifier this cycle.
Attachment #8448436 - Flags: review?(jwalker)
Comment on attachment 8448436 [details] [diff] [review]
Don't prettify source if it has more than one line per rule

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

So I guess the (slightly pathological) case is a compressed reset stylesheet with a low number of complex rules and a long copyright header. In that case count(rules) could be lower than count(lines) and we wouldn't prettify it.

I'm not sure that's a huge problem though?
Attachment #8448436 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #12)
> Comment on attachment 8448436 [details] [diff] [review]
> Don't prettify source if it has more than one line per rule
> 
> Review of attachment 8448436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I guess the (slightly pathological) case is a compressed reset stylesheet
> with a low number of complex rules and a long copyright header. In that case
> count(rules) could be lower than count(lines) and we wouldn't prettify it.
> 
> I'm not sure that's a huge problem though?

Yeah, that would be the problem. I'd think it's unlikely. We should consider adding a prettify button like the debugger has.

Try: https://tbpl.mozilla.org/?tree=Try&rev=80cee7851437
Keywords: checkin-needed
(In reply to Heather Arthur [:harth] from comment #13)
> (In reply to Joe Walker [:jwalker] from comment #12)
> > Comment on attachment 8448436 [details] [diff] [review]
> > Don't prettify source if it has more than one line per rule
> > 
> > Review of attachment 8448436 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So I guess the (slightly pathological) case is a compressed reset stylesheet
> > with a low number of complex rules and a long copyright header. In that case
> > count(rules) could be lower than count(lines) and we wouldn't prettify it.
> > 
> > I'm not sure that's a huge problem though?
> 
> Yeah, that would be the problem. I'd think it's unlikely. We should consider
> adding a prettify button like the debugger has.

The other thing to consider would be to remove the first comment using a regexp (which I think you could do safely and easily if you require the comment comes before any rules [1]) before counting the lines. But it's probably worth waiting to see if we need it.

[1]: /^\w\/\*.*?\*\/\w/ (not tested)
At the risk of asking a dumb question... why do failed-to-pretty-print stylesheets not show up at all anymore anyway? Surely we can just list and display them as-is?
https://hg.mozilla.org/mozilla-central/rev/0438007c97a2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Duplicate of this bug: 1030769
Comment on attachment 8448436 [details] [diff] [review]
Don't prettify source if it has more than one line per rule

Approval Request Comment
[Feature/regressing bug #]:
bug 711401

[User impact if declined]: 
Stylesheets will not be displayed or will not be displayed correctly in the style editor, see other user-reported bugs fixed by this: bug 1030769, http://stackoverflow.com/q/24252004

[Describe test coverage new/current, TBPL]:
Has been in m-c for a few days.

[Risks and why]:

[String/UUID change made/needed]:
None
Attachment #8448436 - Flags: approval-mozilla-aurora?
(In reply to :Gijs Kruitbosch from comment #16)
> At the risk of asking a dumb question... why do failed-to-pretty-print
> stylesheets not show up at all anymore anyway? Surely we can just list and
> display them as-is?

It's just that those stylesheets threw errors during prettification. The stylesheets in question had a tricky property value containing a { that our makeshift prettifier can't handle. We'll have to fix that with the new pretty printer in bug 724505. I've confirmed the new pretty-printer can handle that case.
(In reply to :Gijs Kruitbosch from comment #16)
> At the risk of asking a dumb question... why do failed-to-pretty-print
> stylesheets not show up at all anymore anyway? Surely we can just list and
> display them as-is?

But yeah, we could do a follow-up to catch any errors and display as-is. Good point.
We will definitely want this on Aurora. I'd like to give this a bit more time on m-c (been there 1 day) to verify the fix. I have marked tracking for beta so that Sylvestre and I can have a discussion about whether we want to take the risk of fixing this regression so late in the cycle.
Thanks Lawrence.
Indeed, I am interested to take it for beta 8 (go to build next Monday). Could you fill the uplift request for beta too? Thanks
Flags: needinfo?(fayearthur)
Comment on attachment 8448436 [details] [diff] [review]
Don't prettify source if it has more than one line per rule

As this has now been on m-c for a few days and I haven't seen any fallout, Aurora+. I'm also going to mark as Beta+ based on Sylvestre's request to get this on Beta.
Attachment #8448436 - Flags: approval-mozilla-beta+
Attachment #8448436 - Flags: approval-mozilla-aurora?
Attachment #8448436 - Flags: approval-mozilla-aurora+
Flags: needinfo?(fayearthur)
Ty, this should be fixed now in today's Nightly and tomorrow's Aurora and Beta builds. Can you please verify this to be true?
Flags: needinfo?(ty.ferrie)
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #26)
> Ty, this should be fixed now in today's Nightly and tomorrow's Aurora and
> Beta builds. Can you please verify this to be true?

Thank you all for your assistance with this.  I am not sure how to verify this as I do not have access to the Aurora or Beta builds.  Are these publicly available for download so that I can verify?
Flags: needinfo?(ty.ferrie)
Thank you!  I have verified that this is fixed in both Aurora and Beta.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.