1425759 - Make Shadow DOM stop using XBL entirely.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

So, I was implementing this ::slotted selector, and I just realized how broken all this XBL stuff is, so this is not going to be optional at all.

Consider the following trivial test-case:

<my-host>
  <div id="slotted" class="green">Should be green</div>
</my-host>
<script>
  let shadowHost = document.querySelector('my-host');
  let shadowRoot = shadowHost.attachShadow({mode: 'open'});
  shadowRoot.innerHTML = '<slot></slot><style> ::slotted(.green), ::slotted(myelem) { color:green; } </style>';
</script>

<my-host> doesn't have an XBL binding when #slotted is bound to the tree, so the XBL binding parent is not there.

We hackily attach an XBL binding to <my-host> when the shadow root is created. However, we don't update the binding parent down the tree. We can just walk the tree down and such, but it feels enough of a hack that I think we should avoid it, and ditch the XBL stuff. It's the right thing to do anyway.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This doesn't really block ::slotted. What's going on is that ::slotted is weird as hell, and I have a fix.

Comment 2

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

emilio, so you think we really should fix this before shipping, right?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #2)
> emilio, so you think we really should fix this before shipping, right?

This causes all sorts of trouble everywhere (the XBL leak is just one example), so I'm not sure we can ship without this, yeah...

Actually... The thing that really affects correctness is bug 1418159, which means that if a document doesn't have a shell, suddenly we don't get proper ShadowRoot styles and we get lost in all sorts of funky ways. This is kinda broken on XBL already, but there's no good way to fix it, and it's not problematic usually since we do all the binding stuff on frame construction.

This bug and that one are probably interdependent (in the sense that to remove the layout stuff from XBL I need somewhere better to put them, which right now doesn't exist, but will exist with bug 1418159).

I don't think we should fix bug 1418159 completely to be able to ship, but I think once I get enough of bug 1418159 landed so that I can fix the correctness issues, then this becomes trivial, and we can just do it which is nice.

Bug 1418159 would be easier to do with the old style system removed, I guess... But probably that's just too much to ask for, and I think I can work around it anyway (it's just slightly annoying, because StyleSets would have different lifetimes depending on the style backend the document uses, but oh well, I think I know where to put those bits...).

Comment 4

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

Yeah, I think bug 1418159 shouldn't be blocker if possible.

That XBL leak could be fixed, I just stopped looking at it, since it was unclear whether it would be needed at all (because of this bug).
But how much work it is to support all the Shadow DOM without XBL? DOM part is of course trivial, so I'm concerned about layout.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #4)
> That XBL leak could be fixed, I just stopped looking at it, since it was
> unclear whether it would be needed at all (because of this bug).
> But how much work it is to support all the Shadow DOM without XBL? DOM part
> is of course trivial, so I'm concerned about layout.

I don't think it's much, modulo the StyleSheet bits, for which I need parts of the work for bug 1418159.

Everything other than that that works due to XBL should be trivial to support, and indeed I'm removing the XBL special-cases from the frame constructor slowly, and I think fixing ChildIterator should be enough after the changes from bug 1427677 I think.

There's other broken stuff like counters (like bug 1414303), but that's broken regardless of XBL.

I could try to hack around a way to evaluate media queries without a pres context and hook it up in DocumentOrShadowRoot next week, then try to remove the XBL binding.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425759: Simplify the style backend type setup.

Now that what we use to decide whether a document is styled by Servo are only
prefs and the doc principal, we don't need to inherit the style backend type,
since unless the pref has changed, the result will be the same.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425759: Make Shadow DOM Stylo-only.

I don't plan to fix this stuff up on the old style system.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425759: Make ChildIterator handle Shadow DOM explicitly.

I'll rename mIsXBLInvolved in a bit, since I plan to do more changes here in
bug 1422653.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425759: Make the simplified children iterator check account for Shadow DOM on its own.

We are relying in the XBL binding for the shadow host, we need to check
explicitly.

Also, we were checking in_shadow_tree, which is really broad. Just
is_html_slot_element should do, since the rest of the nodes should have only
light tree children.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425759: Make Shadow DOM not use XBL anymore.

More improvements to come. In particular, this still iterates through Shadow DOM
in each_xbl_cascade_data, but that should be changed later. That allows to
cleanup a bunch of stuff and finally fix Shadow DOM cascade order.

We still rely on the binding parent to be setup properly in the shadow tree, but
that requirement can go away later (we can walk the containing shadow chain
instead).

This mostly focuses on removing the XBL binding from the Shadow host.

It'd be nice to do EnumerateShadowRoots faster. I think that should also be a
followup, if needed.

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

Comment 11

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

Comment on attachment 8953840 [details]
Bug 1425759: Make Shadow DOM Stylo-only.

https://reviewboard.mozilla.org/r/223026/#review228882

Comment 12

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

Comment on attachment 8953841 [details]
Bug 1425759: Make ChildIterator handle Shadow DOM explicitly.

https://reviewboard.mozilla.org/r/223028/#review228884

::: dom/base/ChildIterator.cpp:131
(Diff revision 1)
>    if (aIgnoreXBL) {
>      return;
>    }
>  
> +  // TODO(emilio): I think it probably makes sense to only allow constructing
> +  // FlattenedChildIterators with Element.

I may disagree or may not. For consistency it is often handy if one can just pass nsINode or nsIContent. But if all the callers actually have Element, then fine.

::: dom/base/ChildIterator.cpp:135
(Diff revision 1)
> +  // TODO(emilio): I think it probably makes sense to only allow constructing
> +  // FlattenedChildIterators with Element.
> +  if (mParent->IsElement()) {
> +    if (ShadowRoot* shadow = mParent->AsElement()->GetShadowRoot()) {
> +      mParent = shadow;
> +      mXBLInvolved = true;

ok, mXBLInvolved will be renamed/changed elsewhere.

Comment 13

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

Comment on attachment 8953843 [details]
Bug 1425759: Make Shadow DOM not use XBL anymore.

https://reviewboard.mozilla.org/r/223032/#review228886

::: commit-message-ffaa8:7
(Diff revision 1)
> +
> +More improvements to come. In particular, this still iterates through Shadow DOM
> +in each_xbl_cascade_data, but that should be changed later. That allows to
> +cleanup a bunch of stuff and finally fix Shadow DOM cascade order.
> +
> +We still rely on the binding parent to be setup properly in the shadow tree, but

Yeah, keeping binding parent can be rather nice, since it lets us to have some consistency between NAC, XBL and ShadowDOM

::: layout/inspector/InspectorUtils.cpp:252
(Diff revision 1)
>        // do not apply to the element directly in those cases, their
>        // rules may still show up in the list we get above due to the
>        // inheritance in cascading.
>      }
>  
> +    // Now shadow DOM stuff...

(xidorn should review the style stuff like this.)

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8953842 [details]
Bug 1425759: Make the simplified children iterator check account for Shadow DOM on its own.

https://reviewboard.mozilla.org/r/223030/#review228892

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8953839 [details]
Bug 1425759: Simplify the style backend type setup.

https://reviewboard.mozilla.org/r/223024/#review228896

So I guess the key difference between now and the state of the world circa bug 1389300 comment 16 is that we no longer use the docshell in our backend calculation, so XHR documents should always get the same backend as the document that instantiates them.

Nice cleanup!

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8953843 [details]
Bug 1425759: Make Shadow DOM not use XBL anymore.

https://reviewboard.mozilla.org/r/223032/#review228894

I think the impl of `EnumerateShadowRoots` is a problem we should block on for this patch, given that it can slow down every document whenever the pref is on. This can impact people who is willing to test Shadow DOM for us. We should find a better way (which I have no idea offhand) before landing this, I think, unless you can convince me either this isn't worse than the current situation, or no one is expected to be testing Shadow DOM at all before we fix it.

Otherwise this patch looks fine.

::: dom/base/ShadowRoot.cpp:236
(Diff revision 1)
>    for (size_t i = 0; i <= SheetCount(); i++) {
>      if (i == SheetCount()) {
>        AppendStyleSheet(*aSheet);
> -      mProtoBinding->AppendStyleSheet(aSheet);
> +      Servo_AuthorStyles_AppendStyleSheet(mServoStyles.get(), aSheet->AsServo());
>        break;
>      }
>  
> -    nsINode* sheetOwningNode = SheetAt(i)->GetOwnerNode();
> +    StyleSheet* sheet = SheetAt(i);
> +    nsINode* sheetOwningNode = sheet->GetOwnerNode();
>      if (nsContentUtils::PositionIsBefore(aLinkingContent, sheetOwningNode)) {
>        InsertSheetAt(i, *aSheet);
> -      mProtoBinding->InsertStyleSheetAt(i, aSheet);
> +      Servo_AuthorStyles_InsertStyleSheetBefore(
> +        mServoStyles.get(), aSheet->AsServo(), sheet->AsServo());
>        break;
>      }
>    }

Hmmm... tree position comparison could be more expensive, so this process should probably always be using binary search rather than a linear search. Even if using a linear search, it might be worth having a fast path checking whether we can append directly. But it's unrelated to this bug, anyway.

::: layout/style/ServoStyleSet.cpp:191
(Diff revision 1)
> +    aCb(*shadowRoot);
> +    EnumerateShadowRootsInSubtree(*shadowRoot, aCb);
> +  }
> +}
> +
> +// FIXME(emilio): We may want a faster way to do this.

This is enumerating every node in the document when the pref is on?! This definitely needs to change, especially given that this is on the path of MediumFeaturesChange and DocumentStateChanges, which can happen reasonably often.

I would consider this a blocker of this patch, actually, since this would likely significantly regress performance whoever tries to testing shadow dom.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425759: Make a style sharing check account for Shadow DOM explicitly too.

Good I added tests for this (style-sharing-across-shadow.html).

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

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425759: Update (most) test expectations.

This is enough to get the stylo-enabled build green.

There's still some orange in WPT with stylo disabled (due to interfaces not
exposed and that) that I'll update tomorrow.

Will send a different patch on top of this for that, though I'll land together.

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

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953843 [details]
Bug 1425759: Make Shadow DOM not use XBL anymore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223032/diff/1-2/

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8953893 [details]
Bug 1425759: Make a style sharing check account for Shadow DOM explicitly too.

https://reviewboard.mozilla.org/r/223048/#review228904

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953843 [details]
Bug 1425759: Make Shadow DOM not use XBL anymore.

https://reviewboard.mozilla.org/r/223032/#review228894

> This is enumerating every node in the document when the pref is on?! This definitely needs to change, especially given that this is on the path of MediumFeaturesChange and DocumentStateChanges, which can happen reasonably often.
> 
> I would consider this a blocker of this patch, actually, since this would likely significantly regress performance whoever tries to testing shadow dom.

I think walking the DOM with GetNextNode is not too bad (it's a fully-inline path, so it's pretty fast). It's effectively the same `querySelector` does. I don't think it's terrible enough to block on this, and the logic to track all the Shadow roots in the document seemed hairy enough to be worth doing it on a separate bug.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8953843 [details]
Bug 1425759: Make Shadow DOM not use XBL anymore.

https://reviewboard.mozilla.org/r/223032/#review228908

emilio on the IRC said node traversing is fast enough in practice (10ms for single page HTML spec, according to him), so it may not be a that big concern. Also he said fixing this is in his plan, but non-trivial, so we probably shouldn't block on this. I guess it probably makes sense as far as Shadow DOM is still disabled by default.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8953894 [details]
Bug 1425759: Update (most) test expectations.

https://reviewboard.mozilla.org/r/223050/#review228910

r=me with the nit fixed.

::: dom/html/test/mochitest.ini:443
(Diff revision 1)
>  [test_formSubmission2.html]
>  skip-if = toolkit == 'android'
>  [test_formelements.html]
>  [test_fullscreen-api.html]
>  tags = fullscreen
> -skip-if = toolkit == 'android'
> +skip-if = toolkit == 'android' || !stylo

What's wrong with fullscreen?

If it's because of the subtest file_fullscreen-shadowdom.html, we should just skip that subtest in test_fullscreen-api.html when stylo is not enabled.

Comment 24

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

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #22)
> Comment on attachment 8953843 [details]
> Bug 1425759: Make Shadow DOM not use XBL anymore.
> 
> https://reviewboard.mozilla.org/r/223032/#review228908
> 
> emilio on the IRC said node traversing is fast enough in practice (10ms for
> single page HTML spec, according to him),
10ms is a lot

> Also he said fixing this is in his plan, but non-trivial, so we
> probably shouldn't block on this. I guess it probably makes sense as far as
> Shadow DOM is still disabled by default.
ok, need to file a followup then which blocks Shadow DOM layout meta bug.

Comment 25

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

Comment on attachment 8953894 [details]
Bug 1425759: Update (most) test expectations.

https://reviewboard.mozilla.org/r/223050/#review229058

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425759: Update the rest of the test expectations.

Mostly disabling stuff where Shadow DOM is disabled for non-stylo. The old style
system will go away soon enough I don't think it's worth to keep both.

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

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953839 [details]
Bug 1425759: Simplify the style backend type setup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223024/diff/1-2/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953840 [details]
Bug 1425759: Make Shadow DOM Stylo-only.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223026/diff/1-2/

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953841 [details]
Bug 1425759: Make ChildIterator handle Shadow DOM explicitly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223028/diff/1-2/

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953842 [details]
Bug 1425759: Make the simplified children iterator check account for Shadow DOM on its own.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223030/diff/1-2/

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953893 [details]
Bug 1425759: Make a style sharing check account for Shadow DOM explicitly too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223048/diff/1-2/

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953843 [details]
Bug 1425759: Make Shadow DOM not use XBL anymore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223032/diff/2-3/

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953894 [details]
Bug 1425759: Update (most) test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223050/diff/1-2/

Comment 34

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

Comment on attachment 8954043 [details]
Bug 1425759: Update the rest of the test expectations.

https://reviewboard.mozilla.org/r/223194/#review229332

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d24a4454d076
Simplify the style backend type setup. r=bholley
https://hg.mozilla.org/integration/autoland/rev/db24e045b5db
Make Shadow DOM Stylo-only. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a18fd12a7eae
Make ChildIterator handle Shadow DOM explicitly. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2adcf8c3483a
Make Shadow DOM not use XBL anymore. r=smaug,xidorn
https://hg.mozilla.org/integration/autoland/rev/d8ff7d316f70
Update test expectations. r=smaug,xidorn

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20ef5c2e4329
Mark a devtools test as temporarily failing, pending resolution of bug 1441535. r=me on a CLOSED TREE

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d24a4454d076
https://hg.mozilla.org/mozilla-central/rev/db24e045b5db
https://hg.mozilla.org/mozilla-central/rev/a18fd12a7eae
https://hg.mozilla.org/mozilla-central/rev/2adcf8c3483a
https://hg.mozilla.org/mozilla-central/rev/d8ff7d316f70
https://hg.mozilla.org/mozilla-central/rev/20ef5c2e4329