Closed
Bug 734861
Opened 13 years ago
Closed 11 years ago
Expose stylesheet collection (including UA) for a document via nsIStylesheetService
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: miker, Assigned: almasry.mina)
References
Details
(Whiteboard: [mentor=bz][lang=c++])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
> 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?
Reporter | ||
Comment 2•13 years ago
|
||
(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•12 years ago
|
||
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++]
Reporter | ||
Comment 4•12 years ago
|
||
(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)
Comment 5•12 years ago
|
||
Hmm. There is no such property on sheets right now, but maybe we should add one. Cameron?
Flags: needinfo?(cam)
Comment 6•12 years ago
|
||
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•12 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•11 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
Comment 9•11 years ago
|
||
> 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.
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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•11 years ago
|
||
Changes made. Here is an updated patch.
Attachment #777638 -
Attachment is obsolete: true
Attachment #778191 -
Flags: review?(bzbarsky)
Comment 13•11 years ago
|
||
Comment on attachment 778191 [details] [diff] [review]
patch
r=me
Attachment #778191 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 14•11 years ago
|
||
Not a single nit even. Boy am I proud of myself ;)
Keywords: checkin-needed
Comment 15•11 years ago
|
||
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
Comment 16•11 years ago
|
||
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•11 years ago
|
||
Also, shouldn't this have had an IID rev? It sure caused needs-clobber bustage.
Assignee | ||
Comment 18•11 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•11 years ago
|
||
Still trying to get a feel for what's proper use of the try server and what's overuse...
Comment 20•11 years ago
|
||
You shouldn't need an explicit finish() call in this test.
Assignee | ||
Comment 21•11 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•11 years ago
|
||
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•11 years ago
|
Keywords: checkin-needed
Comment 23•11 years ago
|
||
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
Comment 24•11 years ago
|
||
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•11 years ago
|
||
Boris, here is a fix, added waitForExplicitFinish() call. Review?
Attachment #781191 -
Flags: review?(bzbarsky)
Comment 26•11 years ago
|
||
Comment on attachment 781191 [details] [diff] [review]
Patch fix - added waitForExplicitFinish call
r=me
Attachment #781191 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 27•11 years ago
|
||
please checkin "Patch fix - added waitForExplicitFinish call"
Keywords: checkin-needed
Comment 28•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 29•11 years ago
|
||
Comment 30•11 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•