Closed
Bug 1025724
Opened 10 years ago
Closed 7 years ago
reprocess <style> elements in shadow trees of elements just bound to the document
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
Details
Attachments
(2 files)
13.81 KB,
patch
|
Details | Diff | Splinter Review | |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
Currently if a <style> element is inserted into a ShadowRoot before the ShadowRoot's host is inserted into the document, we won't create a style sheet for the <style> element.
Assignee | ||
Comment 1•10 years ago
|
||
This is because HTMLStyleElement::BindToTree calls nsStyleLinkElement::UpdateStyleSheetInternal, but that only creates a style sheet if we are in a document.
Assignee | ||
Comment 2•10 years ago
|
||
Comment 3•10 years ago
|
||
Hrm. Do we not rebind (in the BindToTree sense) the anonymous content when the shadow host's parent chain changes? That's likely to break other things too, no? Things like iframes in the shadow tree come to mind.
Flags: needinfo?(wchen)
Comment 4•10 years ago
|
||
Let me ask another question. If a <style> is inserted into a ShadowRoot _after_ the host is in the document, what makes _that_ work?
Comment 5•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #3)
> Hrm. Do we not rebind (in the BindToTree sense) the anonymous content when
> the shadow host's parent chain changes? That's likely to break other things
> too, no? Things like iframes in the shadow tree come to mind.
No, we don't rebind anonymous content in the shadow tree right now. Elements in the shadow tree behave similar to as if appended into a document fragment, BindToTree and UnbindFromTree only get called when inserted/removed from the shadow tree.
I guess how we want to handle this depends on whether BindToTree/UnbindFromTree also means BindToComposedTree/UnbindFromComposedTree. It's a little strange for the Shadow DOM to behave like the latter compared to XBL because when the host is unbound, nodes in the shadow tree are unbound from the composed tree but they are not unbound from the shadow tree. In XBL when a host is unbound, node in anonymous content are unbound from the composed tree and due to tear-down they are also unbound from whatever tree they were in.
(In reply to Boris Zbarsky [:bz] from comment #4)
> Let me ask another question. If a <style> is inserted into a ShadowRoot
> _after_ the host is in the document, what makes _that_ work?
http://dxr.mozilla.org/mozilla-central/source/content/base/src/ShadowRoot.cpp#170
Flags: needinfo?(wchen)
Comment 6•10 years ago
|
||
> whether BindToTree/UnbindFromTree also means BindToComposedTree/UnbindFromComposedTree
Right now, for normal content and XBL, BindToTree means binding to composed tree if aDocument is not null. UnbindFromTree always means unbinding from composed tree.
I think we should generally preserve those semantics, ideally. If we do, can we remove the special-casing of style elements in shadow trees we have right now? Seems like at that point the normal style loading/application codepath will do the right thing, modulo the need to scope correctly.
Comment 7•10 years ago
|
||
Comment on attachment 8440477 [details] [diff] [review]
patch
I'd like to understand what our general plan, if any, is for propagating BindToTree to shadow trees. Seems like this is a specific instance of that general plan...
Attachment #8440477 -
Flags: feedback?(wchen)
Attachment #8440477 -
Flags: feedback?(bugs)
Comment 8•10 years ago
|
||
I think the plan now is to have the BindToTree/UnbindFromTree calls propagate into the Shadow DOM. This is being tracked by bug 1062578.
Comment 9•10 years ago
|
||
OK. Would that just fix this bug automagically?
Comment on attachment 8440477 [details] [diff] [review]
patch
Yeah, I don't think we should add this kind of specific hacks, but just
make BindToTree deal with shadow dom
Attachment #8440477 -
Flags: feedback?(bugs)
Updated•10 years ago
|
Attachment #8440477 -
Flags: review?(bzbarsky)
Comment 11•10 years ago
|
||
Comment on attachment 8440477 [details] [diff] [review]
patch
(In reply to Boris Zbarsky [:bz] from comment #9)
> OK. Would that just fix this bug automagically?
I ran the reftests in this patch against the fix in the other bug and it is automagically fixed.
Attachment #8440477 -
Flags: feedback?(wchen)
Assignee | ||
Comment 13•10 years ago
|
||
Cool. Should we land the reftests in this bug, or are they already covered by bug 1062578?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
Emilio, these may be redundant with other tests you've added elsewhere (or we've got from WPT). If so, let me know, and we can drop them.
Flags: needinfo?(cam)
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8965233 [details]
Bug 1025724 - Add some shadow tree style tests.
https://reviewboard.mozilla.org/r/233944/#review239940
Attachment #8965233 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 18•7 years ago
|
||
(Meant to direct the review to Emilio, hence my comment 16.)
Comment 19•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> Comment on attachment 8965233 [details]
> Bug 1025724 - Add some shadow tree style tests.
>
> https://reviewboard.mozilla.org/r/233944/#review239940
Yeah, there are tests for this kind of stuff in the WPT directories css/css-scoping and shadow-dom. But I guess landing these doesn't hurt either.
Comment 20•7 years ago
|
||
Pushed by cam@mcc.id.au:
https://hg.mozilla.org/integration/autoland/rev/fd8b1659a4ab
Add some shadow tree style tests. r=bz
Comment 21•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•