Bug 109939

Created attachment 188566 [details] Patch

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

Created attachment 188580 [details] Patch

Ok, this is too broadly done.

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

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

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

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

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

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

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

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

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

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.

Created attachment 188701 [details] Patch

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

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

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

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

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.

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

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

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.

(In reply to comment #26) > I want the code shared, not just similar, though. That's the point of AncestorChainWalker.

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?

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

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

Created attachment 188736 [details] Patch

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

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

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

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 on attachment 188736 [details] Patch Clearing flags on attachment: 188736 Committed r143148: <http://trac.webkit.org/changeset/143148>

All reviewed patches have been landed. Closing bug.