Bug 110037 - Use EventPathWalker rather than parentNode() to normalize event targets in EventHandler.
Summary: Use EventPathWalker rather than parentNode() to normalize event targets in Ev...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on: 103299
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-16 15:47 PST by Mike West
Modified: 2013-02-20 01:56 PST (History)
9 users (show)

See Also:


Attachments
WIP (7.47 KB, patch)
2013-02-18 01:24 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (10.20 KB, patch)
2013-02-18 04:57 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (11.06 KB, patch)
2013-02-20 00:20 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (11.09 KB, patch)
2013-02-20 01:26 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2013-02-16 15:47:16 PST
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.
Comment 1 Mike West 2013-02-16 15:51:50 PST
CCing Hayato-san, who is apparently doing related work in https://bugs.webkit.org/show_bug.cgi?id=107796. Hello!
Comment 2 Mike West 2013-02-16 15:57:21 PST
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.
Comment 3 Mike West 2013-02-16 16:05:07 PST
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?
Comment 4 Mike West 2013-02-18 01:24:52 PST
Created attachment 188818 [details]
WIP
Comment 5 Mike West 2013-02-18 01:26:41 PST
Comment on attachment 188818 [details]
WIP

Clearing r?, this is a WIP patch.
Comment 6 Ryosuke Niwa 2013-02-18 01:28:43 PST
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.
Comment 7 Hayato Ito 2013-02-18 03:05:02 PST
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.
Comment 8 Mike West 2013-02-18 04:57:41 PST
Created attachment 188854 [details]
Patch
Comment 9 Ojan Vafai 2013-02-18 11:01:10 PST
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.
Comment 10 Ryosuke Niwa 2013-02-18 11:04:47 PST
(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?
Comment 11 Mike West 2013-02-18 11:34:25 PST
(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. :)
Comment 12 Ryosuke Niwa 2013-02-18 12:02:44 PST
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().
Comment 13 Ryosuke Niwa 2013-02-18 12:09:46 PST
(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.
Comment 14 Ryosuke Niwa 2013-02-18 12:10:51 PST
(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.
Comment 15 Mike West 2013-02-18 12:13:27 PST
(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?
Comment 16 Ryosuke Niwa 2013-02-18 12:16:32 PST
(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?
Comment 17 Mike West 2013-02-18 13:12:05 PST
(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. :)
Comment 18 Ryosuke Niwa 2013-02-18 13:37:03 PST
(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.
Comment 19 WebKit Review Bot 2013-02-18 22:16:23 PST
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
Comment 20 Mike West 2013-02-18 23:55:03 PST
(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?
Comment 21 Ojan Vafai 2013-02-19 10:12:59 PST
(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.
Comment 22 Ryosuke Niwa 2013-02-19 10:40:46 PST
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);
Comment 23 Mike West 2013-02-19 10:43:02 PST
(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.
Comment 24 Hayato Ito 2013-02-19 20:00:09 PST
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.
Comment 25 Hayato Ito 2013-02-19 20:01:21 PST
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.
Comment 26 Mike West 2013-02-20 00:20:59 PST
Created attachment 189260 [details]
Patch
Comment 27 Ryosuke Niwa 2013-02-20 01:22:23 PST
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.
Comment 28 Mike West 2013-02-20 01:24:24 PST
(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!
Comment 29 Mike West 2013-02-20 01:26:52 PST
Created attachment 189264 [details]
Patch for landing
Comment 30 WebKit Review Bot 2013-02-20 01:44:53 PST
Comment on attachment 189264 [details]
Patch for landing

Clearing flags on attachment: 189264

Committed r143439: <http://trac.webkit.org/changeset/143439>
Comment 31 WebKit Review Bot 2013-02-20 01:44:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 Hayato Ito 2013-02-20 01:56:38 PST
Thank you for fixing this!

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