Closed Bug 1695598 Opened 2 years ago Closed 10 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
100 Branch
Tracking Status
firefox100 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

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.

Severity: -- → S3
Priority: -- → P3
Blocks: 1758352
Attachment #9206594 - Attachment description: Bug 1695598. Add test. r?botond → WIP: Bug 1695598. Add test. r?botond
Attachment #9206594 - Attachment description: WIP: Bug 1695598. Add test. r?botond → Bug 1695598. Add test. r?botond
Pushed by tnikkel@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch
See Also: → 1569599
You need to log in before you can comment on or make changes to this bug.