1944697 - Window.scrollBy() does not use a relative scroll update with smooth scrolling

Status: NEW

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

Description

[Botond Ballo [:botond]]

Steps to reproduce

1. Ensure smooth scrolling is enabled
2. Load https://bugzilla.mozilla.org/attachment.cgi?id=9445987. This is a page which listens for wheel events, calls preventDefault() on them, and scrolls using scrollBy() instead (originally posted in bug 1932985).
3. Scroll down with the mousewheel, with multiple ticks in succession

Expected results

The page scrolls at a normal speed, comparable to the speed it would scroll at if the wheel events were handled natively by the browser (modulo system settings affecting scrolling speed).

Actual results

The page scrolls very slowly.

Diagnosis

In this scenario (smooth scrolling + wheel ticks in quick succession), when a subsequent wheel event is received, the smooth scroll animation from the previous wheel tick is still ongoing. To scroll by the full delta of both wheel events, the destination of the ongoing animation needs to be extended by the delta of the new event.

The animation is running on the compositor, and the scrolling for a new wheel event happens via window.scrollBy(), so it's sent to the compositor as a scroll update during a main thread transaction. We do have logic to "extend the destination of the existing animation" here, but it requires that the scroll update type be relative (or "pure relative").

However, we do not have window.scrollBy() hooked up to produce relative scroll updates in the case of smooth scrolling. (The operation calls ScrollToWithOrigin() with ScrollOrigin::Relative, but in the smooth scroll case we use ScrollPositionUpdate::NewSmoothScroll which hardcodes ScrollUpdateType::Absolute regardless of origin.)

Comment 1

[Botond Ballo [:botond]]

Attachment: WIP: [WIP] Bug 1944697 - Use the destination of an ongoing smooth scroll as the baseline for a relative scroll

Comment 2

[Botond Ballo [:botond]]

Attachment: WIP: [WIP] Bug 1944697 - Use a relative scroll update for smoth scrolls where appropriate

Comment 3

[Botond Ballo [:botond]]

Attachment: WIP: [WIP] Bug 1944697 - Add a mochitest

Comment 4

[Botond Ballo [:botond]]

I prototyped some patches that fix this to evaluate whether they fix bug 1949977 -- they do not.

What fixing this bug would do is make it so that the arrowscrollbox.js code wouldn't need to do its own destination tracking, because similar destination tracking logic would now be done internally by the platform.

The problem in bug 1949977 is that the destination tracking is thrown off by scrollend events received in the middle of a touchpad pan gesture. The same underlying problem throws off the platform's internal destination tracking (to be specific, the scrollend events are caused by APZ state transitions triggered by a CancelAnimation call, which also has the effect of cancelling the ongoing compositor scroll animation), so these patches make no difference there.

Comment 5

[Botond Ballo [:botond]]

I'm not planning to do further work on this issue until it's shown to affect a real-world web page or a part of the browser UI (such as the tab bar, which I've determined to be currently unaffected in comment 4).