reprocess <style> elements in shadow trees of elements just bound to the document

RESOLVED FIXED in Firefox 61

Status

()

RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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

5 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

5 years ago
Created attachment 8440477 [details] [diff] [review]
patch
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8440477 - Flags: review?(bzbarsky)
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)
Let me ask another question.  If a <style> is inserted into a ShadowRoot _after_ the host is in the document, what makes _that_ work?
(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)
> 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 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)
I think the plan now is to have the BindToTree/UnbindFromTree calls propagate into the Shadow DOM. This is being tracked by bug 1062578.
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)
Attachment #8440477 - Flags: review?(bzbarsky)
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)
Duplicate of this bug: 1056783
(Assignee)

Comment 13

4 years ago
Cool.  Should we land the reftests in this bug, or are they already covered by bug 1062578?
We should land the reftests in this bug.
Flags: needinfo?(cam)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

8 months 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

8 months 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

8 months ago
(Meant to direct the review to Emilio, hence my comment 16.)
(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

8 months 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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fd8b1659a4ab
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months 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.