Style Editor should support @imported stylesheets

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools: Style Editor
P3
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Enrique Jiménez, Assigned: jryans)

Tracking

Trunk
Firefox 22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Firefox don't list CSS imported files.
(Reporter)

Comment 1

5 years ago
Created attachment 592872 [details]
screenhost

Updated

5 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

5 years ago
Will not it be fixed?

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

5 years ago
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)
Created attachment 628691 [details] [diff] [review]
Fix, part 1

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)
(Assignee)

Comment 6

4 years ago
Created attachment 718817 [details] [diff] [review]
Add initial import rules as stylesheets

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

4 years ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
(Assignee)

Comment 7

4 years ago
Created attachment 720516 [details] [diff] [review]
Patch v2

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+
Created attachment 723567 [details] [diff] [review]
Patch v3

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)
https://tbpl.mozilla.org/?tree=Try&rev=8bf3036fe00e
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+
Created attachment 725845 [details] [diff] [review]
Patch v4

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

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22

Comment 18

4 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?
(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
You need to log in before you can comment on or make changes to this bug.