[webkit-reviews] review granted: [Bug 109939] WheelEvent should not target text nodes. : [Attachment 188701] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mike West <mkwst at chromium.org>'s
request for review:
Bug 109939: WheelEvent should not target text nodes.
https://bugs.webkit.org/show_bug.cgi?id=109939

Attachment 188701: Patch
https://bugs.webkit.org/attachment.cgi?id=188701&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.