1695598 - key arrow held down scrolling never updates main thread scroll position (depending on how key repeat events are sent to us)

Status: RESOLVED FIXED

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

Description

[Timothy Nikkel (:tnikkel)]

If you set up a test with a rAF that sends key arrow down then the main thread will never get a scroll update from apz of the scrolling that apz is performing. This can also happen when the user holds down the down arrow key, depending on timing of the key repeat events.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1695598. Allow apz to update the main thread scroll offset if an APZ scroll animation is pending on the main thread or has been sent from the main thread. r?botond

Apz can already update the main thread scroll offset if apz has informed the main thread that it has received a scroll animation and is running it.

This situation can happen when the user holds down an arrow key, or in a test situation if one sends a key event every frame via requestAnimateFrame.

The problem happens in APZCCallbackHelper because aFrame->IsScrollAnimating(IncludeApzAnimation::No) returns true so we don't update the main thread scroll offset from the repaint request. aFrame->IsScrollAnimating(IncludeApzAnimation::No) returns true because there is a smooth scroll request either pending on the main thread, or sent in the last transaction, but not yet acknowledged by apz back to the main thread. (Scroll animations that are acknowledged by apz back to the main thread are ignored in IsScrollAnimating because of the IncludeApzAnimation::No argument.)

So we need to change IsScrollAnimating. However there are two other callers of it:

1. ScrollFrameHelper::SaveState (passes IncludeApzAnimation::No)
2. everything else (passes IncludeApzAnimation::Yes)

We can leave 2) alone, but 1) needs the current behaviour of IncludeApzAnimation::No. This is because there are tests that check that we save mDestination (the destination of the smooth scroll) and restore it. However we do not want to save mDestination if IsApzAnimationInProgress because mDestination has already been overwritten with the current scroll position from apz (via APZCCallbackHelper->ScrollToCSSPixelsApproximate).

So we change IncludeApzAnimation::No to ignore all apz animations (what APZCCallbackHelper needs) and introduce a new value of the enum for what SaveState needs.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1695598. Add test. r?botond

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8da88fe33542
Allow apz to update the main thread scroll offset if an APZ scroll animation is pending on the main thread or has been sent from the main thread. r=botond
https://hg.mozilla.org/integration/autoland/rev/971ed5085fe8
Add test. r=tnikkel

Comment 4

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/8da88fe33542
https://hg.mozilla.org/mozilla-central/rev/971ed5085fe8