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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

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

I got some feedback that we're too strict.

Comment 1

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

By this do you mean we should let the user's finger move farther before deciding what direction they're trying to go?

Comment 2

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

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,

Comment 3

[Patrick H. Lauke]

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 4

[Patrick H. Lauke]

Attachment: event-listener_touch-action-pany.html

Comment 5

[Patrick H. Lauke]

Attachment: pany-chrome.PNG

Comment 6

[Patrick H. Lauke]

Attachment: pany-edge.PNG

Comment 7

[Patrick H. Lauke]

Attachment: pany-fx.PNG

Comment 8

[Patrick H. Lauke]

Attachment: pany-fxnightly-android.png

Comment 9

[Patrick H. Lauke]

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)

Comment 10

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

Thanks for the testcase! I can reproduce the problem, and will take a look.

Comment 11

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

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

Comment 12

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

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).

Comment 13

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

Attachment: Bug 1502010 - Extract helper methods to be more reusable. r?botond

Comment 14

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

Attachment: Bug 1502010 - Tighten up the ArePointerEventsConsumable checks. r?botond

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

Comment 15

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

Attachment: Bug 1502010 - Fix a pending TODO that has been resolved. r?botond

Depends on D12824

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Dorel Luca [:dluca]]

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

Comment 18

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

Looking

Comment 19

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

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

[Pulsebot]

Pushed by [e-mail]:
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

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/97091ffa8d25
https://hg.mozilla.org/mozilla-central/rev/bc5e97142338
https://hg.mozilla.org/mozilla-central/rev/ab2123e59f8a