Increase the threshold to detect panning direction (touch-action: pan-x/y) (make it harder to trigger pointercancel)

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: smaug, Assigned: kats)

Tracking

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(8 attachments)

Reporter

Description

8 months ago
I got some feedback that we're too strict.
Reporter

Updated

8 months ago
Component: Event Handling → Panning and Zooming
By this do you mean we should let the user's finger move farther before deciding what direction they're trying to go?
Flags: needinfo?(bugs)
Reporter

Comment 2

8 months ago
I got feedback at TPAC that Firefox is very picky with directions. If user happens to do even a tiny bit movement to wrong direction, page gets pointercancel,
Flags: needinfo?(bugs)
Priority: -- → P3
Summary: Increase the threshold to detect panning direction (touch-action: pan-x/y) → Increase the threshold to detect panning direction (touch-action: pan-x/y) (make it harder to trigger pointercancel)
Whiteboard: [gfx-noted]

Comment 3

8 months ago
As I'm the feedback giver from TPAC, I'll add some more context here: noticed this while implementing swipe gestures for a carousel (in Bootstrap - https://github.com/twbs/bootstrap/issues/27531#issuecomment-433719595). Attaching a small demo - a <div> with touch-action:pan-y, with attached pointer events listeners. Note that in Firefox, it's impossible to get more than a single pointermove when swiping horizontally before pointercancel is fired. Compare to Chrome and Edge. Tested on a Surface Pro 2017 and an older Android device with the latest Firefox Nightly (65.0a1).

Comment 5

8 months ago
Posted image pany-chrome.PNG

Comment 6

8 months ago
Posted image pany-edge.PNG

Comment 7

8 months ago
Posted image pany-fx.PNG

Comment 9

8 months ago
also adding that, for what it's worth, decided to go ahead with using touch-action:pan-y in Bootstrap's swipe gesture handling https://github.com/twbs/bootstrap/pull/27582 - so current BS carousels won't be swipeable in Firefox (in case this adds any extra "compat" weight behind the priority on this bug)
Thanks for the testcase! I can reproduce the problem, and will take a look.
Assignee: nobody → kats
I looked into this a little bit. We trigger the pointercancel if we get a nsEventStatus_eConsumeDoDefault result from APZ for a touchmove [1]. This in turns happens if we return true from ArePointerEventsConsumable at [2] and the block is out of the slop state (i.e. the finger has moved "far enough" from the original touchdown position). And ArePointerEventsConsumable errs on the side of returning true [3] and doesn't take into account the direction of panning in the touch block (which in theory should be resolvable by the time the block is out of slop). So the fix here is to tighten up ArePointerEventsConsumable to return false in the case where the events are indicating a pan in a non-pannable direction.

[1] https://searchfox.org/mozilla-central/rev/20df68a5f5b5e078a11fa62a681f09debda61d79/gfx/layers/apz/util/APZEventState.cpp#393
[2] https://searchfox.org/mozilla-central/rev/20df68a5f5b5e078a11fa62a681f09debda61d79/gfx/layers/apz/src/InputQueue.cpp#154
[3] https://searchfox.org/mozilla-central/rev/20df68a5f5b5e078a11fa62a681f09debda61d79/gfx/layers/apz/src/AsyncPanZoomController.cpp#980
I have patches to fix this. I'll put them up and work on a test (might have to spin that off into a follow-up).
This patch tries to reduce the false-positive cases where
ArePointerEventsConsumable returns true even though the input events
won't actually result in panning. It does this by ascertaining the
direction of panning (if possible) in the current input block and
checking to see if panning can actually occur in that direction.
Previously it would just check if panning could occur without taking
into account the actual pan direction of the input events.

Depends on D12823
Depends on: 1509959

Comment 16

7 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f324774fb863
Extract helper methods to be more reusable. r=botond
https://hg.mozilla.org/integration/autoland/rev/67c5cdc1e812
Tighten up the ArePointerEventsConsumable checks. r=botond
https://hg.mozilla.org/integration/autoland/rev/e9e34c0b62ac
Fix a pending TODO that has been resolved. r=botond
Backed out 3 changesets (bug 1502010) for Gtest failure. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=213921181&repo=autoland&lineNumber=22960
INFO -  TEST-PASS | APZCPanningTester.PanWithTouchActionNone | test completed (time: 0ms)
[task 2018-11-26T19:54:59.078Z] 19:54:59     INFO -  TEST-START | APZCPanningTester.PanWithTouchActionPanX
[task 2018-11-26T19:54:59.078Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPanningTester.PanWithTouchActionPanX |       Expected: touchMoveStatus
[task 2018-11-26T19:54:59.080Z] 19:54:59     INFO -        Which is: 2
[task 2018-11-26T19:54:59.080Z] 19:54:59     INFO -  To be equal to: statuses[1]
[task 2018-11-26T19:54:59.082Z] 19:54:59     INFO -        Which is: 0 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/APZTestCommon.h:639
[task 2018-11-26T19:54:59.082Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPanningTester.PanWithTouchActionPanX |       Expected: touchMoveStatus
[task 2018-11-26T19:54:59.083Z] 19:54:59     INFO -        Which is: 2
[task 2018-11-26T19:54:59.084Z] 19:54:59     INFO -  To be equal to: statuses[2]
[task 2018-11-26T19:54:59.085Z] 19:54:59     INFO -        Which is: 0 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/APZTestCommon.h:640
[task 2018-11-26T19:54:59.086Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPanningTester.PanWithTouchActionPanX |       Expected: touchMoveStatus
[task 2018-11-26T19:54:59.087Z] 19:54:59     INFO -        Which is: 2
[task 2018-11-26T19:54:59.089Z] 19:54:59     INFO -  To be equal to: statuses[1]
[task 2018-11-26T19:54:59.090Z] 19:54:59     INFO -        Which is: 0 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/APZTestCommon.h:639
[task 2018-11-26T19:54:59.091Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPanningTester.PanWithTouchActionPanX |       Expected: touchMoveStatus
[task 2018-11-26T19:54:59.092Z] 19:54:59     INFO -        Which is: 2
[task 2018-11-26T19:54:59.093Z] 19:54:59     INFO -  To be equal to: statuses[2]
[task 2018-11-26T19:54:59.093Z] 19:54:59     INFO -        Which is: 0 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/APZTestCommon.h:640
[task 2018-11-26T19:54:59.093Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPanningTester.PanWithTouchActionPanX | test completed (time: 1ms)
[task 2018-11-26T19:54:59.094Z] 19:54:59     INFO -  TEST-START | APZCPanningTester.PanWithTouchActionPanY
[task 2018-11-26T19:54:59.094Z] 19:54:59     INFO -  TEST-PASS | APZCPanningTester.PanWithTouchActionPanY | test completed (time: 0ms)
[task 2018-11-26T19:54:59.095Z] 19:54:59     INFO -  TEST-START | APZCPanningTester.PanWithPreventDefaultAndTouchAction
[task 2018-11-26T19:54:59.095Z] 19:54:59     INFO -  TEST-PASS | APZCPanningTester.PanWithPreventDefaultAndTouchAction | test completed (time: 0ms)
[task 2018-11-26T19:54:59.096Z] 19:54:59     INFO -  TEST-START | APZCPanningTester.PanWithPreventDefault
[task 2018-11-26T19:54:59.096Z] 19:54:59     INFO -  TEST-PASS | APZCPanningTester.PanWithPreventDefault | test completed (time: 1ms)
[task 2018-11-26T19:54:59.096Z] 19:54:59     INFO -  TEST-START | APZCPinchTester.Pinch_DefaultGestures_NoTouchAction
[task 2018-11-26T19:54:59.097Z] 19:54:59     INFO -  TEST-PASS | APZCPinchTester.Pinch_DefaultGestures_NoTouchAction | test completed (time: 0ms)
[task 2018-11-26T19:54:59.097Z] 19:54:59     INFO -  TEST-START | APZCPinchTester.Panning_TwoFinger_ZoomDisabled
[task 2018-11-26T19:54:59.098Z] 19:54:59     INFO -  TEST-PASS | APZCPinchTester.Panning_TwoFinger_ZoomDisabled | test completed (time: 0ms)
[task 2018-11-26T19:54:59.098Z] 19:54:59     INFO -  TEST-START | APZCPinchTester.Panning_Beyond_LayoutViewport
[task 2018-11-26T19:54:59.098Z] 19:54:59     INFO -  TEST-PASS | APZCPinchTester.Panning_Beyond_LayoutViewport | test completed (time: 2ms)
[task 2018-11-26T19:54:59.099Z] 19:54:59     INFO -  TEST-START | APZCPinchTester.Pinch_TwoFinger_APZZoom_Disabled_Bug1354185
[task 2018-11-26T19:54:59.099Z] 19:54:59     INFO -  TEST-PASS | APZCPinchTester.Pinch_TwoFinger_APZZoom_Disabled_Bug1354185 | test completed (time: 0ms)
[task 2018-11-26T19:54:59.100Z] 19:54:59     INFO -  TEST-START | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_NoTouchAction
[task 2018-11-26T19:54:59.100Z] 19:54:59     INFO -  TEST-PASS | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_NoTouchAction | test completed (time: 1ms)
[task 2018-11-26T19:54:59.100Z] 19:54:59     INFO -  TEST-START | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNone
[task 2018-11-26T19:54:59.101Z] 19:54:59     INFO -  TEST-PASS | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNone | test completed (time: 1ms)
[task 2018-11-26T19:54:59.101Z] 19:54:59     INFO -  TEST-START | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionZoom
[task 2018-11-26T19:54:59.102Z] 19:54:59     INFO -  TEST-PASS | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionZoom | test completed (time: 0ms)
[task 2018-11-26T19:54:59.102Z] 19:54:59     INFO -  TEST-START | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNotAllowZoom
[task 2018-11-26T19:54:59.102Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNotAllowZoom |       Expected: expectedMoveStatus
[task 2018-11-26T19:54:59.103Z] 19:54:59     INFO -        Which is: 0
[task 2018-11-26T19:54:59.104Z] 19:54:59     INFO -  To be equal to: statuses[1]
[task 2018-11-26T19:54:59.104Z] 19:54:59     INFO -        Which is: 2 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/APZTestCommon.h:813
[task 2018-11-26T19:54:59.105Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNotAllowZoom |       Expected: expectedMoveStatus
[task 2018-11-26T19:54:59.105Z] 19:54:59     INFO -        Which is: 0
[task 2018-11-26T19:54:59.105Z] 19:54:59     INFO -  To be equal to: statuses[2]
[task 2018-11-26T19:54:59.105Z] 19:54:59     INFO -        Which is: 2 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/APZTestCommon.h:814
[task 2018-11-26T19:54:59.105Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNotAllowZoom |       Expected: expectedMoveStatus
[task 2018-11-26T19:54:59.106Z] 19:54:59     INFO -        Which is: 0
[task 2018-11-26T19:54:59.106Z] 19:54:59     INFO -  To be equal to: statuses[1]
[task 2018-11-26T19:54:59.106Z] 19:54:59     INFO -        Which is: 2 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/APZTestCommon.h:813
[task 2018-11-26T19:54:59.107Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNotAllowZoom |       Expected: expectedMoveStatus
[task 2018-11-26T19:54:59.107Z] 19:54:59     INFO -        Which is: 0
[task 2018-11-26T19:54:59.108Z] 19:54:59     INFO -  To be equal to: statuses[2]
[task 2018-11-26T19:54:59.108Z] 19:54:59     INFO -        Which is: 2 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/APZTestCommon.h:814
[task 2018-11-26T19:54:59.108Z] 19:54:59  WARNING -  TEST-UNEXPECTED-FAIL | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNotAllowZoom | test completed (time: 0ms)
[task 2018-11-26T19:54:59.109Z] 19:54:59     INFO -  TEST-START | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNone_NoAPZZoom
[task 2018-11-26T19:54:59.109Z] 19:54:59     INFO -  TEST-PASS | APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNone_NoAPZZoom | test completed (time: 1ms)
[task 2018-11-26T19:54:59.110Z] 19:54:59     INFO -  TEST-START | APZCPinchGestureDetectorTester.Pinch_PreventDefault

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=e9e34c0b62ac841810517ebb5c5a3dd4c7dd439a

Backout:
https://hg.mozilla.org/integration/autoland/rev/f3849030a93c44b14343bae1cc70709b8a56fa71
Flags: needinfo?(kats)
Looking
Flags: needinfo?(kats)
The PanWithTouchActionPanX test expectation needs updating, since the second patch (the main fix here) directly affects the scenario the test is exercising. As for Pinch_UseGestureDetector_TouchActionNotAllowZoom, that's affected by the third patch on this bug. I'm modifying the test slightly so that it continues testing what it's supposed to in the face of this change.

Try push to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=1bbe52a181395249ec80d5fbc2754557a9b98e18

Comment 20

7 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97091ffa8d25
Extract helper methods to be more reusable. r=botond
https://hg.mozilla.org/integration/autoland/rev/bc5e97142338
Tighten up the ArePointerEventsConsumable checks. r=botond
https://hg.mozilla.org/integration/autoland/rev/ab2123e59f8a
Fix a pending TODO that has been resolved. r=botond

Comment 21

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/97091ffa8d25
https://hg.mozilla.org/mozilla-central/rev/bc5e97142338
https://hg.mozilla.org/mozilla-central/rev/ab2123e59f8a
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.