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.

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

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

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

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.

(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 on attachment 8953840 [details] Bug 1425759: Make Shadow DOM Stylo-only. https://reviewboard.mozilla.org/r/223026/#review228882

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

(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 on attachment 8953894 [details] Bug 1425759: Update (most) test expectations. https://reviewboard.mozilla.org/r/223050/#review229058

Comment on attachment 8954043 [details] Bug 1425759: Update the rest of the test expectations. https://reviewboard.mozilla.org/r/223194/#review229332

Pushed by ecoal95@gmail.com: 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

Pushed by ecoal95@gmail.com: 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

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