| Summary: | Element should be able to have multiple shadow roots. | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Shinya Kawanaka <shinyak> | ||||||||||||||||||||||||
| Component: | DOM | Assignee: | 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
Shinya Kawanaka
2012-02-06 20:52:48 PST
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. |