Closed Bug 722506 Opened 9 years ago Closed 8 years ago

Style Editor should support @imported stylesheets

Categories

(DevTools :: Style Editor, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: kikizgz, Assigned: jryans)

References

Details

Attachments

(2 files, 4 obsolete files)

Firefox don't list CSS imported files.
Attached image screenhost
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: Bug in Style Editor → Style Editor should support @imported stylesheets
Version: 11 Branch → Trunk
Will not it be fixed?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → major
Priority: -- → P1
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
Got bitten by that today. I can't analyze a page without that. (I do not control the content)
Attached patch Fix, part 1 (obsolete) — Splinter Review
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.
Attachment #628691 - Flags: feedback?(cedricv)
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: nobody → jryans
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
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 on attachment 720516 [details] [diff] [review]
Patch v2

I'll take a look at this too.
Attachment #720516 - Flags: feedback?(fayearthur)
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+
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 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+
Attached patch Patch v3 (obsolete) — Splinter Review
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)
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+
Attached patch Patch v4Splinter Review
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+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/d855dc68a202
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d855dc68a202
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
(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?
(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! :)
Duplicate of this bug: 840859
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.