[webkit-reviews] review granted: [Bug 198036] The "mouseenter" and "pointerenter" events are fired from the bottom up : [Attachment 370735] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 198036: The "mouseenter" and "pointerenter" events are fired from the
bottom up
https://bugs.webkit.org/show_bug.cgi?id=198036

Attachment 370735: Patch

https://bugs.webkit.org/attachment.cgi?id=370735&action=review




--- Comment #22 from Darin Adler <darin at apple.com> ---
Comment on attachment 370735
  --> https://bugs.webkit.org/attachment.cgi?id=370735
Patch

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

Looks like the test isn’t passing on EWS yet.

> Source/WebCore/page/PointerCaptureController.cpp:194
> +	       Vector<Ref<Element>, 32> enterChain;

Putting the elements into a vector before dispatching events has another effect
besides reversing the order. It also means that DOM mutation inside the event
handler has no effect on which elements receive the dispatched events. So the
change is observable.

Therefore, I think we should consider using a vector in both cases. This would
also make the code less repetitive.

We should also consider making test cases that demonstrate how mutating the DOM
to change the parent chain affects or does not affect which elements get an
event dispatched. This is the kind of thing that can lead to incompatibilities
between web browsers when their implementations differ. It’s probably not all
that unusual for an event handler to end up removing the node that received the
event from its parent, for example.

> Source/WebCore/page/PointerCaptureController.cpp:195
> +	       for (Element* element = &downcast<Element>(target); element;
element = element->parentElementInComposedTree()) {

Not new to this patch, but why not use targetElement instead of writing
&downcast<Element>(target) again?

> Source/WebCore/page/PointerCaptureController.cpp:203
> +	       for (Element* element = &downcast<Element>(target); element;
element = element->parentElementInComposedTree()) {

Seems like this loop needs to use RefPtr<Element>. Otherwise what guarantees
that the code we dispatch to won’t deallocate the element? Same idea about
targetElement as above.