1340085 - [Pointer Event] Stop firing pointer events after firing eTouchCancel

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Comment 1

[Ming-Chou Shih [:stone]]

When dragging the page to scroll, we should fire the following events
ePointerDown
eTouchStart
ePointerMove
eTouchMove
ePointerCancel
eTouchMove
eTouchEnd

PointerEvent spec [1] says the user agent should fire pointercancel when the touch input is to be consumed for a touch behavior. For me, that implies we shouldn't fire the subsequent pointer events and behave the same as Chrome and edge.

TouchEvent spec [2] says we should fire touchcancel when a touch point has been disrupted in an implementation-specific manner.

[1] https://w3c.github.io/pointerevents/#the-touch-action-css-property
[2] https://w3c.github.io/touch-events/#event-touchcancel

Comment 2

[Ming-Chou Shih [:stone]]

Attachment: Stop firing pointer events after firing eTouchCancel

Current implementations fires eTouchCancel when PointerEvent is turned on, which clears TouchInfo in sCaptureTouchList and stop firing eTouchEnd to content (because we can't get its target from TouchInfo). Also, the eTouchEnd will generate ePointerUp even we already fired ePointerCancel (by eTouchCancel).

The major changes of this patch includes
1. Add a flag 'mConvertToPointer' in TouchInfo to record whether we should fire pointer events by touch events.
2. Replace the synthesized eTouchCancel with new event eMozTouchCancel. (It's only used to generate ePointerCancel). Set TouchInfo::mConvertToPointer to false when eMozTouchCancel is received to prevent the subsequent touch events generate pointer events
3. Check TouchInfo::mConvertToPointer before dispatching pointer events by touch events

Comment 3

[Ming-Chou Shih [:stone]]

Attachment: Stop firing pointer events after firing eTouchCancel

Refined test case bug970964_inner.html because it synthesized pointermove w/o pointerstart. It's impacted by this patch because it adds TouchInfo with mConvertToPointer=true when receiving pointerstart.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8839042 [details] [diff] [review]
Stop firing pointer events after firing eTouchCancel

Review of attachment 8839042 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok to me. Some minor comments below. Also flagging smaug just in case he has any concerns with this.

::: layout/base/PresShell.cpp
@@ +7093,5 @@
>  
>      for (uint32_t i = 0; i < touchEvent->mTouches.Length(); ++i) {
>        mozilla::dom::Touch* touch = touchEvent->mTouches[i];
> +      if (!touch || !touch->convertToPointer ||
> +          (touchEvent->mMessage != eTouchStart &&

If I understand correctly, you are excluding eTouchStart from this check because this code runs before the TouchManager even has the touch registered in its touch list. Is that correct? If so I think it would be better to modify the ShouldConvertTouchToPointer function to take a Touch* instead of an identifier. Then you can move this to be inside the function - if the touch id is not found in the list, and the touch is an eTouchStart, the function can return true. That way it's more contained and you can add some comments there explaining what's going on.

::: widget/EventMessageList.h
@@ +396,5 @@
>  NS_EVENT_MESSAGE(eTouchStart)
>  NS_EVENT_MESSAGE(eTouchMove)
>  NS_EVENT_MESSAGE(eTouchEnd)
>  NS_EVENT_MESSAGE(eTouchCancel)
> +NS_EVENT_MESSAGE(eMozTouchCancel)

I don't really like this name. I would prefer something like eTouchPointerCancel as it's slightly more descriptive (but not really great either).

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8839042 [details] [diff] [review]
Stop firing pointer events after firing eTouchCancel



I agree with kats that ShouldConvertTouchToPointer should probably take Touch

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Stop firing pointer events after firing eTouchCancel

Followed the review comments to refine the patch. Also add WidgetTouchEvent as one of the arguments of ShouldConvertTouchToPointer because dom::Touch::mMessage is not assigned when handling eTouchStart.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

LGTM

Comment 8

[Ming-Chou Shih [:stone]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> LGTM
Thanks for reviewing my patch.

Comment 9

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00d4a5781b5ac2deeb019fd7650bd65ecaad13ff

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/317ab487c123
[Pointer Event] Stop firing pointer events after firing eTouchCancel. f=smaug. r=kats

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/317ab487c123