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

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: miker, Assigned: almasry.mina)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bz][lang=c++])

Attachments

(2 attachments, 2 obsolete attachments)

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.
> 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?
(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.
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?
Flags: needinfo?(mratcliffe)
Whiteboard: [mentor=bz][lang=c++]
(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.
Flags: needinfo?(mratcliffe)
Hmm.  There is no such property on sheets right now, but maybe we should add one.  Cameron?
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
Assignee

Comment 7

6 years ago
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?
Assignee

Comment 8

6 years ago
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?
Assignee: nobody → almasry.mina
> 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.
Status: NEW → ASSIGNED
Assignee

Comment 10

6 years ago
Posted patch patch with test (obsolete) — Splinter Review
Alrighty then. When you break it down like that it becomes really easy. Here is a patch with a test like you described.
Attachment #777638 - Flags: review?(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.
Attachment #777638 - Flags: review?(bzbarsky)
Attachment #777638 - Flags: review-
Attachment #777638 - Flags: feedback+
Assignee

Comment 12

6 years ago
Posted patch patch (obsolete) — Splinter Review
Changes made. Here is an updated patch.
Attachment #777638 - Attachment is obsolete: true
Attachment #778191 - Flags: review?(bzbarsky)
Comment on attachment 778191 [details] [diff] [review]
patch

r=me
Attachment #778191 - Flags: review?(bzbarsky) → review+
Assignee

Comment 14

6 years ago
Not a single nit even. Boy am I proud of myself ;)
Keywords: checkin-needed
Not a single Try link even. Boy am I nervous ;)

https://hg.mozilla.org/integration/mozilla-inbound/rev/83f4f6f435cf
Flags: in-testsuite+
Keywords: checkin-needed
Also, shouldn't this have had an IID rev? It sure caused needs-clobber bustage.
Assignee

Comment 18

6 years ago
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
Assignee

Comment 19

6 years ago
Still trying to get a feel for what's proper use of the try server and what's overuse...
You shouldn't need an explicit finish() call in this test.
Assignee

Comment 21

6 years ago
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...
Assignee

Comment 22

6 years ago
Posted patch PatchSplinter Review
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.
Attachment #778191 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
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.
Assignee

Comment 25

6 years ago
Boris, here is a fix, added waitForExplicitFinish() call. Review?
Attachment #781191 - Flags: review?(bzbarsky)
Comment on attachment 781191 [details] [diff] [review]
Patch fix - added waitForExplicitFinish call

r=me
Attachment #781191 - Flags: review?(bzbarsky) → review+
Assignee

Comment 27

6 years ago
please checkin "Patch fix - added waitForExplicitFinish call"
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e499b8afbdc6

\o/
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.