Closed
Bug 722506
Opened 13 years ago
Closed 12 years ago
Style Editor should support @imported stylesheets
Categories
(DevTools :: Style Editor, defect, P3)
DevTools
Style Editor
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 22
People
(Reporter: kikizgz, Assigned: jryans)
References
Details
Attachments
(2 files, 4 obsolete files)
46.24 KB,
image/png
|
Details | |
11.33 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
Firefox don't list CSS imported files.
Reporter | ||
Comment 1•13 years ago
|
||
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: Bug in Style Editor → Style Editor should support @imported stylesheets
Version: 11 Branch → Trunk
Reporter | ||
Comment 2•13 years ago
|
||
Will not it be fixed?
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Updated•13 years ago
|
Severity: normal → major
Priority: -- → P1
Comment 3•13 years ago
|
||
Enrique, this will get fixed. However this is not trivial to fix and there is more important issues to work on.
This can be worked around by un-@importing the style sheets (using multiple <link> or <style>) on a web page you're working on.
Severity: major → normal
Priority: P1 → P3
Comment 4•13 years ago
|
||
Got bitten by that today. I can't analyze a page without that. (I do not control the content)
This fix part 1 adds
1. visibility of all nested levels of imported stylesheets
2. application of changes in imported stylesheets
3. modification of styleSheetIndex to correctly handle all stylesheets
Part 2 will focus on what happens when an stylesheet's import is added to or
removed from a parent stylesheet.
Updated•13 years ago
|
Attachment #628691 -
Flags: feedback?(cedricv)
Assignee | ||
Comment 6•12 years ago
|
||
I've revived the patch from comment #5, cleaned it up a bit, and added a test.
This enables you to see and edit stylesheets loaded via @import rules. If this looks good, I'll start looking into further changes for dynamically added @imports.
Attachment #718817 -
Flags: review?(mihai.sucan)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•12 years ago
|
||
Only correcting the commit message format, otherwise identical to attachment 718817 [details] [diff] [review].
Attachment #628691 -
Attachment is obsolete: true
Attachment #718817 -
Attachment is obsolete: true
Attachment #628691 -
Flags: feedback?(cedricv)
Attachment #718817 -
Flags: review?(mihai.sucan)
Attachment #720516 -
Flags: feedback?(mihai.sucan)
Comment 8•12 years ago
|
||
Comment on attachment 720516 [details] [diff] [review]
Patch v2
I'll take a look at this too.
Attachment #720516 -
Flags: feedback?(fayearthur)
Comment 9•12 years ago
|
||
Comment on attachment 720516 [details] [diff] [review]
Patch v2
Great to see @import working. Thank you for the patch!
Just one issue: I am not seeing any styles at on this page:
http://mihaisucan.github.com/mozilla-work/test.html
Do you know why?
I would like to apologize for the time it took me to get to your patch. Lots of stuff in my queue.
Please also Heather in the feedback and review process. She is working on style editor improvements, if I am not mistaken. I do not know how this patch fits with the work she is doing. I am currently involved in other developer tools projects.
Heather, thank you for taking this patch in your review queue.
Attachment #720516 -
Flags: feedback?(mihai.sucan) → feedback+
Comment 10•12 years ago
|
||
erm typos.
(In reply to Mihai Sucan [:msucan] from comment #9)
> Comment on attachment 720516 [details] [diff] [review]
> Patch v2
>
> Great to see @import working. Thank you for the patch!
>
> Just one issue: I am not seeing any styles at on this page:
s/at//
> Please also Heather in the feedback and review process. She is working on
Please also keep Heather in the loop.
Comment 11•12 years ago
|
||
Comment on attachment 720516 [details] [diff] [review]
Patch v2
Review of attachment 720516 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks so much for the patch. As for the site that Mihai pointed out, when you open the style editor this error appears in the Error Console:
Error: TypeError: aSheet is null
Source File: resource:///modules/devtools/StyleEditorChrome.jsm
Line: 297
So we'll need to fix that.
I'm currently rewriting the style editor to do remote debugging, but that won't land for a couple more weeks. If we get this in in the meantime, and I can just incorporate your code before I land.
::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +136,5 @@
> + if (this._styleSheetIndex != -1)
> + return index;
> + } else if (rule.type != Ci.nsIDOMCSSRule.CHARSET_RULE) {
> + // @import rules must precede all others except @charset
> + return index;
nice
::: browser/devtools/styleeditor/test/browser_styleeditor_import_rule.js
@@ +19,5 @@
> +function run(aChrome)
> +{
> + is(aChrome.editors.length, 3,
> + "there are 3 stylesheets after loading @imports");
> +
Might as well make sure it's the correct style sheet while you're at it. Good to be safe.
Attachment #720516 -
Flags: feedback?(fayearthur) → feedback+
Assignee | ||
Comment 12•12 years ago
|
||
This patch should be ready to go. Dynamically added style sheets are not yet handled though. My preference would be to defer that and file a separate bug to resolve it, but let me know if you feel it belongs here instead.
(In reply to Heather Arthur [:harth] from comment #11)
> Comment on attachment 720516 [details] [diff] [review]
> Patch v2
>
> Review of attachment 720516 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Thanks so much for the patch. As for the site that Mihai pointed out, when
> you open the style editor this error appears in the Error Console:
>
> Error: TypeError: aSheet is null
> Source File: resource:///modules/devtools/StyleEditorChrome.jsm
> Line: 297
>
> So we'll need to fix that.
Mihai's test page had an @import cycle (A imports B imports A), which results in the styleSheet property being null once when you visit the same one again further down the chain.
I'm checking for this case now, and Mihai's test site now shows all relevant CSS files.
> ::: browser/devtools/styleeditor/test/browser_styleeditor_import_rule.js
> @@ +19,5 @@
> > +function run(aChrome)
> > +{
> > + is(aChrome.editors.length, 3,
> > + "there are 3 stylesheets after loading @imports");
> > +
>
> Might as well make sure it's the correct style sheet while you're at it.
> Good to be safe.
Added these tests. I've also changed the test case to be an import cycle like Mihai's site.
Attachment #720516 -
Attachment is obsolete: true
Attachment #723567 -
Flags: review?(fayearthur)
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Comment on attachment 723567 [details] [diff] [review]
Patch v3
Review of attachment 723567 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry for the delay, this looks good. Just a couple small things:
::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +122,5 @@
> },
>
> /**
> + * Recursively traverse imported stylesheets to find the index
> + *
add a @param comment for the 'aIndex' argument as well.
::: browser/devtools/styleeditor/StyleEditorChrome.jsm
@@ +326,5 @@
> + }
> +
> + let editor = new StyleEditor(document, rule.styleSheet);
> + editor.addActionListener(this);
> + this._editors.push(editor);
I can just change this when I merge this into my remoting patch, but just as a note, this chunk of code is duplicated in a few places so a new method like `createStyleEditor(document, styleSheet)` would save some duplication.
Attachment #723567 -
Flags: review?(fayearthur) → review+
Assignee | ||
Comment 15•12 years ago
|
||
I've made both changes as requested, so this one should be ready to land.
Attachment #723567 -
Attachment is obsolete: true
Attachment #725845 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [land-in-fx-team]
Comment 16•12 years ago
|
||
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 17•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Comment 18•12 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #12)
> Dynamically added style sheets are not yet handled though
is there a bug# for this?
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to j.j. (mostly inactive in 2013, too) from comment #18)
> (In reply to J. Ryan Stinnett [:jryans] from comment #12)
> > Dynamically added style sheets are not yet handled though
>
> is there a bug# for this?
I am no longer able to reproduce this issue with the latest code in fx-team, so it look like no bug is needed. Sorry I forgot to update this issue about it, and thanks for double checking! :)
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•