Bug 110037 – Use EventPathWalker rather than parentNode() to normalize event targets in EventHandler.

• Component: UI Events
• Status: RESOLVED FIXED

[Mike West]

While addressing https://bugs.webkit.org/show_bug.cgi?id=109939, we discovered that we're avoiding targeting mouse events to text nodes in a way that can cause problems when coupled with shadow dom. Rather than grabbing a node's parentNode() directly for use in an event, we should be using AncestorChainWalker to grab the nearest targetable parent node.

This effects EventHandler::handleMouseDraggedEvent, EventHandler::updateDragAndDrop, closestScrollableNodeCandidate, and EventHandler::handleTouchEvent at least.

[Mike West]

CCing Hayato-san, who is apparently doing related work in https://bugs.webkit.org/show_bug.cgi?id=107796. Hello!

[Mike West]

Details, from dglazkov@ in https://bugs.webkit.org/show_bug.cgi?id=109939#c27:

---

Consider this scenario:

1) a shadow tree is hosted by element A
2) element A has a child text node B
3) shadow tree has an insertion point
4) B is distributed into this insertion point.

If you need more background on the terminology/plumbing, seek knowledge here: https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#dfn-distribution

If we just use B's parent as initial target, it will be impossible for an event listener inside the shadow tree to ever hear this event. That seems bad. Thus, we should use AncestorChainWalker.

As a rule of thumb, AncestorChainWalker will do the right thing for any ancestor traversal related to events.

[Mike West]

Perhaps we should teach HitTestResult to return an "eventTargetableNode" (or something better named) alongside it's current "innerNode"? That would mean that that EventHandler wouldn't need to know about ancestors or chains, and we'd be able to reuse the targeting logic if we need it elsewhere.

WDYT?

[Mike West]

Created attachment 188818 [details]
WIP

[Mike West]

Comment on attachment 188818 [details]
WIP

Clearing r?, this is a WIP patch.

[Ryosuke Niwa]

Comment on attachment 188818 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=188818&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please update this. We have a test.

> Source/WebCore/page/EventHandler.cpp:263
> +    // Events targeting text nodes should dispatch on their parent node
> +    // (rdar://4196646). This is complicated a bit by Shadow DOM goodness,
> +    // so we'll need to walk the ancestor chain rather than grabbing the
> +    // Node's parentNode() directly.

You can probably fit all of this comment in two lines.
Also, it’s not great add a comment like this when we can instead rename the function to convey the reason.
e.g. retargetTextNodeEventsToParentConsideringShadowDOM

Furthermore, “Shadow DOM goodness” is extremely vague. Please elaborate it in the change log and
the function name should also convey the semantics. And that should eliminate the need for the comment.

[Hayato Ito]

As we chatted in #webkit, the layout test in the patch does not output the expected behavior.
Let me debug this tomorrow.

(In reply to comment #5)
> (From update of attachment 188818 [details])
> Clearing r?, this is a WIP patch.

[Mike West]

Created attachment 188854 [details]
Patch

[Ojan Vafai]

Comment on attachment 188854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188854&action=review

> Source/WebCore/page/EventHandler.cpp:255
> +static inline Node* retargetTextNodeEventsToParentOrShadowHost(Node* node)

This isn't really retargeting the event itself. It's returning a new node that will be used at the new target. How about nonTextNodeParentOrShadowHost? I'm not thrilled with that name either, but it's the best I can think of at the moment.

[Ryosuke Niwa]

(In reply to comment #9)
> (From update of attachment 188854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188854&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:255
> > +static inline Node* retargetTextNodeEventsToParentOrShadowHost(Node* node)
> 
> This isn't really retargeting the event itself. It's returning a new node that will be used at the new target. How about nonTextNodeParentOrShadowHost? I'm not thrilled with that name either, but it's the best I can think of at the moment.

parentOrShadowHostElement?

[Mike West]

(In reply to comment #9)
> (From update of attachment 188854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188854&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:255
> > +static inline Node* retargetTextNodeEventsToParentOrShadowHost(Node* node)
> 
> This isn't really retargeting the event itself. It's returning a new node that will be used at the new target. How about nonTextNodeParentOrShadowHost? I'm not thrilled with that name either, but it's the best I can think of at the moment.

Would you accept the name if it described a method that actually modified the given Node* rather than returning a new Node*? I think the word "retarget" is fairly critical in understanding what's going on.

"parentOrShadowHostElement" also describes what we're getting, but doesn't really explain to any extent why we're doing it.

*shrug* This was the shortest descriptive name I could come up with. :)

[Ryosuke Niwa]

Comment on attachment 188854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188854&action=review

> Source/WebCore/page/EventHandler.cpp:260
> +    AncestorChainWalker walker(node);

The problem is the name of this obnoxious class.
It's not obvious at all why AncestorChainWalker(node).parent() would never return a node other than node->parent().

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 188854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188854&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:260
> > +    AncestorChainWalker walker(node);
> 
> The problem is the name of this obnoxious class.
> It's not obvious at all why AncestorChainWalker(node).parent() would never return a node other than node->parent().

I've filed https://bugs.webkit.org/show_bug.cgi?id=110146 to track this.

[Ryosuke Niwa]

(In reply to comment #11)
> Would you accept the name if it described a method that actually modified the given Node* rather than returning a new Node*? I think the word "retarget" is fairly critical in understanding what's going on.
> 
> "parentOrShadowHostElement" also describes what we're getting, but doesn't really explain to any extent why we're doing it.

parentNodeCrossingInsertionPointIfNeeded.

[Mike West]

(In reply to comment #14)
> (In reply to comment #11)
> > Would you accept the name if it described a method that actually modified the given Node* rather than returning a new Node*? I think the word "retarget" is fairly critical in understanding what's going on.
> > 
> > "parentOrShadowHostElement" also describes what we're getting, but doesn't really explain to any extent why we're doing it.
> 
> parentNodeCrossingInsertionPointIfNeeded.

You don't need the parent node unless you're targeting a text node. If the method name doesn't tell you that somehow, why would you call it?

[Ryosuke Niwa]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #11)
> > > Would you accept the name if it described a method that actually modified the given Node* rather than returning a new Node*? I think the word "retarget" is fairly critical in understanding what's going on.
> > > 
> > > "parentOrShadowHostElement" also describes what we're getting, but doesn't really explain to any extent why we're doing it.
> > 
> > parentNodeCrossingInsertionPointIfNeeded.
> 
> You don't need the parent node unless you're targeting a text node. If the method name doesn't tell you that somehow, why would you call it?

parentCrossingInsertionPointIfNotTextNode?

[Mike West]

(In reply to comment #16)
> > You don't need the parent node unless you're targeting a text node. If the method name doesn't tell you that somehow, why would you call it?
> 
> parentCrossingInsertionPointIfNotTextNode?

Again, this method only has an effect if the current target _is_ a text node. I'd like to end up with a name that makes it clear that (a) this method should be called when dealing with events that target a node, (b) this method only has an effect when the current target is a text node, and (c) that effect is to change the event's target to the node's insertion point's parent (or something like that).

Alternatively, we could name it something more generic, and add a descriptive comment. :)

[Ryosuke Niwa]

(In reply to comment #17)
> (In reply to comment #16)
> > > You don't need the parent node unless you're targeting a text node. If the method name doesn't tell you that somehow, why would you call it?
> > 
> > parentCrossingInsertionPointIfNotTextNode?
> 
> Again, this method only has an effect if the current target _is_ a text node. I'd like to end up with a name that makes it clear that (a) this method should be called when dealing with events that target a node, (b) this method only has an effect when the current target is a text node, and (c) that effect is to change the event's target to the node's insertion point's parent (or something like that).

nonTextNodeAncestorCrossingInsertionPointIfNeeded.

> Alternatively, we could name it something more generic, and add a descriptive comment. :)

Please don't.

[WebKit Review Bot]

Comment on attachment 188854 [details]
Patch

Attachment 188854 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16617487

New failing tests:
fast/forms/datalist/update-range-with-datalist.html
fast/dom/shadow/shadow-dom-event-dispatching-text-node-in-shadow-root.html
fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node.html

[Mike West]

(In reply to comment #19)
> (From update of attachment 188854 [details])
> Attachment 188854 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16617487
> 
> New failing tests:
> fast/forms/datalist/update-range-with-datalist.html
> fast/dom/shadow/shadow-dom-event-dispatching-text-node-in-shadow-root.html
> fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node.html

Ugh. I changed the tests, but didn't rebaseline them. :)

Regarding the name, how about a different direction: findNearestEventTargetableNode? That drops some of the detail, but has the advantage of sounding like something you'd want to do.

WDYT?

[Ojan Vafai]

(In reply to comment #20)
> Regarding the name, how about a different direction: findNearestEventTargetableNode? That drops some of the detail, but has the advantage of sounding like something you'd want to do.
> 
> WDYT?

Works for me. Better than the other suggested names so far.

[Ryosuke Niwa]

Now that we have https://bugs.webkit.org/show_bug.cgi?id=110146, we probably don't need a function for it:
if (node && node->isTextNode())
    node = EventPathWalker.parent(node);

[Mike West]

(In reply to comment #22)
> Now that we have https://bugs.webkit.org/show_bug.cgi?id=110146, we probably don't need a function for it:
> if (node && node->isTextNode())
>     node = EventPathWalker.parent(node);

Makes sense to me. I'll wait till that refactoring lands, then update this patch to fix the places where it's not being used. Thanks for the heads up.

[Hayato Ito]

I've just landed the patch in https://bugs.webkit.org/show_bug.cgi?id=110146.
I think we should also rename the title of this bug.

(In reply to comment #23)
> (In reply to comment #22)
> > Now that we have https://bugs.webkit.org/show_bug.cgi?id=110146, we probably don't need a function for it:
> > if (node && node->isTextNode())
> >     node = EventPathWalker.parent(node);
> 
> Makes sense to me. I'll wait till that refactoring lands, then update this patch to fix the places where it's not being used. Thanks for the heads up.

[Hayato Ito]

I've renamed the title.

(In reply to comment #24)
> I've just landed the patch in https://bugs.webkit.org/show_bug.cgi?id=110146.
> I think we should also rename the title of this bug.
> 
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Now that we have https://bugs.webkit.org/show_bug.cgi?id=110146, we probably don't need a function for it:
> > > if (node && node->isTextNode())
> > >     node = EventPathWalker.parent(node);
> > 
> > Makes sense to me. I'll wait till that refactoring lands, then update this patch to fix the places where it's not being used. Thanks for the heads up.

[Mike West]

Created attachment 189260 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 189260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189260&action=review

> Source/WebCore/page/EventHandler.cpp:711
> +        renderer = parent ? parent->renderer() : 0;

I would have preferred to see an early return when parent was 0.

[Mike West]

(In reply to comment #27)
> (From update of attachment 189260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189260&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:711
> > +        renderer = parent ? parent->renderer() : 0;
> 
> I would have preferred to see an early return when parent was 0.

I'll make that change before landing. Thanks!

[Mike West]

Created attachment 189264 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189264 [details]
Patch for landing

Clearing flags on attachment: 189264

Committed r143439: <http://trac.webkit.org/changeset/143439>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hayato Ito]

Thank you for fixing this!

(In reply to comment #31)
> All reviewed patches have been landed.  Closing bug.