Bug 77931

Summary: Element should be able to have multiple shadow roots.
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, gustavo, hayato, morrita, pnormand, rolandsteiner, shinyak, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 77935, 78069, 78313, 78453, 78465, 79071    
Bug Blocks: 77503, 78474    
Attachments:
Description Flags
Patch
none
Test
none
Test
none
Patch
none
Patch
none
WIP
none
WIP
none
Patch
none
Patch
none
Patch
none
Rebased ToT none

Shinya Kawanaka
Reported 2012-02-06 20:52:48 PST
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.
Attachments
Patch (16.82 KB, patch)
2012-02-16 18:42 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Test (19.73 KB, patch)
2012-02-16 20:05 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Test (19.73 KB, patch)
2012-02-16 20:32 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (21.02 KB, patch)
2012-02-16 23:48 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (31.36 KB, patch)
2012-02-19 18:29 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
WIP (47.06 KB, patch)
2012-02-20 02:32 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
WIP (45.63 KB, patch)
2012-02-23 01:50 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (38.11 KB, patch)
2012-02-23 22:22 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (38.21 KB, patch)
2012-02-23 23:03 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (38.06 KB, patch)
2012-02-24 00:08 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Rebased ToT (32.75 KB, patch)
2012-02-28 01:01 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2012-02-16 18:42:59 PST
Created attachment 127493 [details] Patch
Philippe Normand
Comment 2 2012-02-16 19:04:44 PST
Comment on attachment 127493 [details] Patch Attachment 127493 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11538318
Hajime Morrita
Comment 3 2012-02-16 19:51:16 PST
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.
Shinya Kawanaka
Comment 4 2012-02-16 20:05:46 PST
Created attachment 127502 [details] Test
Philippe Normand
Comment 5 2012-02-16 20:12:21 PST
Comment on attachment 127502 [details] Test Attachment 127502 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11539342
Shinya Kawanaka
Comment 6 2012-02-16 20:32:13 PST
Created attachment 127507 [details] Test
Shinya Kawanaka
Comment 7 2012-02-16 23:48:14 PST
Created attachment 127535 [details] Patch
Hajime Morrita
Comment 8 2012-02-17 03:16:49 PST
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.
Shinya Kawanaka
Comment 9 2012-02-19 18:29:48 PST
Created attachment 127748 [details] Patch
Hajime Morrita
Comment 10 2012-02-19 18:51:45 PST
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?
Shinya Kawanaka
Comment 11 2012-02-19 21:21:08 PST
It turned out that this patch does not work well if dynamically a new shadow root is attached... Let me take time to fix.
Shinya Kawanaka
Comment 12 2012-02-20 01:38:50 PST
(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?
Shinya Kawanaka
Comment 13 2012-02-20 02:32:18 PST
Created attachment 127787 [details] WIP
WebKit Review Bot
Comment 14 2012-02-20 06:02:51 PST
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
Shinya Kawanaka
Comment 15 2012-02-23 01:50:26 PST
Created attachment 128434 [details] WIP
Shinya Kawanaka
Comment 16 2012-02-23 22:22:12 PST
Created attachment 128651 [details] Patch
Shinya Kawanaka
Comment 17 2012-02-23 23:03:31 PST
Created attachment 128659 [details] Patch
Hajime Morrita
Comment 18 2012-02-23 23:32:35 PST
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
Shinya Kawanaka
Comment 19 2012-02-24 00:03:15 PST
(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.
Shinya Kawanaka
Comment 20 2012-02-24 00:08:39 PST
Created attachment 128670 [details] Patch
Shinya Kawanaka
Comment 21 2012-02-26 23:57:09 PST
*** Bug 78474 has been marked as a duplicate of this bug. ***
Shinya Kawanaka
Comment 22 2012-02-28 01:01:17 PST
Created attachment 129212 [details] Rebased ToT
WebKit Review Bot
Comment 23 2012-02-28 03:36:55 PST
Comment on attachment 129212 [details] Rebased ToT Clearing flags on attachment: 129212 Committed r109096: <http://trac.webkit.org/changeset/109096>
WebKit Review Bot
Comment 24 2012-02-28 03:37:02 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.