99228 – The shadow element is not reprojected to a nested shadowRoot

RESOLVED FIXED 99228

The shadow element is not reprojected to a nested shadowRoot

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

Summary The shadow element is not reprojected to a nested shadowRoot

Steve Orvell

Reported 2012-10-12 18:18:23 PDT

Created attachment 168525 [details] Reduction showing reprojection of shadow element with expected output. If a host has 2 shadowRoots (A-old and A) and a nested shadowRoot (B) and B's light dom contains a shadow element that should display A-old, then A-old is not distributed to an insertion point in B that should accept its contents.

Attachments
Reduction showing reprojection of shadow element with expected output. (750 bytes, text/html) 2012-10-12 18:18 PDT, Steve Orvell	no flags	Details
WIP (14.41 KB, patch) 2012-10-23 02:38 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (9.90 KB, patch) 2012-10-24 00:51 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (16.00 KB, patch) 2012-10-24 03:09 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (19.51 KB, patch) 2012-10-24 18:01 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (28.65 KB, patch) 2012-10-24 23:55 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (37.01 KB, patch) 2012-10-25 16:35 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (39.77 KB, patch) 2012-10-25 18:36 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (63.19 KB, patch) 2012-10-25 19:36 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (43.79 KB, patch) 2012-10-25 19:51 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (43.73 KB, patch) 2012-10-25 21:02 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (46.20 KB, patch) 2012-10-25 21:53 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2012-10-16 22:07:11 PDT

I found that we have to change the current algorithm to reproject <shadow>. Currently, we resolve <content> from the youngest shadow root. However, when we reproject <shadow>, we have to resolve <content> and <shadow> from the oldest shadow root, since we have to know what elements are distributed to <shadow>. This is an example. host ------- SR -------------- SR (younger) |- div (A) |-div (C) |-div -------- SR |- div (B) |-content[B] |-shadow |-content[div]#target |-shadow When we resolve content[div]#target, we have to resolve all the other shadow and content. This might be a drastic spec change.

Dimitri Glazkov (Google)

Comment 2 2012-10-17 12:45:44 PDT

I think we can punt on this for now, possibly forever.

Shinya Kawanaka

Comment 3 2012-10-17 21:48:54 PDT

(In reply to comment #2) > I think we can punt on this for now, possibly forever. When we adopt OOP model to understand reprojection, reprojecting shadow seems like seeing a private field of a base class. (reprojecting content is just passing arguments to super class.) I think it would be better to postpone implementing this until we strongly require this.

Shinya Kawanaka

Comment 4 2012-10-23 00:39:20 PDT

I'm now thinking implementing this is not so hard...

Shinya Kawanaka

Comment 5 2012-10-23 02:38:00 PDT

Created attachment 170094 [details] WIP

Shinya Kawanaka

Comment 6 2012-10-23 02:39:02 PDT

This WIP includes code related to https://bugs.webkit.org/show_bug.cgi?id=99552 also. I've confirmed that distribution works, but we have to fix ComposedShadowTreeWalker also.

Dimitri Glazkov (Google)

Comment 7 2012-10-23 09:04:06 PDT

Comment on attachment 170094 [details] WIP This looks promising!

Shinya Kawanaka

Comment 8 2012-10-24 00:51:37 PDT

Created attachment 170338 [details] WIP

Shinya Kawanaka

Comment 9 2012-10-24 00:52:38 PDT

Now shadow-reprojection works in some case, though we have a lot of failing tests.

Shinya Kawanaka

Comment 10 2012-10-24 00:54:37 PDT

<shadow> in <shadow> does not work again :-/

Shinya Kawanaka

Comment 11 2012-10-24 03:09:32 PDT

Created attachment 170352 [details] WIP

Shinya Kawanaka

Comment 12 2012-10-24 03:11:01 PDT

The last WIP patch passes all the existing fast/dom/shadow tests. I'll add more tests and refine the code tomorrow.

Shinya Kawanaka

Comment 13 2012-10-24 18:01:22 PDT

Created attachment 170527 [details] WIP

Shinya Kawanaka

Comment 14 2012-10-24 18:03:19 PDT

Now writing a composed shadow tree walker test, and it's discovering a lot of failures. I need more time to make it review-ready.

Hayato Ito

Comment 15 2012-10-24 23:25:36 PDT

We have to update the AncestorChainWalker also. AncestorChainWalker::parent() if (!m_node->isShadowRoot()) { m_node = m_node->parentNode(); (*) m_distributedNode = m_node; m_isCrossingInsertionPoint = false; return; } (*) Here, we should preserve m_node as a m_distributed node before assigning m_node->parentNode() to m_node if the following condition is met: m_node->parenetNode()->isShadowRoot() && toShadowRoot(m_node->parentNode())->isAssignedTo() since m_node might be re-projected. So we should keep it as a m_distributed_node. Let me take a look at other places also. I am afraid that we have to update other places. (In reply to comment #14) > Now writing a composed shadow tree walker test, and it's discovering a lot of failures. I need more time to make it review-ready.

Shinya Kawanaka

Comment 16 2012-10-24 23:55:47 PDT

Created attachment 170566 [details] WIP

Hayato Ito

Comment 17 2012-10-24 23:57:52 PDT

I've taken a look at EventDispatcher::ensureEventAncestors. I expect that we don't have to update EventDispatcher as long as we can calculate ancestors chain in AncestorChainWalker correctly. (In reply to comment #15) > We have to update the AncestorChainWalker also. > > AncestorChainWalker::parent() > > if (!m_node->isShadowRoot()) { > m_node = m_node->parentNode(); (*) > m_distributedNode = m_node; > m_isCrossingInsertionPoint = false; > return; > } > > (*) Here, we should preserve m_node as a m_distributed node before assigning m_node->parentNode() to m_node if the following condition is met: > > m_node->parenetNode()->isShadowRoot() && toShadowRoot(m_node->parentNode())->isAssignedTo() > > since m_node might be re-projected. So we should keep it as a m_distributed_node. > > Let me take a look at other places also. I am afraid that we have to update other places. > > > > (In reply to comment #14) > > Now writing a composed shadow tree walker test, and it's discovering a lot of failures. I need more time to make it review-ready.

Shinya Kawanaka

Comment 18 2012-10-24 23:58:32 PDT

> We have to update the AncestorChainWalker also. > Done. What we have to fix now is: - ComposedShadowTreeWalker::nextSibling(), previousSibling(). If you find other places where we have to fix, please tell me.

Early Warning System Bot

Comment 19 2012-10-24 23:59:53 PDT

Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14593126

Early Warning System Bot

Comment 20 2012-10-25 00:01:39 PDT

Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14555512

EFL EWS Bot

Comment 21 2012-10-25 00:27:56 PDT

Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14551008

WebKit Review Bot

Comment 22 2012-10-25 00:35:02 PDT

Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14566506

Peter Beverloo (cr-android ews)

Comment 23 2012-10-25 00:45:10 PDT

Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14562497

Hayato Ito

Comment 24 2012-10-25 00:45:56 PDT

Comment on attachment 170566 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=170566&action=review > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:67 > +static inline InsertionPoint* resolveReprojection(const Node* projectedNode, const Node* baseNode) Do we need two parameters? it seems one parameter, projectedNode, is enough. > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:86 > + return resolveShadowProjection(root->assignedTo()); I am afraid that this does not resolve reprojection for shadow. If we resolve reprojection recursively, we need two nodes as state, one is an insertion point and the other is a re-projected node. > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:223 > + InsertionPoint* insertionPoint = resolveReprojection(node, node); One parameter seems to be enough. > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:291 > + if (InsertionPoint* insertionPoint = resolveReprojection(node, node)) { Ditto.

Build Bot

Comment 25 2012-10-25 07:25:28 PDT

Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14554656

Shinya Kawanaka

Comment 26 2012-10-25 16:35:49 PDT

Created attachment 170766 [details] WIP

Shinya Kawanaka

Comment 27 2012-10-25 16:37:23 PDT

It started to pass the tests again.

Shinya Kawanaka

Comment 28 2012-10-25 16:39:25 PDT

Comment on attachment 170766 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=170766&action=review > Source/WebCore/css/StyleResolver.cpp:1547 > + } memo: This is necessary because the direct child of shadow root can be reprojected to somewhere now. In that case, parentElement() will return 0. > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:108 > + This should die.

Shinya Kawanaka

Comment 29 2012-10-25 18:36:11 PDT

Created attachment 170782 [details] WIP

Shinya Kawanaka

Comment 30 2012-10-25 18:41:46 PDT

This version works correctly including ComposedShadowTreeWalker. I'll refine the code.

Shinya Kawanaka

Comment 31 2012-10-25 19:36:43 PDT

Created attachment 170788 [details] Patch

Shinya Kawanaka

Comment 32 2012-10-25 19:51:46 PDT

Created attachment 170792 [details] Patch

Shinya Kawanaka

Comment 33 2012-10-25 19:52:14 PDT

Fixed ChangeLog mojibake

Dimitri Glazkov (Google)

Comment 34 2012-10-25 20:04:42 PDT

(In reply to comment #33) > Fixed ChangeLog mojibake Sounds delicious.

Shinya Kawanaka

Comment 35 2012-10-25 21:02:45 PDT

Created attachment 170801 [details] Patch

Shinya Kawanaka

Comment 36 2012-10-25 21:03:25 PDT

Minor updates: removed a debug code, and ChangeLog update.

Shinya Kawanaka

Comment 37 2012-10-25 21:53:52 PDT

Created attachment 170807 [details] Patch

Shinya Kawanaka

Comment 38 2012-10-25 21:54:40 PDT

Added a testcase about inert InsertionPoint, and fixed a bug related to it.

Shinya Kawanaka

Comment 39 2012-10-28 17:57:36 PDT

Hmm... No one did review? :-(

Dimitri Glazkov (Google)

Comment 40 2012-10-28 18:24:58 PDT

(In reply to comment #38) > Added a testcase about inert InsertionPoint, and fixed a bug related to it. I'll try to get this in a couple of hours. Sorry :-\

Dimitri Glazkov (Google)

Comment 41 2012-10-28 20:57:40 PDT

Comment on attachment 170807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170807&action=review > Source/WebCore/dom/ElementShadow.cpp:135 > InsertionPoint* ElementShadow::insertionPointFor(const Node* node) const That method got whittled away to nothing! Can probably remove it. > Source/WebCore/html/shadow/InsertionPoint.h:135 > +inline Element* parentElementForDistribution(const Node* node) Is this guy still being used somewhere?

Shinya Kawanaka

Comment 42 2012-10-28 21:08:18 PDT

(In reply to comment #41) > (From update of attachment 170807 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170807&action=review > > > Source/WebCore/dom/ElementShadow.cpp:135 > > InsertionPoint* ElementShadow::insertionPointFor(const Node* node) const > > That method got whittled away to nothing! Can probably remove it. > > > Source/WebCore/html/shadow/InsertionPoint.h:135 > > +inline Element* parentElementForDistribution(const Node* node) > > Is this guy still being used somewhere? Thanks for reviewing! I'll make follow-up patches after landing this.

WebKit Review Bot

Comment 43 2012-10-28 21:08:23 PDT

Comment on attachment 170807 [details] Patch Clearing flags on attachment: 170807 Committed r132760: <http://trac.webkit.org/changeset/132760>

WebKit Review Bot

Comment 44 2012-10-28 21:08:30 PDT

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-10-12 18:18 PDT

Modified

2012-10-28 21:08 PDT History

CC List

14 users Show

URL

Keywords HasReduction

Depends on

100353

Blocks

96986

Dependencies

tree graph