WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
110037
Use EventPathWalker rather than parentNode() to normalize event targets in EventHandler.
https://bugs.webkit.org/show_bug.cgi?id=110037
Summary
Use EventPathWalker rather than parentNode() to normalize event targets in Ev...
Mike West
Reported
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.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Mike West
Comment 1
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!
Mike West
Comment 2
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.
Mike West
Comment 3
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?
Mike West
Comment 4
2013-02-18 01:24:52 PST
Created
attachment 188818
[details]
WIP
Mike West
Comment 5
2013-02-18 01:26:41 PST
Comment on
attachment 188818
[details]
WIP Clearing r?, this is a WIP patch.
Ryosuke Niwa
Comment 6
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.
Hayato Ito
Comment 7
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.
Mike West
Comment 8
2013-02-18 04:57:41 PST
Created
attachment 188854
[details]
Patch
Ojan Vafai
Comment 9
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.
Ryosuke Niwa
Comment 10
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?
Mike West
Comment 11
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. :)
Ryosuke Niwa
Comment 12
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().
Ryosuke Niwa
Comment 13
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.
Ryosuke Niwa
Comment 14
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.
Mike West
Comment 15
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?
Ryosuke Niwa
Comment 16
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?
Mike West
Comment 17
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. :)
Ryosuke Niwa
Comment 18
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.
WebKit Review Bot
Comment 19
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
Mike West
Comment 20
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?
Ojan Vafai
Comment 21
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.
Ryosuke Niwa
Comment 22
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);
Mike West
Comment 23
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.
Hayato Ito
Comment 24
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.
Hayato Ito
Comment 25
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.
Mike West
Comment 26
2013-02-20 00:20:59 PST
Created
attachment 189260
[details]
Patch
Ryosuke Niwa
Comment 27
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.
Mike West
Comment 28
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!
Mike West
Comment 29
2013-02-20 01:26:52 PST
Created
attachment 189264
[details]
Patch for landing
WebKit Review Bot
Comment 30
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
>
WebKit Review Bot
Comment 31
2013-02-20 01:44:59 PST
All reviewed patches have been landed. Closing bug.
Hayato Ito
Comment 32
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug