Bug 109939

Summary: WheelEvent should not target text nodes.
Product: WebKit Reporter: Mike West <mkwst>
Component: DOMAssignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, arv, darin, dglazkov, hayato, morrita, ojan, syoichi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111439    
Bug Blocks: 110007    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Mike West 2013-02-15 07:28:06 PST
MouseWheel events should not target text nodes.
Comment 1 Mike West 2013-02-15 07:30:17 PST
Created attachment 188566 [details]
Patch
Comment 2 Mike West 2013-02-15 07:30:49 PST
So. Let's see if this explodes before asking for opinions.
Comment 3 Mike West 2013-02-15 08:39:46 PST
Created attachment 188580 [details]
Patch
Comment 4 Mike West 2013-02-15 08:47:18 PST
Ok, this is too broadly done.
Comment 5 Mike West 2013-02-15 08:49:13 PST
(In reply to comment #4)
> Ok, this is too broadly done.

Some DOM Events should, apparently, target text nodes. DOMCharacterDataModified, DOMNodeInserted, and DOMSubtreeModified, for instance, are all breaking in tests. Someone better informed about the spec will have to help me out here.

Ojan? Opinions? :)
Comment 6 Mike West 2013-02-15 08:50:04 PST
For background, this report is coming from jQuery, which is currently working around WebKit's behavior in this regard: https://github.com/jquery/jquery/commit/c61150427fc8ccc8e884df8f221a6c9bb5477929
Comment 7 WebKit Review Bot 2013-02-15 09:31:32 PST
Comment on attachment 188580 [details]
Patch

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

New failing tests:
fast/parser/nested-fragment-parser-crash.html
svg/custom/tref-remove-target-crash.svg
fast/events/crash-on-mutate-during-drop.html
editing/inserting/insert-paragraph-separator-crash.html
Comment 8 Darin Adler 2013-02-15 09:38:10 PST
Comment on attachment 188580 [details]
Patch

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

> Source/WebCore/dom/EventRetargeter.h:70
> +    if (referenceNode->isPseudoElement() || referenceNode->isTextNode())

To me this seems like the wrong level to fix the bug. What code is sending an event with a target of a text node? Having the event system retarget the event will cover all events, but it’s really sloppy to patch it up at this level instead of doing it right at the higher level.
Comment 9 Darin Adler 2013-02-15 09:39:27 PST
Further, putting the fix at this low level, but adding only one test, is not a good idea. We would like tests covering all the cases that are malfunctioning.
Comment 10 Darin Adler 2013-02-15 09:40:13 PST
Comment on attachment 188580 [details]
Patch

Thanks for tackling this; I’m going to say review- for now. Maybe you can convince me (or other experts, since there’s no guarantee I will be around to review again) that this is the best approach; I may be missing something.
Comment 11 Mike West 2013-02-15 09:44:57 PST
(In reply to comment #10)
> (From update of attachment 188580 [details])
> Thanks for tackling this; I’m going to say review- for now. Maybe you can convince me (or other experts, since there’s no guarantee I will be around to review again) that this is the best approach; I may be missing something.

For the record, I completely agree that we need more tests here. This was really just for the bots to see what would break. I didn't actually intend for it to be r?. :)

As far as the level at which we make the change, I think I agree with you. Since there are likely events that do need to target text nodes, this is the wrong place to make the change. It's overly broad. I'll work to narrow down the events we should care about, and find better places to retarget them.
Comment 12 Ojan Vafai 2013-02-15 13:37:26 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Ok, this is too broadly done.
> 
> Some DOM Events should, apparently, target text nodes. DOMCharacterDataModified, DOMNodeInserted, and DOMSubtreeModified, for instance, are all breaking in tests. Someone better informed about the spec will have to help me out here.

I'm pretty sure these are the only events that will target text nodes. Arv might know of others. FWIW, we're trying to remove these events from the platform. In the meantime we need to support it though.

Do we know if jquery is hitting this through code other than by creating their own synthetic wheelevent and calling dispatchEvent on it? If so, it sounds like we have a bug somewhere else.

Whether dispatchEvent on a text node should ever work is another question. As best I can tell, the current DOM spec allows it: http://dom.spec.whatwg.org/#concept-event-dispatch. What do other browsers do? It's not clear to me what the best behavior here is.
Comment 13 Darin Adler 2013-02-15 16:26:42 PST
(In reply to comment #12)
> Do we know if jquery is hitting this through code other than by creating their own synthetic wheelevent and calling dispatchEvent on it? If so, it sounds like we have a bug somewhere else.

That’s a good question. If real wheel events are coming through with text nodes as their targets, then that’s one thing, and something we should fix. But dispatchEvent allowing you to dispatch that kind of event is another thing entirely.

> Whether dispatchEvent on a text node should ever work is another question. As best I can tell, the current DOM spec allows it: http://dom.spec.whatwg.org/#concept-event-dispatch. What do other browsers do? It's not clear to me what the best behavior here is.

Seems clear our current behavior is right. I doubt very much the other browsers retarget when dispatchEvent is called. If someone is claiming that, we should test to verify and then get the DOM specification updated if it’s not matching the de facto standard.
Comment 14 Ojan Vafai 2013-02-15 17:19:19 PST
(In reply to comment #13)
> (In reply to comment #12)
> > Whether dispatchEvent on a text node should ever work is another question. As best I can tell, the current DOM spec allows it: http://dom.spec.whatwg.org/#concept-event-dispatch. What do other browsers do? It's not clear to me what the best behavior here is.
> 
> Seems clear our current behavior is right. I doubt very much the other browsers retarget when dispatchEvent is called. If someone is claiming that, we should test to verify and then get the DOM specification updated if it’s not matching the de facto standard.

Yes!
Comment 15 Mike West 2013-02-15 22:47:49 PST
(In reply to comment #13)
> (In reply to comment #12)
> > Do we know if jquery is hitting this through code other than by creating their own synthetic wheelevent and calling dispatchEvent on it? If so, it sounds like we have a bug somewhere else.
> 
> That’s a good question. If real wheel events are coming through with text nodes as their targets, then that’s one thing, and something we should fix. But dispatchEvent allowing you to dispatch that kind of event is another thing entirely.

Real wheel events are targeting text nodes: http://bugs.jquery.com/ticket/13143 is the original report against jQuery, and I've verified it at http://jsfiddle.net/RuCjf/.

I wrote the test with dispatchEvent, as it seemed like the simplest way of testing the behavior, but I'm sure there's a testRunner method to simulate a real mouse. I'll track that down.
Comment 16 Mike West 2013-02-16 01:23:52 PST
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Do we know if jquery is hitting this through code other than by creating their own synthetic wheelevent and calling dispatchEvent on it? If so, it sounds like we have a bug somewhere else.
> > 
> > That’s a good question. If real wheel events are coming through with text nodes as their targets, then that’s one thing, and something we should fix. But dispatchEvent allowing you to dispatch that kind of event is another thing entirely.
> 
> Real wheel events are targeting text nodes: http://bugs.jquery.com/ticket/13143 is the original report against jQuery, and I've verified it at http://jsfiddle.net/RuCjf/.

http://jsfiddle.net/RuCjf/1/, sorry.
Comment 17 Mike West 2013-02-16 01:39:42 PST
Digging through EventHandler, it looks like we're already handling other mouse events correctly. I'll limit this bug down to WheelEvents, as those are missing the step up to the parent node.
Comment 18 Mike West 2013-02-16 02:09:03 PST
Created attachment 188701 [details]
Patch
Comment 19 Mike West 2013-02-16 02:12:29 PST
(In reply to comment #18)
> Created an attachment (id=188701) [details]
> Patch

This patch is much narrower than the previous patch, as it looks like we're already doing the right thing for other mouse events, drag events, and touch events. WDYT?
Comment 20 Ojan Vafai 2013-02-16 09:49:12 PST
It looks like mouse events do something a bit more complicated. Either they are doing something too complicated, or this code is too simple. I'm not sure which. :) The shadow DOM folks hopefully can explain why mouse events need to use AncestorChainWalker instead of just grabbing parentNode.

http://trac.webkit.org/browser/trunk/Source/WebCore/page/EventHandler.cpp#L2118
Comment 21 Mike West 2013-02-16 09:56:09 PST
(In reply to comment #20)
> It looks like mouse events do something a bit more complicated. Either they are doing something too complicated, or this code is too simple. I'm not sure which. :) The shadow DOM folks hopefully can explain why mouse events need to use AncestorChainWalker instead of just grabbing parentNode.
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/page/EventHandler.cpp#L2118

Hrm. You're right. Drag and touch are much simpler. Funky. Seems like we should unify these somehow.
Comment 22 Ojan Vafai 2013-02-16 10:37:00 PST
(In reply to comment #21)
> Hrm. You're right. Drag and touch are much simpler. Funky. Seems like we should unify these somehow.

Totally. We just need to understand what the case is where we need to use AncestorChainWalker instead of just grabbing parent. Looks like this code was added in http://trac.webkit.org/changeset/131070.

Hayato-san, can you explain to us the cases where this is necessary so that we can create a wheel event case that hits it as well?
Comment 23 Darin Adler 2013-02-16 13:13:04 PST
Comment on attachment 188701 [details]
Patch

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

Patch is OK, but probably not 100% correct since it’s not the same as for mouse events.

> Source/WebCore/page/EventHandler.cpp:2399
> +    // Wheel events should not dispatch to text nodes.
> +    if (node && node->isTextNode())
> +        node = node->parentNode();

The code to do this for mouse events is in EventHandler::updateMouseEventTargetNode, which is in turn called by EventHandler::dispatchMouseEvent. It uses AncestorChainWalker instead of just calling parentNode. It would be best if this code was shared for mouse and wheel events. It’s hard to imagine the AncestorChainWalker being needed for mouse events but not wheel events!

Also, I think this logic should take effect before the code that sets m_latchedWheelEventNode. We don’t want it to latch to the text node.

Aside from this particular patch, it’s not good that wheel events have similar requirements, but functions that are factored and named completely differently. I think that choice led more-or-less directly to this bug.

Seeing the ancestor chain walker code upset me because I assume it’s some kind of subtle new rule about Shadow DOM that is not clearly explained anywhere, so I have no idea how we are supposed to get this sort of thing right. I am uncomfortable with how we are doing the shadow DOM work in the WebKit tree.
Comment 24 Darin Adler 2013-02-16 13:24:26 PST
Heh, that’s funny. I see other people discussing the mouse vs. wheel event thing here too. I didn’t see those comments before my review. Glad you all discovered that stuff too.
Comment 25 Mike West 2013-02-16 15:02:45 PST
(In reply to comment #24)
> Heh, that’s funny. I see other people discussing the mouse vs. wheel event thing here too. I didn’t see those comments before my review. Glad you all discovered that stuff too.

Thanks for the review, Darin.

I do think I'll wait until there's some clarity around AncestorChainWalker before landing this just to avoid churn. Depending on the intended usage, it might be worth changing drag and touch in a followup patch... If it's necessary for mouse events, I'd guess that it's necessary for those as well.
Comment 26 Darin Adler 2013-02-16 15:17:50 PST
I want the code shared, not just similar, though.
Comment 27 Dimitri Glazkov (Google) 2013-02-16 15:38:33 PST
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 28 Dimitri Glazkov (Google) 2013-02-16 15:38:56 PST
(In reply to comment #26)
> I want the code shared, not just similar, though.

That's the point of AncestorChainWalker.
Comment 29 Darin Adler 2013-02-16 15:40:49 PST
Are the uses of parentNode in EventHandler::handleMouseDraggedEvent, EventHandler::updateDragAndDrop, closestScrollableNodeCandidate, and EventHandler::handleTouchEvent all incorrect? And if so, who is going to fix those?
Comment 30 Mike West 2013-02-16 15:41:52 PST
(In reply to comment #29)
> Are the uses of parentNode in EventHandler::handleMouseDraggedEvent, EventHandler::updateDragAndDrop, closestScrollableNodeCandidate, and EventHandler::handleTouchEvent all incorrect?

Sounds like it.

> And if so, who is going to fix those?

Me. In a bug I'm about to file. :)
Comment 31 Dimitri Glazkov (Google) 2013-02-16 15:48:09 PST
(In reply to comment #30)
> 
> Me. In a bug I'm about to file. :)

Hayato-san is working on gesture/touch-related events in bug 107796, make sure to check with him.
Comment 32 Mike West 2013-02-16 15:56:35 PST
Created attachment 188736 [details]
Patch
Comment 33 Mike West 2013-02-16 16:00:07 PST
(In reply to comment #26)
> I want the code shared, not just similar, though.

Darin, would you be OK with landing this patch to fix the narrow WheelEvent bug, and covering the merger of the text node logic in https://bugs.webkit.org/show_bug.cgi?id=110037?
Comment 34 Dimitri Glazkov (Google) 2013-02-16 16:01:16 PST
(In reply to comment #23)
> Seeing the ancestor chain walker code upset me because I assume it’s some kind of subtle new rule about Shadow DOM that is not clearly explained anywhere, so I have no idea how we are supposed to get this sort of thing right. I am uncomfortable with how we are doing the shadow DOM work in the WebKit tree.

I don't want you to be upset :) How can help to fix this?
Comment 35 Darin Adler 2013-02-16 16:06:42 PST
(In reply to comment #33)
> Darin, would you be OK with landing this patch to fix the narrow WheelEvent bug, and covering the merger of the text node logic in https://bugs.webkit.org/show_bug.cgi?id=110037?

Yes.
Comment 36 Hayato Ito 2013-02-17 17:19:02 PST
The change to EventHandler in  http://trac.webkit.org/changeset/131070 seems to be just a mechanical renaming (from ComposedShadowTreeParentWalker to AncestorChainWalker).

It seems I've introduced ComposedShadowTreeWalker to MouseEvent handler in https://bugs.webkit.org/show_bug.cgi?id=86999, which handles only MouseEvent's bug.

I think we can do the similar things to the other events.
We should adress that in https://bugs.webkit.org/show_bug.cgi?id=110037 as Mike has filed.

We should dig into EventHandler more and more.


(In reply to comment #22)
> (In reply to comment #21)
> > Hrm. You're right. Drag and touch are much simpler. Funky. Seems like we should unify these somehow.
> 
> Totally. We just need to understand what the case is where we need to use AncestorChainWalker instead of just grabbing parent. Looks like this code was added in http://trac.webkit.org/changeset/131070.
> 
> Hayato-san, can you explain to us the cases where this is necessary so that we can create a wheel event case that hits it as well?
Comment 37 WebKit Review Bot 2013-02-17 22:32:59 PST
Comment on attachment 188736 [details]
Patch

Clearing flags on attachment: 188736

Committed r143148: <http://trac.webkit.org/changeset/143148>
Comment 38 WebKit Review Bot 2013-02-17 22:33:05 PST
All reviewed patches have been landed.  Closing bug.