Last Comment Bug 722506 - Style Editor should support @imported stylesheets
: Style Editor should support @imported stylesheets
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Style Editor (show other bugs)
: Trunk
: All All
: P3 normal with 1 vote (vote)
: Firefox 22
Assigned To: J. Ryan Stinnett [:jryans] (use ni?)
:
Mentors:
: 840859 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-30 14:48 PST by Enrique Jiménez
Modified: 2014-04-21 18:05 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenhost (46.24 KB, image/png)
2012-01-30 14:50 PST, Enrique Jiménez
no flags Details
Fix, part 1 (4.86 KB, patch)
2012-05-31 05:55 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
Add initial import rules as stylesheets (8.09 KB, patch)
2013-02-26 19:50 PST, J. Ryan Stinnett [:jryans] (use ni?)
no flags Details | Diff | Splinter Review
Patch v2 (8.11 KB, patch)
2013-03-03 20:10 PST, J. Ryan Stinnett [:jryans] (use ni?)
mihai.sucan: feedback+
fayearthur: feedback+
Details | Diff | Splinter Review
Patch v3 (9.52 KB, patch)
2013-03-11 11:27 PDT, J. Ryan Stinnett [:jryans] (use ni?)
fayearthur: review+
Details | Diff | Splinter Review
Patch v4 (11.33 KB, patch)
2013-03-17 00:07 PDT, J. Ryan Stinnett [:jryans] (use ni?)
jryans: review+
Details | Diff | Splinter Review

Description Enrique Jiménez 2012-01-30 14:48:42 PST
Firefox don't list CSS imported files.
Comment 1 Enrique Jiménez 2012-01-30 14:50:20 PST
Created attachment 592872 [details]
screenhost
Comment 2 Enrique Jiménez 2012-03-03 14:31:35 PST
Will not it be fixed?
Comment 3 Cedric Vivier [:cedricv] 2012-03-28 05:36:09 PDT
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.
Comment 4 Anthony Ricaud (:rik) 2012-04-04 03:19:17 PDT
Got bitten by that today. I can't analyze a page without that. (I do not control the content)
Comment 5 Daniel Glazman (:glazou) 2012-05-31 05:55:40 PDT
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.
Comment 6 J. Ryan Stinnett [:jryans] (use ni?) 2013-02-26 19:50:01 PST
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.
Comment 7 J. Ryan Stinnett [:jryans] (use ni?) 2013-03-03 20:10:20 PST
Created attachment 720516 [details] [diff] [review]
Patch v2

Only correcting the commit message format, otherwise identical to attachment 718817 [details] [diff] [review].
Comment 8 Heather Arthur [:harth] 2013-03-05 13:13:13 PST
Comment on attachment 720516 [details] [diff] [review]
Patch v2

I'll take a look at this too.
Comment 9 Mihai Sucan [:msucan] 2013-03-07 12:11:12 PST
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.
Comment 10 Mihai Sucan [:msucan] 2013-03-07 12:12:50 PST
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 Heather Arthur [:harth] 2013-03-08 18:42:13 PST
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.
Comment 12 J. Ryan Stinnett [:jryans] (use ni?) 2013-03-11 11:27:07 PDT
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.
Comment 13 J. Ryan Stinnett [:jryans] (use ni?) 2013-03-12 08:28:39 PDT
https://tbpl.mozilla.org/?tree=Try&rev=8bf3036fe00e
Comment 14 Heather Arthur [:harth] 2013-03-15 11:04:02 PDT
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.
Comment 15 J. Ryan Stinnett [:jryans] (use ni?) 2013-03-17 00:07:57 PDT
Created attachment 725845 [details] [diff] [review]
Patch v4

I've made both changes as requested, so this one should be ready to land.
Comment 16 Victor Porof [:vporof][:vp] 2013-03-20 04:07:33 PDT
https://hg.mozilla.org/integration/fx-team/rev/d855dc68a202
Comment 17 Tim Taubert [:ttaubert] 2013-03-27 06:53:49 PDT
https://hg.mozilla.org/mozilla-central/rev/d855dc68a202
Comment 18 j.j. 2013-03-29 04:48:46 PDT
(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?
Comment 19 J. Ryan Stinnett [:jryans] (use ni?) 2013-03-29 19:56:43 PDT
(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! :)
Comment 20 Heather Arthur [:harth] 2014-04-21 18:05:53 PDT
*** Bug 840859 has been marked as a duplicate of this bug. ***

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