[Pointer Event] Stop firing pointer events after firing eTouchCancel

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: stone, Assigned: stone)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Assignee: nobody → sshih
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
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
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.
Attachment #8838461 - Attachment is obsolete: true
Attachment #8839042 - Flags: review?(bugmail)
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).
Attachment #8839042 - Flags: review?(bugmail)
Attachment #8839042 - Flags: review+
Attachment #8839042 - Flags: feedback?(bugs)
Comment on attachment 8839042 [details] [diff] [review]
Stop firing pointer events after firing eTouchCancel



I agree with kats that ShouldConvertTouchToPointer should probably take Touch
Attachment #8839042 - Flags: feedback?(bugs) → feedback+
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.
Attachment #8839042 - Attachment is obsolete: true
Attachment #8840337 - Flags: review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> LGTM
Thanks for reviewing my patch.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/317ab487c123
[Pointer Event] Stop firing pointer events after firing eTouchCancel. f=smaug. r=kats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/317ab487c123
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.