1756529 - msdPhysics doesn't honor general.smoothScroll.(other|pages)

Status: RESOLVED FIXED

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

Description

[tustamido]

general.smoothScroll.other and general.smoothScroll.pages allow to disable smoothScroll on keybinds that scroll large amounts at once: Home, End, PageUp and PageDown.

But the great general.smoothScroll.msdPhysics currently doesn't follow these two prefs.

Comment 1

[Timothy Nikkel (:tnikkel)]

Are you saying that if you set general.smoothScroll.msdPhysics.enabled to true, and general.smoothScroll.pages to false then you get undesired smooth scrolling when pressing pageup and pagedown?

Comment 2

[tustamido]

Yes.

Comment 3

[Botond Ballo [:botond]]

The non-MSD (Bezier) animation respects this setting here. (Its way of respecting the setting is interesting: it still runs an animation, but with the durations set to zero, thereby appearing instant.)

For MSD, the settings is respected in a different way by this check, which avoids runs the animation altogether. However, this check is only performed for mousewheel events, not keyboard events.

Comment 4

[Botond Ballo [:botond]]

Fixing this should be fairly straightforward, would make a nice mentored bug.

Comment 5

[Matteo Ruello]

I am going to take a look at this, as discussed with @botond

Comment 6

[Botond Ballo [:botond]]

Thanks Matteo! I assigned the bug to you.

Comment 7

[Matteo Ruello]

Attachment: Bug 1756529 ScrollPage now honors general.smoothScroll.page preference with MSD physics r?botond

Comment 8

[Matteo Ruello]

Attachment: Bug 1756529 - Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r?botond

Depends on D139793

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58f10ceb16ba
Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r=botond,hiro

Comment 10

[Iulian Moraru]

Backed out for causing mochitest failures on test_group_keyboard.html.

Push with failures

Failure log for when it fails with | Scrolled more than once, but expecting an instant scroll[origin: line; msdPhysics: false] - got 2, expected 1
Failure log for when it fails with | The page did not scroll [origin: other, smooth: false]
Failure log for when it fails with | The page did not scroll [origin: line, smooth: true]

Backout link

Comment 11

[Botond Ballo [:botond]]

Based on the logs, it looks like there are some "carry-over" scroll offset updates from the previous test case. I have an idea for a fix; will do a Try push that runs the test on all platforms this time.

Comment 12

[Botond Ballo [:botond]]

Unfortunately, the test is still failing a lot: https://treeherder.mozilla.org/jobs?repo=try&revision=1097c5179dd26658f0311b1fc63a0d43e05335bb

Comment 13

[Botond Ballo [:botond]]

I'm not able to reproduce a test failure locally in repeated runs. Unfortunately, that means getting this test to be reliable enough to land is going to be an uphill battle...

Comment 14

[Matteo Ruello]

Are you testing on Windows? Seems like it does not fail there, right?

Comment 15

[Matteo Ruello]

It seems like the scroll counter is not incremented even on multiple scrolls (I see multiple redraws, but a single increment at the end) Is it possible that we don't parse the counter thread until the UI is completely redrawn?

Comment 16

[Botond Ballo [:botond]]

(In reply to Matteo Ruello from comment #14)

Are you testing on Windows? Seems like it does not fail there, right?

There are test failures on all three desktop platforms; here is one on Windows. Locally I'm running Linux.

Comment 17

[Botond Ballo [:botond]]

I've been continuing to investigate this using Try pushes with added logging. It seems like one issue is that the window.scrollTo(0, 0) call is not effective at restoring the scroll position to 0 for the next test case.

Part of the reason for this is that, since bug 1692708, the scrollTo() gets translated into a relative scroll (meaning, if the current scroll position is, say, 100, it gets intereprested as "scroll by -100" rather than "scroll to 0"). Relative scrolls do not interrupt ongoing smooth scroll animations; rather, they adjust the destination of animation by the relative delta (that happens here).

Unfortunately, it looks like this adjustment is not implemented correctly for MSD scroll physics. Specifically, while ScrollAnimationMSDPhysics::ApplyContentShift() (called from here) adjusts ScrollAnimationMSDPhysics::mDestination, that value is never actually read; the copy of the destination stored internally by the AxisPhysicsMSDModel is what actually needs to adjusted.

Comment 18

[Botond Ballo [:botond]]

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

Unfortunately, it looks like this adjustment is not implemented correctly for MSD scroll physics. Specifically, while ScrollAnimationMSDPhysics::ApplyContentShift() (called from here) adjusts ScrollAnimationMSDPhysics::mDestination, that value is never actually read; the copy of the destination stored internally by the AxisPhysicsMSDModel is what actually needs to adjusted.

Filed bug 1759958 for this.

Comment 19

[Botond Ballo [:botond]]

Here's a Try push with the patches from bug 1759958: https://treeherder.mozilla.org/jobs?repo=try&revision=543f65cdd0c17748c6bbfba9a4374af08ce70bf1.

The number of failures is significantly reduced, but there are still some, most of them on Mac.

Comment 20

[Botond Ballo [:botond]]

At least some of the Mac failures are due a line scroll only getting as far as 48px when the test is expecting at least 50px.

The test uses line-height: 100px;, and seems to expect a line scroll to scroll by that amount (I assume the 50px is just a conservative bound).

The actual distance of a line scroll is three lines, with the height of a line determined by the scroll frame's "line scroll amount". The "line scroll amount" seems to be 19px on Linux and 16px on Mac, resulting in total scroll distances of 57px and 48px, respectively.

The line-height: 100px; seems to have no impact on the "line scroll amount".

Comment 21

[Botond Ballo [:botond]]

Updated the patch to use a different method of controlling the distance of the line scroll, by using the pref toolkit.scrollbox.verticalScrollDistance to increase the number of lines scrolled from 3 to 5.

Here's a new Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=6d7075e02159d84a7bef9fc3136fb2ffbaf05fe9. There are now fewer failures on Mac, but there are still some remaining ones.

Comment 22

[Botond Ballo [:botond]]

The remaining Mac failures were caused by the fact that even if the relative scroll update from window.scrollTo(0, 0) is handled correctly, if the smooth scroll animation is still in progress, we're still going to get residual scrolling from it that interferes with the next test case. We really want to do something that cancels any in-progress animation to properly clean up the current testcase.

I looked around for what would do that, and settled on sending a synthesized touch-tap event. The touch-down interrupts all in-progress animations, since a touch is supposed to "hold" the page in place.

This then caused another complication, which is that in the case of instant scrolling test cases, two touch-taps could be sent in quick enough succession that they'd be gesture-detected as a double-tap and trigger double-tap-zooming, which also interferes with subsequent test cases. I worked around this by running the tests with apz.allow_double_tap_zooming=false.

With these changes, the test seems to be green on Try: https://treeherder.mozilla.org/jobs?repo=try&revision=d68664e752783d90dd16dfcb8a3e0f90123d2d14.

(Having to synthesize a touch-tap to interrupt ongoing scrolling animations feels a bit hackish. It would probably be nicer to add a new test API specifically for the purpose of cancelling ongoing animations.)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4be195c46471
Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r=botond,hiro

Comment 24

[Cristian Tuns]

Backed out for causing mochitest failures in test_group_keyboard.html

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_keyboard.html | Expected to be scrolled to origin, actually scrolled to 1 - got +0, expected 1

Comment 25

[Botond Ballo [:botond]]

Hmm. The failures that I see on the autoland push are:

• test-windows10-32-2004-qr/debug-mochitest-plain-fis-e10s-3 - intermittent failure, on 4/7 runs
• test-windows10-64-2004-qr/debug-mochitest-plain-spi-nw-fis-e10s-3 - intermittent failure, on 4/6 runs

But on my Try push, I've retriggered both of the above jobs 10 times and they pass 10/10 times.

Comment 26

[Botond Ballo [:botond]]

I did another Try push from the autoland branch, and this time the failures are readily reproducible (they happened on the first try).

The issue seems to be that while the test synthesized a touch-tap to interrupt the animation, it wasn't correctly waiting for the consequences to be propagated to the main thread. (It was calling promiseApzFlushedRepaints(), but the notification that sends to the compositor can get there before the synthesized touch events are processed, and therefore before any repaints have been enqueued.)

When this happens, the test can get to the scrollTo(0, 0) while there is still a repaint pending from the last time the animation was sampled prior to being cancelled. In such a situation, the relative delta from the scrollTo(0, 0) will be combined with the scroll from that last animation sample (often (0, 1)), and result in the scroll offset ending up at (0, 1).

This can be solved by waiting for the synthesized touch event to be dispatched to the page (then we know it has been processed by APZ), then calling promiseApzFlushedRepaints() and finally the scrollTo().

Patch with that change is green on Try, 10/10 retriggers of the jobs in question: https://treeherder.mozilla.org/jobs?repo=try&revision=8be920025304eaab944d6db1459ec39165b2b90a

Comment 27

[Botond Ballo [:botond]]

Unfortunately, while passing on Windows, the revised test is failing (intermittently but frequently) on Linux, though I can't reproduce the failure locally: https://treeherder.mozilla.org/jobs?repo=try&revision=9f6549f0968e3451003897a6456aa973e293556e

Comment 28

[Botond Ballo [:botond]]

I can't really justify spending more time investigating this test. It seems to pass reliably on platforms other than Linux, so let's land the patch with the test disabled on Linux.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb898843abd1
Honor general.smoothScroll.[line|page|other] for keyboard scrolls with MSD physics r=botond,hiro

Comment 30

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fb898843abd1