Bug 97151

Created attachment 165061 [details] wip. including other patches

Comment on attachment 165061 [details] wip. including other patches This is starting to come together!

Created attachment 165723 [details] WIP

Created attachment 165727 [details] Test case span 1, span 2, span 3, and span 4 should be displayed as console message in this order.

Comment on attachment 165723 [details] WIP Attachment 165723 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14035295 New failing tests: fast/dom/shadow/shadow-dom-event-dispatching.html fast/dom/shadow/composed-shadow-tree-walker.html

Created attachment 167092 [details] Rebase

The patch can be applied on https://bugs.webkit.org/attachment.cgi?id=167079. (In reply to comment #6) > Created an attachment (id=167092) [details] > Rebase There are still about 10 failing layout tests. Let me fix that.

Comment on attachment 167092 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=167092&action=review > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:248 > + if (InsertionPoint* insertedTo = shadow->insertionPointFor(node)) { > + current = insertedTo; > + insertionPoint = insertedTo; Are you sure this is the right approach? I think we're skipping all the intermediate insertion points here. Shouldn't we be adding them to the parent chain instead?

Comment on attachment 167092 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=167092&action=review >> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:248 >> + insertionPoint = insertedTo; > > Are you sure this is the right approach? I think we're skipping all the intermediate insertion points here. Shouldn't we be adding them to the parent chain instead? We have two walkers, ComposedShadowTreeWalker and ComposedShadowTreeParentWalker. - The former is used in Rendering, which can skip all the intermediate insertion points. So this static function is okay for that. - The latter is used in EventDispatcher, which can not skip all the intermediate insertion points. I have to update ParentWalker later, where we add all intermediate insertion points to the ancestor chain.

(In reply to comment #9) > (From update of attachment 167092 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167092&action=review > > >> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:248 > >> + insertionPoint = insertedTo; > > > > Are you sure this is the right approach? I think we're skipping all the intermediate insertion points here. Shouldn't we be adding them to the parent chain instead? > > We have two walkers, ComposedShadowTreeWalker and ComposedShadowTreeParentWalker. > > - The former is used in Rendering, which can skip all the intermediate insertion points. So this static function is okay for that. > - The latter is used in EventDispatcher, which can not skip all the intermediate insertion points. I have to update ParentWalker later, where we add all intermediate insertion points to the ancestor chain. Ahh, I see. We should file a bug somewhere to rename these walkers to actually reflect how they are used. The distinction was lost on me, and I reviewed most of that code :)

Yeah, al least, ComposedShadowTreeParentWalker is not a good name. That is used only from EventDispatcher. That should have a better name. Let me rename it later. (In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 167092 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=167092&action=review > > > > >> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:248 > > >> + insertionPoint = insertedTo; > > > > > > Are you sure this is the right approach? I think we're skipping all the intermediate insertion points here. Shouldn't we be adding them to the parent chain instead? > > > > We have two walkers, ComposedShadowTreeWalker and ComposedShadowTreeParentWalker. > > > > - The former is used in Rendering, which can skip all the intermediate insertion points. So this static function is okay for that. > > - The latter is used in EventDispatcher, which can not skip all the intermediate insertion points. I have to update ParentWalker later, where we add all intermediate insertion points to the ancestor chain. > > Ahh, I see. > > We should file a bug somewhere to rename these walkers to actually reflect how they are used. The distinction was lost on me, and I reviewed most of that code :)

Created attachment 167726 [details] WIP. We have to update an event re-targeting algorithm for re-projection.

One test, fast/dom/shadow/shadow-dom/event-dispatching.html is failing. I have to update an event re-targeting algorithm fo re-projection. (In reply to comment #12) > Created an attachment (id=167726) [details] > WIP. We have to update an event re-targeting algorithm for re-projection.

Created attachment 167905 [details] WIP. Pass all tests.

It turned out that I have to update ContentDistributor further so that <shadow> element is ready for re-projection. (In reply to comment #14) > Created an attachment (id=167905) [details] > WIP. Pass all tests.

It seems we have to use 2-pass algorithm to resolve distributions for multiple shadow roots. The first pass: From younger to older: Resolve distributions for <content> element. Because <content> element in younger shadow root has higher priority to get host's children than older's. The second pass: From older to younger: Resolve distribution for <shadow> element. Since <shadow> elements depend on the older's shadowroot's direct children, which might be a <content> element. Hmm. I am wondering whether this (2-path) is really acceptable solution or not. (In reply to comment #15) > It turned out that I have to update ContentDistributor further so that <shadow> element is ready for re-projection. > > (In reply to comment #14) > > Created an attachment (id=167905) [details] [details] > > WIP. Pass all tests.

(In reply to comment #16) > It seems we have to use 2-pass algorithm to resolve distributions for multiple shadow roots. > > The first pass: > From younger to older: > Resolve distributions for <content> element. Because <content> element in younger shadow root has higher priority to get host's children than older's. > > The second pass: > From older to younger: > Resolve distribution for <shadow> element. Since <shadow> elements depend on the older's shadowroot's direct children, which might be a <content> element. > > Hmm. I am wondering whether this (2-path) is really acceptable solution or not. Let's not stress about <shadow> just yet.

Comment on attachment 167905 [details] WIP. Pass all tests. View in context: https://bugs.webkit.org/attachment.cgi?id=167905&action=review This looks good as is (aside from commented out code and frpintfs of course). > Source/WebCore/dom/ComposedShadowTreeWalker.h:177 > + enum TraversalState {TraversingParent, CrossingInsertionPoint}; Does this have to be an enum? Seems like bool m_isCrossingInsertionPoint, based its use. > Source/WebCore/dom/ComposedShadowTreeWalker.h:179 > + const Node* m_distributedNode; Yep. That's what I need to add to the spec, too :-\ > Source/WebCore/html/shadow/ContentDistributor.cpp:74 > + for (Node* fallbackNode = insertionPoint->firstChild(); fallbackNode; fallbackNode = fallbackNode->nextSibling()) > + pool.append(fallbackNode); This is interesting. I need to add that in spec.

(In reply to comment #17) > (In reply to comment #16) > > It seems we have to use 2-pass algorithm to resolve distributions for multiple shadow roots. > > > > The first pass: > > From younger to older: > > Resolve distributions for <content> element. Because <content> element in younger shadow root has higher priority to get host's children than older's. > > > > The second pass: > > From older to younger: > > Resolve distribution for <shadow> element. Since <shadow> elements depend on the older's shadowroot's direct children, which might be a <content> element. > > > > Hmm. I am wondering whether this (2-path) is really acceptable solution or not. > > Let's not stress about <shadow> just yet. I agree. Let me file that as another bug. Nobody doesn't need it yet.

Created attachment 168182 [details] ready for reivew, but the patch still depends on https://bugs.webkit.org/show_bug.cgi?id=96990.

Comment on attachment 167905 [details] WIP. Pass all tests. View in context: https://bugs.webkit.org/attachment.cgi?id=167905&action=review >> Source/WebCore/dom/ComposedShadowTreeWalker.h:177 >> + enum TraversalState {TraversingParent, CrossingInsertionPoint}; > > Does this have to be an enum? Seems like bool m_isCrossingInsertionPoint, based its use. Done.

Comment on attachment 168182 [details] ready for reivew, but the patch still depends on https://bugs.webkit.org/show_bug.cgi?id=96990. View in context: https://bugs.webkit.org/attachment.cgi?id=168182&action=review Thank you! > Source/WebCore/ChangeLog:20 > + - AncestorChainWalker (formerly named ComposedShadowTreeParentWallker): Much better name!

Comment on attachment 168182 [details] ready for reivew, but the patch still depends on https://bugs.webkit.org/show_bug.cgi?id=96990. Clearing flags on attachment: 168182 Committed r131070: <http://trac.webkit.org/changeset/131070>

All reviewed patches have been landed. Closing bug.