734861 - Expose stylesheet collection (including UA) for a document via nsIStylesheetService

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

UA stylesheets aren't included in document.styleSheets so there is no way to get a list of all stylesheets used by a document (including UA sheets) without running domUtils.getCSSStyleRules() on every element of a page and building up a list of sheets as well as grabbing the sheets from document.styleSheets.

I believe that nsStylesheetService contains a collection (sheets) of all stylesheets contained in a document, but this collection is not exposed via nsIStylesheetService. This would be useful for our own developer tools and for extension developers.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> I believe that nsStylesheetService contains a collection (sheets) of all stylesheets
> contained in a document

I don't believe it does.

That said, this could probably be done, but I have a question about desired behavior.  Do you want to see sheets that ere in document.styleSheets but disabled?

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Boris Zbarsky (:bz) from comment #1)
> > I believe that nsStylesheetService contains a collection (sheets) of all stylesheets
> > contained in a document
> 
> I don't believe it does.
> 

As a JS guy, I am not surprised that I got that wrong.

> That said, this could probably be done, but I have a question about desired
> behavior.  Do you want to see sheets that ere in document.styleSheets but
> disabled?

Yes, it would be useful if we could list all sheets along with their enabled / disabled states.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So here's the story in terms of how this stuff works...

UA/user sheets are not stored in the document directly.  They're only stored in the style set.

Author sheets are stored in the style set if enabled, but in the document no matter what.

So maybe what we really want here is a method in inspector utils that grabs the UA and user sheets from the style set for the given document, then grabs the author sheets from the document itself.  I think that should give us the desired behavior...

What do we want here for scoped stylesheets?

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Boris Zbarsky (:bz) from comment #3)
> So here's the story in terms of how this stuff works...
> 
> UA/user sheets are not stored in the document directly.  They're only stored
> in the style set.
> 
> Author sheets are stored in the style set if enabled, but in the document no
> matter what.
> 
> So maybe what we really want here is a method in inspector utils that grabs
> the UA and user sheets from the style set for the given document, then grabs
> the author sheets from the document itself.  I think that should give us the
> desired behavior...
> 

Agreed.

> What do we want here for scoped stylesheets?

After thinking on this I would say that returning scoped stylesheets with a scope property containing the scope root node would make the most sense. Checking sheet.scope is truthy would let us know if a stylesheet is scoped else we would easily be able to see what it is scoped to.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.  There is no such property on sheets right now, but maybe we should add one.  Cameron?

Comment 6

[Cameron McCormack (:heycam)]

On a DOM StyleSheet object itself?  Yeah, I think it should have a .scope property.  I have https://www.w3.org/Bugs/Public/show_bug.cgi?id=19995 filed for the spec side of things.

Comment 7

[Mina Almasry]

Hey bz. I think I can also take on this, if that's okay.

Where do I start with this? What guides/code should I read to get a grasp of how this works?

> Author sheets are stored in the style set if enabled, but in the document no
> matter what.
> 
> So maybe what we really want here is a method in inspector utils 

Where is inspector utils? Is this inIDOMUtils again?

> that grabs the UA and user sheets from the style set for the given document, then grabs
> the author sheets from the document itself.

Where do I find these? 

> What do we want here for scoped stylesheets?

>After thinking on this I would say that returning scoped stylesheets with a scope property >containing the scope root node would make the most sense.

Where do I find the scope root?

Comment 8

[Mina Almasry]

I'd love to work on this. Can you break down for me what needs to be done?

>want here is a method in inspector utils that grabs the UA and user sheets from the style set >for the given document, then grabs the author sheets from the document itself.

I assume this means a method in inIDOMUtils? where is the style set and the author sheets in a document? How do I write a test for this?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Where is inspector utils? Is this inIDOMUtils again?

Yes.

> Where do I find these? 

Presumably the method would take a document as an argument.  You can get the style set from the document's presshell, if it's not null.  For getting the document sheets, you'd just GetNumberOfStyleSheets/GetStyleSheetAt on the document.

> Where do I find the scope root?

Don't worry about this part; as pointed out in the W3C bug, sheets have an ownerNode which can then be checked for .scoped.

> How do I write a test for this?

Probably using a chrome mochitest, like other tests for inIDOMUtils, right?  And then checking that the sheets you get include one whose URI is resource://gre-resources/ua.css maybe.

Comment 10

[Mina Almasry]

Attachment: patch with test

Alrighty then. When you break it down like that it becomes really easy. Here is a patch with a test like you described.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 777638 [details] [diff] [review]
patch with test

So I don't think we should be putting the non-CSS sheets from the style set in the list, since script can't do anything with them.  Furthermore, we shouldn't be adding the author sheets twice.

So I think we should only add the eAgentSheet and eUserSheet sheets from the style set.

Furthermore, I think we should return the sheets in agent, user, document order, because that's the cascade order.

Finally, I think we should return an actual JS array here, not an nsIArray.  So probably just use [array] annotations (see nsTransactionList::GetData and nsITransactionList.getData for an example) to do that.

Comment 12

[Mina Almasry]

Attachment: patch

Changes made. Here is an updated patch.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 778191 [details] [diff] [review]
patch

r=me

Comment 14

[Mina Almasry]

Not a single nit even. Boy am I proud of myself ;)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Not a single Try link even. Boy am I nervous ;)

https://hg.mozilla.org/integration/mozilla-inbound/rev/83f4f6f435cf

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Nope, sure didn't see that coming. Backed out for mochitest-5 timeouts on all platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8950b75893e

https://tbpl.mozilla.org/php/getParsedLog.php?id=25610189&tree=Mozilla-Inbound

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Also, shouldn't this have had an IID rev? It sure caused needs-clobber bustage.

Comment 18

[Mina Almasry]

My mistake Ryan :(

I revved the iid, added SimpleTest.finish() call in the test (I didn't think I'd have to there...) and here is a try push:

https://tbpl.mozilla.org/?tree=Try&rev=821074da0542

Comment 19

[Mina Almasry]

Still trying to get a feel for what's proper use of the try server and what's overuse...

Comment 20

[Boris Zbarsky [:bzbarsky]]

You shouldn't need an explicit finish() call in this test.

Comment 21

[Mina Almasry]

I know I shouldn't, but without it the test just keeps running after it shows TEST-PASS and I assume it keeps running until it times out. I've also noticed in my other tests, I have finish() in there even when they weren't async...

So after revving the iid a couple more times, here is a decent try: https://tbpl.mozilla.org/?tree=Try&rev=8aa5e4b0fd2a

I don't know what to make of it. I have some test failures, but they seem unrelated to this patch, and they fail in the optimized build, and pass in debug, or vice versa. If it's okay with you, I'll try another checkin...

Comment 22

[Mina Almasry]

Attachment: Patch

I'll mark this checkin. Feel free to refuse or remove checkin if the try link doesn't look good or you want the SimpleTest.finish() removed.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

None of the failures in your Try run look particularly suspicious (the B2G ones were known bustage from an external repo yesterday).

https://hg.mozilla.org/integration/mozilla-inbound/rev/e499b8afbdc6

Comment 24

[Boris Zbarsky [:bzbarsky]]

No.  No.  This is NOT OK.

You should not be manually calling SimpleTest.finish() unless you called SimpleTest.waitForExplicitFinish, ever.


If for some reason the finish() is needed (because someone broke the test harness?) then you need a waitForExplicitFinish() at toplevel in that script.

Comment 25

[Mina Almasry]

Attachment: Patch fix - added waitForExplicitFinish call

Boris, here is a fix, added waitForExplicitFinish() call. Review?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 781191 [details] [diff] [review]
Patch fix - added waitForExplicitFinish call

r=me

Comment 27

[Mina Almasry]

please checkin "Patch fix - added waitForExplicitFinish call"

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e499b8afbdc6

\o/

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b476f31f2762

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b476f31f2762