1932985 - preventDefault-ed wheel event incorrectly interrupts ongoing smooth scroll animation

Status: VERIFIED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: t.html

STR;

1. Open the attachment
2. Try to scroll the content by mouse wheel

You will see a bunch of scroll and scrollend, but the order is something wrong.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:hiro, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Botond Ballo [:botond]]

On Linux, the output I get is:

scroll
scrollend
scrollend
scroll
scrollend
scrollend
scroll
scrollend
scrollend

i.e. for every wheel tick, I get one scroll event and two scrollend events.

The duplicate scrollend event might be a known issue? Dan would probably know.

Are you seeing something different than the above pattern?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I guess this is related to bug 1924193, given that fixing this bug will also fix bug 1924193, I am going to set same priority and severity as the bug.

Comment 4

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

fixing this bug will also fix bug 1924193

I am not convinced that the testcase is this bug is representative of the scenario in bug 1924193. The testcase in this bug scrolls with instant scrolling, while in bug 1924193, the vertical tabs sidebar scrolls with smooth scrolling.

Getting scrollend events between each tick is expected with instant scrolling (even if getting two scrollend events per each tick is not), but not with smooth scrolling.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

fixing this bug will also fix bug 1924193

I am not convinced that the testcase is this bug is representative of the scenario in bug 1924193. The testcase in this bug scrolls with instant scrolling, while in bug 1924193, the vertical tabs sidebar scrolls with smooth scrolling.

I guess this bug's case is not directly tied to instant scroll operations?

Comment 6

[Botond Ballo [:botond]]

(As a side note, by doing window.scrollBy(0, window.scrollY + 1);, the test case scrolls by a quadratically increasing distance with each scroll. I'm guessing this part is unintentional.)

Comment 7

[Botond Ballo [:botond]]

Attachment: Smooth scrolling variant of testcase

Here is a variant of the testcase which does smooth scrolling (and which also does not have the quadratic scrolling behaviour).

Here, if I tick the mouse wheel multiple times in quick succession, the sequence of events is something like:

scroll
scroll
scroll
scroll
scrollend
scroll
scroll
scrollend
scroll
scroll
scrollend
scroll
scroll
scroll
scrollend
scroll
scroll
scroll
scrollend
scroll
scroll
scroll
scroll
scrollend
scroll
scroll
scrollend
scroll
scroll
scroll
scrollend
scroll
scroll
scroll
scroll
scroll
scrollend
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scroll
scrollend

I think the number of scrollend events corresponds to the number of wheel ticks.

Comment 8

[Botond Ballo [:botond]]

An interesting observation about the smooth scrolling testcase is that, even ignoring the scrollend events, it exhibits very sluggish scrolling behaviour.

Though the amount of scrolling is now window.scrollBy(0, 50);, i.e. 50 pixels per wheel tick (similar to what the default scrolling behaviour would be), the actual amount scrolled is much less than with a similar testcase that does not use preventDefault() and scrollBy().

In other words, the revised testcase basically reproduces the sluggish scrolling in bug 1924193, even though in the testcase, the scrollend events don't have any effect (unlike in bug 1924193, where arrowscrollbox.js listens to scrollend events and updates some of its internal state in response).

Comment 9

[Botond Ballo [:botond]]

Here is a stack trace fragment that helps explain both the scrollend events, and the sluggish scrolling:

#17 0x00007f49d774b463 in mozilla::layers::RemoteContentController::NotifyAPZStateChange (this=0x7f49bce0bbe0, aGuid=..., 
    aChange=mozilla::layers::GeckoContentController_APZStateChange::eTransformEnd, aArg=0, aInputBlockId=<error reading variable: Cannot access memory at address 0x0>)
    at /home/botond/dev/projects/mozilla/central/gfx/layers/ipc/RemoteContentController.cpp:225
#18 0x00007f49d76c59a9 in mozilla::layers::AsyncPanZoomController::DispatchStateChangeNotification (this=this@entry=0x7f49a8e9c800, 
    aOldState=mozilla::layers::AsyncPanZoomController::SMOOTHMSD_SCROLL, aNewState=aNewState@entry=mozilla::layers::AsyncPanZoomController::NOTHING)
    at /home/botond/dev/projects/mozilla/central/gfx/layers/apz/src/AsyncPanZoomController.cpp:6468
#19 0x00007f49d76b8ff5 in mozilla::layers::AsyncPanZoomController::SetState (this=0x7f49a8e9c800, aNewState=mozilla::layers::AsyncPanZoomController::NOTHING)
    at /home/botond/dev/projects/mozilla/central/gfx/layers/apz/src/AsyncPanZoomController.cpp:6449
#20 mozilla::layers::AsyncPanZoomController::CancelAnimation (this=0x7f49a8e9c800, 
    aFlags=(mozilla::layers::ExcludeOverscroll | mozilla::layers::ScrollSnap | mozilla::layers::ExcludeWheel))
    at /home/botond/dev/projects/mozilla/central/gfx/layers/apz/src/AsyncPanZoomController.cpp:4290
#21 0x00007f49d76f0d5e in mozilla::layers::OverscrollHandoffChain::CancelAnimations (this=0x7f49a8e4dbc0, 
    aFlags=(mozilla::layers::ExcludeOverscroll | mozilla::layers::ScrollSnap | mozilla::layers::ExcludeWheel))
    at /home/botond/dev/projects/mozilla/central/gfx/layers/apz/src/OverscrollHandoffState.cpp:80
#22 mozilla::layers::InputQueue::CancelAnimationsForNewBlock (this=0x7f49d0314560, aBlock=0x7f49a8bc8ce0, aExtraFlags=mozilla::layers::ExcludeWheel)
    at /home/botond/dev/projects/mozilla/central/gfx/layers/apz/src/InputQueue.cpp:605
#23 mozilla::layers::InputQueue::ReceiveScrollWheelInput (this=this@entry=0x7f49d0314560, aTarget=..., aFlags=..., aEvent=...)
    at /home/botond/dev/projects/mozilla/central/gfx/layers/apz/src/InputQueue.cpp:361
#24 0x00007f49d76f01ad in mozilla::layers::InputQueue::ReceiveInputEvent (this=0x7f49d0314560, aTarget=..., aFlags=..., aEvent=..., aTouchBehaviors=...)
    at /home/botond/dev/projects/mozilla/central/gfx/layers/apz/src/InputQueue.cpp:54

We are calling CancelAnimation() at the time a wheel event is enqueued into the input queue here, when we don't yet know whether it will be prevent-defaulted.

We are passing the ExcludeWheel flag to CancelAnimation(), so in the case where the browser is handling the wheel scrolls, we don't cancel the ongoing animation because it's of type WheelScrollAnimation.

But in the case where the page is handling the wheel scrolls with scrollBy(), the compositor animation is a SmoothScrollAnimation, and we do cancel it. This has the effect that the next scrollBy() will be relative to the current position, rather than relative to the destination of the previous animation (hence the sluggish scrolling), and also the effect of triggering a scrollend event.

Comment 10

[Botond Ballo [:botond]]

I'm wondering if there's any good reason for ReceiveScrollWheelInput to call CancelAnimationsForNewBlock() at all.

The comment inside CancelAnimationsForNewBlock() says (emphasis mine):

// We want to cancel animations here as soon as possible (i.e. without waiting
// for content responses) because a finger has gone down and we don't want to
// keep moving the content under the finger. [...]

i.e. it was written with touch input in mind.

The call in ReceiveScrollWheelInput was introduced in bug 1013432, which introduced wheel input blocks to begin with. That patch seems to have cargo-culted the call over from InputQueue code for dealing with touch events.

In bug 1331194, which introduced the ExcludeWheel flag, Kats contemplated removing the call altogether but ended up opting to keep the scope of the change narrow by continuing to cancel other animation types.

Now that we know canceling at least one other animation type (SmoothScrollAnimation) is problematic, I'm thinking of trying to just remove the call altogether.

Comment 11

[Botond Ballo [:botond]]

Based on the diagnosis in comment 9, the scrollend events are a symptom of the underlying problem, not the cause; revising issue title to reflect this.

Comment 12

[Botond Ballo [:botond]]

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

I'm wondering if there's any good reason for ReceiveScrollWheelInput to call CancelAnimationsForNewBlock() at all.

[...] I'm thinking of trying to just remove the call altogether.

I've confirmed that this fixes bug 1924193 for me.

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1932985 - Do not cancel animations when starting a new wheel block. r=dlrobertson

Comment 14

[Botond Ballo [:botond]]

It turns out my understanding of the situation was incomplete.

Not cancelling ongoing animations when starting a new wheel block does fix bug 1924193, but it does not fix the testcase I attached in this bug (the scrolling in the testcase remains sluggish), which I discovered when I tried to turn the testcase into a mochitest.

I did some debugging to understand why. It turns out that, even if we don't cancel the ongoing smooth scroll animation from the previous scroll, correctly extending the destination of the existing animation by an additional scrollBy delta requires the delta to be communicated to APZ using a relative scroll update, and we don't actually have relative updates hooked up for scrollBy in the smooth scroll case. (The operation calls ScrollToWithOrigin() with ScrollOrigin::Relative, but in the smooth scroll case we use ScrollPositionUpdate::NewSmoothScroll which hardcodes ScrollUpdateType::Absolute regardless of origin.)

I then investigated why the vertical tab bar case does not suffer from this issue. The reason is that the arrowscrollbox.js code does its own tracking of the destination which ensures the deltas are accumulated correctly even in the absence of relative updates.

Finally, I investigated why the arrowscrollbox.js code does suffer from the CancelAnimation issue even though it does its own tracking of the destination: the reason is that CancelAnimation causes a scrollend event to be sent which resets the destination tracking.

Summary:

• The scrollend events are indeed part of the cause of the problem in the arrowscrollbox.js case, not just a symptom.
• The CancelAnimation fix is sufficient to get the arrowscrollbox.js code to work properly.
• Additional fixes would be needed to get the reduced testcase (which does not do its own destination tracking) to work properly (but that's not as high priority since the remaining issues do not affect arrowscrollbox.js).

I will post the CancelAnimation fix for review along with a modified testcase which simulates the arrowscrollbox.js code more closely.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ac654165d7a
Do not cancel animations when starting a new wheel block. r=dlrobertson

Comment 16

[Sandor Molnar[:smolnar]]

Backed out for causing multiple android failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/0fb7c067253f6a7cfcea9162f3e3400f031d1977

Push with failures

Failure log ->

There are multiple android timeouts

Also these mochitest failures popped up - > https://treeherder.mozilla.org/logviewer?job_id=491732139&repo=autoland&lineNumber=13941

TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_wheelevents.html | Test timed out

Comment 17

[Botond Ballo [:botond]]

Attachment: Bug 1932985 - Move apz.test.logging_enabled out of getSmoothScrollPrefs(). r=dlrobertson

Only a subset of the tests that use getSmoothScrollPrefs() use
APZ test logging.

Comment 18

[Botond Ballo [:botond]]

The mochitest failures were intermittent failures of the test being added in this patch; I've addressed that in the updated patch.

The other Android failures look like they were some sort of unrelated issue; here is a Try push with the updated patches rebased onto a recent m-c which shows Android tests are green.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f6c162d69d2
Do not cancel animations when starting a new wheel block. r=dlrobertson,hiro

Comment 20

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7f6c162d69d2

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcd3eb236365
Move apz.test.logging_enabled out of getSmoothScrollPrefs(). r=hiro

Comment 22

[Botond Ballo [:botond]]

(In reply to Pulsebot from comment #21)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcd3eb236365
Move apz.test.logging_enabled out of getSmoothScrollPrefs(). r=hiro

(I forgot to land this as part of the patch series; landing it now.)

Comment 23

[Botond Ballo [:botond]]

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

• Additional fixes would be needed to get the reduced testcase (which does not do its own destination tracking) to work properly (but that's not as high priority since the remaining issues do not affect arrowscrollbox.js).

Filed bug 1944697 to track the remaining issue with the reduced testcase.

Comment 24

[Sandor Molnar[:smolnar]]

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

Comment 25

[Negritas Sergiu, Desktop QA]

Hello! I have managed to reproduce the issue with firefox 134.0a1(2024-11-22) on Ubuntu 22.04.

I can confirm that the issue is fixed with firefox 136.0b4 on Ubuntu 22.04, Windows 10 and MacOS 15.2. I will update the falg and the status of this issue.

Have a nice day!