Before supporting multiple shadow subtrees in all elements, let's support it in the case an element doesn't have a built-in shadow root, because a built-in shadow root may be created dynamically. After refactoring elements having a built-in shadow root, let's turn multiple shadow root ON for any elements.
Created attachment 127493 [details] Patch
Comment on attachment 127493 [details] Patch Attachment 127493 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11538318
Comment on attachment 127493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127493&action=review I think we should land rendering related changes at the same time even if the patch will be bigger. Without rendering, I cannot see if the patch is correct or not. > LayoutTests/fast/dom/shadow/multiple-shadowroot.html:21 > +shouldBe("internals.address(internals.shadowRoot(div))", "internals.address(shadowRoot1)"); We should make "===" work for ShadowRoot instances instead of relying address(). Having address() on Internals object is OK because it's useful for trouble shooting. But I don't think use it for testing is good idea.
Created attachment 127502 [details] Test
Comment on attachment 127502 [details] Test Attachment 127502 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11539342
Created attachment 127507 [details] Test
Created attachment 127535 [details] Patch
Comment on attachment 127535 [details] Patch We need some test to examine how multiple shadows are rendered/layouted. It doesn't need to be Ultra-comprehensive. But it covers some basic patterns.
Created attachment 127748 [details] Patch
Comment on attachment 127748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127748&action=review > Source/WebCore/dom/Element.cpp:1204 > + root->detach(); It this order intentional? This means that newly added ShadowRoot is lazy-attached then detached. Also, how we can ensure these shadow root attached? I'm confusing. Maybe we need some way to investite attach-ness using internals API. > LayoutTests/fast/dom/shadow/multiple-shadowroot-rendering.html:1 > +<!DOCTYPE html> This test is Good! Could you add some dynamic attaching case?
It turned out that this patch does not work well if dynamically a new shadow root is attached... Let me take time to fix.
(In reply to comment #10) > (From update of attachment 127748 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127748&action=review > > > Source/WebCore/dom/Element.cpp:1204 > > + root->detach(); > > It this order intentional? > This means that newly added ShadowRoot is lazy-attached then detached. > Also, how we can ensure these shadow root attached? I'm confusing. > Maybe we need some way to investite attach-ness using internals API. Actually detach()-ing non youngest shadow roots. > > LayoutTests/fast/dom/shadow/multiple-shadowroot-rendering.html:1 > > +<!DOCTYPE html> > > This test is Good! > Could you add some dynamic attaching case?
Created attachment 127787 [details] WIP
Comment on attachment 127787 [details] WIP Attachment 127787 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11548282 New failing tests: fast/html/details-remove-summary-5-and-click.html fast/html/details-add-details-child-1.html fast/html/details-remove-summary-5.html fast/html/details-add-summary-5-and-click.html fast/html/details-add-child-1.html fast/html/details-add-summary-4.html http/tests/inspector/inspect-element.html fast/html/details-remove-summary-6.html fast/html/details-remove-summary-3.html fast/html/details-click-controls.html fast/html/details-add-summary-5.html css2.1/20110323/abspos-containing-block-initial-004d.htm fast/html/details-remove-child-2.html fast/html/details-add-summary-10.html fast/html/details-add-summary-10-and-click.html fast/html/details-add-summary-9.html fast/html/details-remove-summary-2.html fast/html/details-add-child-2.html fast/html/details-remove-child-1.html css2.1/20110323/abspos-containing-block-initial-004b.htm fast/block/block-remove-child-delete-line-box-crash.html fast/dom/shadow/multiple-shadowroot-rendering.html accessibility/aria-describedby-on-input.html fast/html/details-remove-summary-2-and-click.html fast/html/details-replace-text.html fast/html/details-remove-summary-6-and-click.html fast/html/details-add-summary-9-and-click.html fast/html/details-remove-summary-3-and-click.html fast/html/details-add-summary-4-and-click.html
Created attachment 128434 [details] WIP
Created attachment 128651 [details] Patch
Created attachment 128659 [details] Patch
Comment on attachment 128659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128659&action=review > Source/WebCore/dom/Element.cpp:79 > +#if ENABLE(SHADOW_DOM) Don't need this ENABLE because this file is always available and doesn't affect the behaviour. > Source/WebCore/dom/Element.cpp:1193 > +#endif This is pretty confusing - mysterious #ifdef and "Add"-ing a shadow results removal. Could you clarify around this somehow? > Source/WebCore/dom/Element.cpp:1202 > + for (ShadowRoot* root = shadowRootList()->youngestShadowRoot()->olderShadowRoot(); root; root = root->olderShadowRoot()) Looks like a method of ShadowRootList. > Source/WebCore/dom/Element.cpp:1231 > + oldRoot->setNext(0); Could you make this series of call a method? > Source/WebCore/dom/NodeRenderingContext.cpp:62 > + if (toShadowRoot(parent)->youngerShadowRoot()) Why not isYoungest ? > Source/WebCore/dom/NodeRenderingContext.cpp:78 > + if (!toShadowRoot(m_insertionPoint->shadowTreeRootNode())->youngerShadowRoot()) { Why not use isYoungestShadowRoot()? > Source/WebCore/dom/ShadowRoot.h:76 > + bool isYoungestShadowRoot() const { return !youngerShadowRoot(); } Drop the "ShadowRoot", just "isYoungest". It's cleaner. > Source/WebCore/dom/ShadowRoot.h:77 > + bool isOldestShadowRoot() const { return !olderShadowRoot(); } Ditto. > LayoutTests/fast/dom/shadow/multiple-shadowroot-rendering.html:160 > + document.getElementById('actual-container').appendChild(root); Coud you force layout here to make the intention clearer? > LayoutTests/fast/dom/shadow/multiple-shadowroot-rendering.html:174 > +</html> Could you add dynamic removal case? - Dynamic removal for the yougest tree - Dynamic removel for non-youngest tree
(In reply to comment #18) > (From update of attachment 128659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128659&action=review > > > Source/WebCore/dom/Element.cpp:79 > > +#if ENABLE(SHADOW_DOM) > > Don't need this ENABLE because this file is always available and doesn't affect the behaviour. Done. > > > Source/WebCore/dom/Element.cpp:1193 > > +#endif > > This is pretty confusing - mysterious #ifdef and "Add"-ing a shadow results removal. > Could you clarify around this somehow? Hmm... I'm now thinking just adding ASSERT is OK. It should be checked in ShadowRoot::create. > > > Source/WebCore/dom/Element.cpp:1202 > > + for (ShadowRoot* root = shadowRootList()->youngestShadowRoot()->olderShadowRoot(); root; root = root->olderShadowRoot()) > > Looks like a method of ShadowRootList. Yeah... We should have pushed a shadow root into the shadow root list after detaching older shadow roots. > > > Source/WebCore/dom/Element.cpp:1231 > > + oldRoot->setNext(0); > > Could you make this series of call a method? We should have done it in ShadowRootList::popShadowRoot(). > > > Source/WebCore/dom/NodeRenderingContext.cpp:62 > > + if (toShadowRoot(parent)->youngerShadowRoot()) > > Why not isYoungest ? Done. > > > Source/WebCore/dom/NodeRenderingContext.cpp:78 > > + if (!toShadowRoot(m_insertionPoint->shadowTreeRootNode())->youngerShadowRoot()) { > > Why not use isYoungestShadowRoot()? Done. > > > Source/WebCore/dom/ShadowRoot.h:76 > > + bool isYoungestShadowRoot() const { return !youngerShadowRoot(); } > > Drop the "ShadowRoot", just "isYoungest". It's cleaner. Done. > > > Source/WebCore/dom/ShadowRoot.h:77 > > + bool isOldestShadowRoot() const { return !olderShadowRoot(); } > > Ditto. Done. > > > LayoutTests/fast/dom/shadow/multiple-shadowroot-rendering.html:160 > > + document.getElementById('actual-container').appendChild(root); > > Coud you force layout here to make the intention clearer? Actually we force layout using setTImeout in callIdDone(). > > > LayoutTests/fast/dom/shadow/multiple-shadowroot-rendering.html:174 > > +</html> > > Could you add dynamic removal case? > - Dynamic removal for the yougest tree > - Dynamic removel for non-youngest tree Actually we don't have a method to such removal methods. We only support removing all shadow subtrees, and it is used for testing.
Created attachment 128670 [details] Patch
*** Bug 78474 has been marked as a duplicate of this bug. ***
Created attachment 129212 [details] Rebased ToT
Comment on attachment 129212 [details] Rebased ToT Clearing flags on attachment: 129212 Committed r109096: <http://trac.webkit.org/changeset/109096>
All reviewed patches have been landed. Closing bug.