1692708 - Smooth scroll animations are reset when scrollTop is changed

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

When scrollTop is set, smooth scroll animations are interrupted. This disturbs the user experience on pages that lazily load content and adjust the scroll position in response, like Slack. In fact, Slack is giving Firefox a slow path due to this bug. See bug 1692705 for more details.

Steps to reproduce:

1. Load attachment 9203072 [details].
2. Switch to the setting document.scrollingElement.scrollTop option with the radio button.
3. Using the pageup/pagedown keys, or using an external mouse with discrete mousewheel "notches", scroll up and down.

Expected results:
The smooth scroll animation should complete undisturbed.

Actual results:
The smooth scroll animation is interrupted. In addition, the page is reset to an earlier scroll position, discarding the async scroll offset from the smooth scroll. The latter is bug 1692705, the former is this bug.

Comment 1

[Markus Stange [:mstange]]

I think this aspect is a behavior we've had for a long time, and is not new with APZ.

Comment 2

[Botond Ballo [:botond]]

Thanks for the reports (bug 1692705, bug 1692707, and this one), Markus!

I've marked them as P3 for now, but given the impact on Slack, I'm going to push to prioritize these (likely shortly after Proton).

Comment 3

[Frank Griffith Jr]

Updated Assignee. Part of Jira Epic FFXP-370

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Scroll offset changes with PageDown key

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Scroll offset changes with arrow down key

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692708 - Await promiseApzFlushedRepaints() before calling window.scrollTo(). r?tnikkel

This is the way what other similar tests do (e.g.
helper_zoomToFocusedInput_multiline.html [1] and
helper_zoomToFocusedInput_scroll.html [2]).

[1] https://searchfox.org/mozilla-central/rev/126f016b59513988fae56c2b3f69c1cd23fe6ca1/gfx/layers/apz/test/mochitest/helper_zoomToFocusedInput_multiline.html#35-36
[2] https://searchfox.org/mozilla-central/rev/126f016b59513988fae56c2b3f69c1cd23fe6ca1/gfx/layers/apz/test/mochitest/helper_zoomToFocusedInput_scroll.html#33-34

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692708 - Specify MOUSESCROLL_SCROLL_LINES in synthesizeNativeWheel if apz.test.mac.synth_wheel_input is true. r?tnikkel

Without specifying the value, even if synthesizeNativeWheel causes wheel inputs,
it will be converted to an instant scroll rather than smooth. In fact on Mac
mouse wheeling causes this type of events that is that the events'
hasPreciseScrollingDeltas flag is false.

Depends on D129590

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692708 - Run helper_relative_scroll_smoothness.html with mouse wheel events. r?botond

Depends on D129591

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692708 - Run helper_relative_scroll_smoothness.html with native key events both on Windows and Mac. r?botond

synthesizeKey() just invokes a corresponding command. In "KEY_ArrowDown" case
it's nsISelectionController::ScrollLine [1] which is actually
PresShell::ScrollLine which calls nsIScrollable::ScrollBy, that's not what we
wanted to test for bug 1692707. To exercise APZ's keyboard scrolling we need to
use synthesizeNativeKey instead.

[1] https://searchfox.org/mozilla-central/rev/126f016b59513988fae56c2b3f69c1cd23fe6ca1/dom/base/nsGlobalWindowCommands.cpp#363-364

Depends on D129592

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692708 - Transmogrify ScrollToCSSPixels to ScrollByCSSPixels if there's any on-going APZ animation and there's no on-going async scroll by script. r?botond

Some mochitests for this change is in the next commit.

Depends on D129593

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692708 - Add scrollTo and scrollTop test cases in helper_relative_scroll_smoothness.html. r?botond

Depends on D129594

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692708 - Add cancelScrollAnimation and use it in window_bug1369072.html and browser_scroll.js. r?botond

This test has a pattern something like;

1. Call synthesizeKey to scroll an element
2. Wait a scroll event
3. Test something
4. (Re)set the scroll position to (0, 0) by calling Element.scrollTop and
   Element.scrollLeft

With the new transmogrification the step 4) will not reset the scroll position
if there's an on-going scroll animation in APZ triggered by the synthesizeKey.
To avoid the situation where there's an on-going animation, we simply set the
minimum possible smooth scroll animation parameters to minimize the animation
duration thus the animation will immediately end in the first sampling, so at
the step 2) the test can ensure there's no animation.

Depends on D129593

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1692708 - Make browser_scroll.js work with the new relative transmogrification. r?botond

This test has the same pattern as window_bug1369072.html does.

Call synthesizeKey, wait a scroll event, test something and reset the scrolled
position by calling scrollTo.
We'd set the same preference values in the previous commit.

Depends on D130852

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I did disable helper_relative_scroll_smoothness.html with native-key on Mac's on chaos mode runs (e.g. TV runs) since it fairly fails.

For the record, the failure happens because there appear flat scroll position changes which should have been fixed in bug 1692707. A failure log is;

scroll offset should be strictly monotonically increasing previous offset: 0.01666259765625, offset: 0.01666259765625

I am assuming on chaos mode we somehow sample same scroll position because we miss keyboard input event handling or some such on heavy load state.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d8fb911d61e
Await promiseApzFlushedRepaints() before calling window.scrollTo(). r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/9256faddf559
Specify MOUSESCROLL_SCROLL_LINES in synthesizeNativeWheel if apz.test.mac.synth_wheel_input is true. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/d4b0e298abed
Run helper_relative_scroll_smoothness.html with mouse wheel events. r=botond
https://hg.mozilla.org/integration/autoland/rev/8beac9395793
Run helper_relative_scroll_smoothness.html with native key events both on Windows and Mac. r=botond
https://hg.mozilla.org/integration/autoland/rev/b31c1668258d
Add cancelScrollAnimation and use it in window_bug1369072.html and browser_scroll.js. r=botond
https://hg.mozilla.org/integration/autoland/rev/f26b5d17b34b
Transmogrify ScrollToCSSPixels to ScrollByCSSPixels if there's any on-going APZ animation and there's no on-going async scroll by script. r=botond
https://hg.mozilla.org/integration/autoland/rev/361c96f8f312
Add scrollTo and scrollTop test cases in helper_relative_scroll_smoothness.html. r=botond

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/5d8fb911d61e
https://hg.mozilla.org/mozilla-central/rev/9256faddf559
https://hg.mozilla.org/mozilla-central/rev/d4b0e298abed
https://hg.mozilla.org/mozilla-central/rev/8beac9395793
https://hg.mozilla.org/mozilla-central/rev/b31c1668258d
https://hg.mozilla.org/mozilla-central/rev/f26b5d17b34b
https://hg.mozilla.org/mozilla-central/rev/361c96f8f312