1773378 - Dominant axis scrolling doesn't switch axes reliably

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Steps to reproduce:

1. Be on macOS.
2. Go to https://share.firefox.dev/3H3IIvW .
3. Do some slow scrolling in the call tree, which is scrollable on both axis: In one scroll motion, scroll horizontally to the right, then to the left again, and then slowly to the right but slowly turning downwards until you're scrolling straight down.

Expected results:
During the downwards turn, we should start scrolling downwards as soon as the finger movement direction is more vertical than horizontal.

Actual results:
The downwards turn is ignored and only horizontal scrolling is happening.

Comment 1

[Dan Robertson (:dlrobertson)]

This largely appears to be an issue of parameter tuning. I was able to reproduce this today, and it seems that in this case we get to a state where the breakout threshold is never hit on each call to HandlePanningUpdate (I guess that might be why this only happens with slow scrolls). I couldn't reproduce this once I bumped the breakout threshold down to 1/64. So maybe when enabling dominant axis mode, we should drop the breakout threshold? I do hit bug 1770421 at a much greater frequency, when testing with the lower breakout threshold.

Comment 2

[Botond Ballo [:botond]]

Markus' suggestion of a "stateless" mode (i.e. a mode where we don't set or check mAxisLocked, just unconditionally drop the smaller of the two components) might be a simple solution to both this and bug 1770421.

Comment 3

[Dan Robertson (:dlrobertson)]

(In reply to Botond Ballo [:botond] from comment #2)

Markus' suggestion of a "stateless" mode (i.e. a mode where we don't set or check mAxisLocked, just unconditionally drop the smaller of the two components) might be a simple solution to both this and bug 1770421.

This is a good point and probably the way to go.

Comment 4

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1773378 - Implement stateless dominant axis scrolling. r?botond,mstange

Implement the new dominant axis locking mode for the apz.axis_lock.mode
preference. When using this mode, we do not use the traditional axis locks.
Instead we only consider the input pan displacement for the axis with
a larger value, zeroing out the displacement on the opposite axis.

Comment 5

[Dan Robertson (:dlrobertson)]

The current implementation would also fix bug 1770421. This fixes the test APZCAxisLockTester.TestDominantAxisScrolling to work even in the case that both the panY and panX are equal. Note that this does not follow [:hiro]'s comment from the APZ channel:

I am curious how Safari/Chrome handles the exact 45 degree gesture case. I guess the block direction on the scroll container would be preferable?

Instead the current implementation is biased towards the vertical axis.

Comment 6

[Markus Stange [:mstange]]

I tested the try build and it feels perfect. Thanks a lot!!

I think biasing towards the vertical axis is perfectly fine. Let's not make this more complicated than it needs to be.

Comment 7

[Dan Robertson (:dlrobertson)]

(In reply to Markus Stange [:mstange] from comment #6)

I tested the try build and it feels perfect.

Thanks for testing!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6aefdcf0d53b
Implement stateless dominant axis scrolling. r=botond,mstange

Comment 9

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing mochitest failures on browser_test_background_tab_scroll.js. CLOSED TREE
Backout link
Push with failures
Link to failure log
Failure line :
TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/browser_test_background_tab_scroll.js | Expected background tab to have scrolled up, is at 5000

Comment 10

[bszekely]

There is a second perma fail from the backed out push.

Failure line: TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_bug1151667.html | Test timed out. -

Failure log

Push with failures

Backout link

Comment 11

[Dan Robertson (:dlrobertson)]

Thanks, I'll look into this. I couldn't get this to fail locally, but I'll poke around at things.

Comment 12

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1773378 - Add basic axis lock mode compatibility test. r=botond

Add a basic test to ensure the delta for the pan start gesture is evaluated for
all axis locking modes.

Depends on D152104

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab7464800992
Implement stateless dominant axis scrolling. r=botond,mstange
https://hg.mozilla.org/integration/autoland/rev/79a105583b26
Add basic axis lock mode compatibility test. r=botond

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ab7464800992
https://hg.mozilla.org/mozilla-central/rev/79a105583b26