Bug 109939 – WheelEvent should not target text nodes.

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

[Mike West]

MouseWheel events should not target text nodes.

[Mike West]

Created attachment 188566 [details]
Patch

[Mike West]

So. Let's see if this explodes before asking for opinions.

[Mike West]

Created attachment 188580 [details]
Patch

[Mike West]

Ok, this is too broadly done.

[Mike West]

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

[Mike West]

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

[WebKit Review Bot]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Mike West]

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

[Ojan Vafai]

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

[Darin Adler]

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

[Ojan Vafai]

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

[Mike West]

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

[Mike West]

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

[Mike West]

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.

[Mike West]

Created attachment 188701 [details]
Patch

[Mike West]

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

[Ojan Vafai]

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

[Mike West]

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

[Ojan Vafai]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Mike West]

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

[Darin Adler]

I want the code shared, not just similar, though.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

(In reply to comment #26)
> I want the code shared, not just similar, though.

That's the point of AncestorChainWalker.

[Darin Adler]

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?

[Mike West]

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

[Dimitri Glazkov (Google)]

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

[Mike West]

Created attachment 188736 [details]
Patch

[Mike West]

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

[Dimitri Glazkov (Google)]

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

[Darin Adler]

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

[Hayato Ito]

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?

[WebKit Review Bot]

Comment on attachment 188736 [details]
Patch

Clearing flags on attachment: 188736

Committed r143148: <http://trac.webkit.org/changeset/143148>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.