Bug 97151 – Support re-projection for Shadow DOM.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hayato Ito]

ComposedShadowTreeWalker needs to be updated so that it should be aware of re-projection of Shadow DOM.

[Hayato Ito]

Created attachment 165061 [details]
wip. including other patches

[Dimitri Glazkov (Google)]

Comment on attachment 165061 [details]
wip. including other patches

This is starting to come together!

[Shinya Kawanaka]

Created attachment 165723 [details]
WIP

[Shinya Kawanaka]

Created attachment 165727 [details]
Test case

span 1, span 2, span 3, and span 4 should be displayed as console message in this order.

[WebKit Review Bot]

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

[Hayato Ito]

Created attachment 167092 [details]
Rebase

[Hayato Ito]

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.

[Dimitri Glazkov (Google)]

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?

[Hayato Ito]

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.

[Dimitri Glazkov (Google)]

(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 :)

[Hayato Ito]

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 :)

[Hayato Ito]

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

[Hayato Ito]

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.

[Hayato Ito]

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

[Hayato Ito]

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.

[Hayato Ito]

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.

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

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.

[Hayato Ito]

(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.

[Hayato Ito]

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

[Hayato Ito]

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.

[Dimitri Glazkov (Google)]

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!

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.