1655238 - when starting an apz scrollbar drag wait until the drag is confirmed to cancel animations

Status: RESOLVED FIXED

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

Description

[Timothy Nikkel (:tnikkel)]

If the user clicks in the scrollbar track (but not on the thumb) we cancel all animations when we create the new drag input block in InputQueue::ReceiveMouseInput. So this cancels an in progress smooth scroll. It doesn't actually cancel the animation, it clear mAnimation and clears the state on the controller but the animation still runs to completion. Should we file another bug for that?

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1655238. When starting an apz scrollbar drag don't cancel animations. r?kats!,botond

If the user clicks in the scrollbar track (but not on the thumb) we cancel all animations when we create the new drag input block in InputQueue::ReceiveMouseInput. So this cancels an in progress smooth scroll. It doesn't actually cancel the animation, it clear mAnimation and clears the state on the controller but the animation still runs to completion. Should we file another bug for that?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Can you provide specific STR for the problem this bug is attempting to fix, including prefs that need to be set? I'd like to understand it a bit better.

Comment 3

[Timothy Nikkel (:tnikkel)]

Set apz.force_disable_desktop_zooming_scrollbars to false, apz.allow_zooming to true. Load a page with a good amount of vertical scroll. Click 3 times in the scrollbar track to do 3 page scrolls down, do it slowly so each animation has time to finish. Observe the scroll position. Scroll back to the top. Now do the 3 clicks quickly. Observe the scroll position, it will not have scrolled as far as the first time.

This happens because we cancel the inprogress smooth scroll animations. So the code added in bug 1655242 cannot find an in progress animation, and we start a new relative smooth scroll from where ever the scroll offset happens to be.

Bug 1655237 fixed another problem that prevents the code added in bug 1655242 from finding an in progress animation.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Thanks, that helps.

So the goal here is to only stop existing APZ animations when the user mouses down on the scrollthumb. Right now we start a drag input block on any mousedown, and so we stop the animations on any mousedown (including on content area). This doesn't happen if we use the main-thread mechanism for scrollbar paging, because that doesn't happen via an APZ animation.

APZ does know when the user clicks on a scrollthumb vs scrollbar vs neither, but that information just doesn't get plumbed all the way through to the InputQueue. So if we plumb it in, we can make better decisions about when to cancel animations. I'll try and write something.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

For completeness I should also mention there's the case where a scrollbar animation is going, and then you alt+click on a part of the scrollbar track (alt modifier on macOS, might be different on other platforms). This makes the scrollthumb jump to your cursor and puts you into a scrollthumb dragging state. AFAIK we don't handle this kind of scrollbar dragging in APZ, so we can do the easy thing on the APZ side, and not cancel the existing APZ animation (this is easy because it is consistent with the rule of "only cancel animations when the user clicks on the thumb"). Then the main thread will take care of jumping into a dragging state and kill any other animation activity.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1655238 - Add a hit-scrollthumb flag to the TargetConfirmationFlags.

Instead of setting the mTargetConfirmed flag to false in APZCTreeManager when
potentially starting a drag block, let the input block creation do it a little
bit later. This change ensures that the InputQueue ReciveInputEvent methods
have access to the mHitScrollthumb flag, and so can make better decisions.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1655238 - Only cancel existing APZ animations on mousedown when the mousedown lands on a scrollthumb.

Depends on D85452

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I'm attaching the WIPs I wrote. This seems to accomplish the goal of not canceling animations unless the user mouses down on the scrollthumb. However it doesn't fix the STR in comment 3, there might be additional places that trigger cancellation of the animation. It might also be because the code I'm running on is older; I had to manually apply bug 1655237. I would investigate more but it would involve inserting printfs into FrameMetrics.h and I don't feel like waiting for another build right now...

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)

I'm attaching the WIPs I wrote. This seems to accomplish the goal of not canceling animations unless the user mouses down on the scrollthumb. However it doesn't fix the STR in comment 3, there might be additional places that trigger cancellation of the animation. It might also be because the code I'm running on is older; I had to manually apply bug 1655237. I would investigate more but it would involve inserting printfs into FrameMetrics.h and I don't feel like waiting for another build right now...

If you didn't have bug 1655237 in your tree then you probably didn't have bug 1655242, which is also necessary to fix the STR. I can try your patches today.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tnikkel) from comment #9)

If you didn't have bug 1655237 in your tree then you probably didn't have bug 1655242, which is also necessary to fix the STR. I can try your patches today.

Yeah I don't have that either. I guess I should update my tree.

Comment 11

[Timothy Nikkel (:tnikkel)]

Your two WIP patches do fix the bug for me.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

I wrote a test too. Will put that patch up assuming the try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f0198aa8c6324d8a8b126ed381da1fa70e1e259

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Ugh, behaviour is different on linux when clicking on the scrollbar track. I'll have to tweak the test a bit.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=09e8f041a6abb955c0142da92f5e4142a0514fed

Now with ui.scrollToClick=0, which fixes it for me locally on Linux.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1655238 - Add a test. r?botond

Depends on D85453

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d74e81817c7e
Add a hit-scrollthumb flag to the TargetConfirmationFlags. r=botond
https://hg.mozilla.org/integration/autoland/rev/20c5d5880939
Only cancel existing APZ animations on mousedown when the mousedown lands on a scrollthumb. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/9dc94550a9db
Add a test. r=botond

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d74e81817c7e
https://hg.mozilla.org/mozilla-central/rev/20c5d5880939
https://hg.mozilla.org/mozilla-central/rev/9dc94550a9db