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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Cameron McCormack (:heycam)]

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.

Comment 1

[Cameron McCormack (:heycam)]

This is because HTMLStyleElement::BindToTree calls nsStyleLinkElement::UpdateStyleSheetInternal, but that only creates a style sheet if we are in a document.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 3

[Boris Zbarsky [: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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

[William Chen [:wchen]]

(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

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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

[Boris Zbarsky [:bzbarsky]]

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...

Comment 8

[William Chen [:wchen]]

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

[Boris Zbarsky [:bzbarsky]]

OK.  Would that just fix this bug automagically?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 11

[William Chen [:wchen]]

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.

Comment 13

[Cameron McCormack (:heycam)]

Cool.  Should we land the reftests in this bug, or are they already covered by bug 1062578?

Comment 14

[Boris Zbarsky [:bzbarsky]]

We should land the reftests in this bug.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Bug 1025724 - Add some shadow tree style tests.

Review commit: https://reviewboard.mozilla.org/r/233944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233944/

Comment 16

[Cameron McCormack (:heycam)]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8965233 [details]
Bug 1025724 - Add some shadow tree style tests.

https://reviewboard.mozilla.org/r/233944/#review239940

Comment 18

[Cameron McCormack (:heycam)]

(Meant to direct the review to Emilio, hence my comment 16.)

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd8b1659a4ab
Add some shadow tree style tests. r=bz

Comment 21

[Noemi Erli[:noemi_erli]]

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