77931 – Element should be able to have multiple shadow roots.

RESOLVED FIXED Bug 77931

Element should be able to have multiple shadow roots.

https://bugs.webkit.org/show_bug.cgi?id=77931

Summary Element should be able to have multiple shadow roots.

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	Details Formatted Diff Diff
Test (19.73 KB, patch) 2012-02-16 20:05 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Test (19.73 KB, patch) 2012-02-16 20:32 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (21.02 KB, patch) 2012-02-16 23:48 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (31.36 KB, patch) 2012-02-19 18:29 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (47.06 KB, patch) 2012-02-20 02:32 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (45.63 KB, patch) 2012-02-23 01:50 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (38.11 KB, patch) 2012-02-23 22:22 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (38.21 KB, patch) 2012-02-23 23:03 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (38.06 KB, patch) 2012-02-24 00:08 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Rebased ToT (32.75 KB, patch) 2012-02-28 01:01 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Shinya Kawanaka

Reported

2012-02-06 20:52 PST

Modified

2012-02-28 03:37 PST History

CC List

10 users Show

URL

Keywords

Duplicates (1)

78474 View as bug list

Depends on

77935 78069 78313 78453 78465 79071

Blocks

77503 78474

Dependancies

tree graph