1720240 - Incorrectly hit overscroll bounce with nested scroll frames

Status: RESOLVED FIXED

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

Description

[Timothy Nikkel (:tnikkel)]

Load https://searchfox.org/mozilla-central/source/gfx/layers/apz/test/mochitest/helper_test_select_zoom.html
Resize your browser so that there is a small amount of scroll.
Pinch zoom in.
Use touchpad to scroll up and down.

I get overscroll bounce after every gesture even though the scrollbars indicate I am not near the end/start.

Comment 1

[Botond Ballo [:botond]]

On this Searchfox page, the code view is a scrollable subframe.

So, when you pinch-zoom in, we get two nested scrollables (the viewport, and the subframe).

If you scroll all the way to the right, you can see the two different scrollbars side by side (may be easier to see on a platform like Linux with non-hiding scrollbars).

If you then perform vertical scroll gestures and watch the scrollbars, you can see that the first gesture scrolls the subframe, and we get a bounce at the end, and then the next gesture scrolls the viewport, and then we get another bounce.

Comment 2

[Botond Ballo [:botond]]

So, I don't think the behaviour here is incorrect, in the sense of triggering a bounce on a scroll frame which is not at the end of its scroll range (we just happen to have two nested scroll frames with small scroll ranges).

The interaction between overscroll and scroll handoff is also working as intended, though admittedly it leads to a suboptimal experience in this case.

Comment 3

[Timothy Nikkel (:tnikkel)]

I don't just get two bounces, I keep getting bounces as I scroll in one direction.

Safari seems to handle this even worse.

Chrome seems to handle it pretty well.

Comment 4

[Botond Ballo [:botond]]

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

I don't just get two bounces, I keep getting bounces as I scroll in one direction.

Hmm, ok, maybe on Mac there's something else going on. On Linux I only get two bounces.

Comment 5

[Botond Ballo [:botond]]

Going to mark this as S3 for now, as we don't know of other affected sites (but it may be worth investigating by someone who can repro it, as understanding the mechanism of failure might give us a better idea of what sorts of sites are likely to be affected).

Comment 6

[Timothy Nikkel (:tnikkel)]

I'm also able to reproduce a similar effect when neither scroll frame is near a boundary with https://bug1719913.bmoattachments.org/attachment.cgi?id=9231273 from bug 1719913, using a lot of smaller scrolls/flings quickly in a row, changing direction when I have scrolled as far as I can in one direction (no need for pinch zooming).

Comment 7

[Botond Ballo [:botond]]

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

I'm also able to reproduce a similar effect when neither scroll frame is near a boundary with https://bug1719913.bmoattachments.org/attachment.cgi?id=9231273 from bug 1719913, using a lot of smaller scrolls/flings quickly in a row, changing direction when I have scrolled as far as I can in one direction (no need for pinch zooming).

I can't reproduce this on Linux either, though I do see a different issue: when overscrolling the subframe at the top, the overscroll effect can get stuck if the cursor is near the top of the subframe's composition bounds (I think that may be related to bug 1701831).

Comment 8

[Timothy Nikkel (:tnikkel)]

I also got a similar effect scrolling a chat on facebook where there was a continued overscroll bounce on the root scroll frame (I assume) while I scrolled the chat (the chat was not in an overscroll situation while I scrolled it). No pinch zooming.

Comment 9

[Botond Ballo [:botond]]

Sounds like we may want to bump this to an S2 as it's been observed on popular websites.

Comment 10

[Timothy Nikkel (:tnikkel)]

I should note that I don't see this all of the time on Facebook, only sometimes.

Comment 11

[Timothy Nikkel (:tnikkel)]

Real world site where I see this

https://www.ilovewinnipegrealestate.ca/steve-snyder-on-urban-development-and-land/

It also has two nested scrollable scroll frames.

Comment 12

[Frank Griffith Jr]

We can stage this within our APZ Stabilization Sprint I FFXP-370

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: Screen Recording 2021-10-26 at 5.13.15 AM.mov

This is what I see. I'm doing a lot of quick short flings in a row until I get to the end, then I switched direction of the flings. It might be easier to see if you slow down the video.

Comment 14

[Markus Stange [:mstange]]

I hit this whenever there are nested scroll frames. As long as there's an ongoing overscroll animation, handoff is wonky, and we re-overscroll the inner or outer scroll frame which is still running its overscroll animation.

Comment 15

[Markus Stange [:mstange]]

You can reproduce this in the bugzilla comment box text field, for example. (Just need to add enough line breaks to make it scrollable.)

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Okay, I think I've found a reliable way to reproduce the issue Timothy reported. The way is,

1. Try overscrolling at the edge of the inner scroller
2. During the overscroll is still happening, try scrolling in an opposite direction
3. Keep scrolling during the overscroll animation is running

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1720240 - Clamp overscroll animation positions to zero to not get across over zero. r?botond

It does make sense to use the given velocity only for the |aStart| APZC since
the velocity is actually consumed by the APZC, not for ancestor APZCs. Rather
using the velocity for ancestors is destructive. For example, if there are two
nested scrollable frames and if the parent one is overscrolled at the top edge,
and if a new pan gesture tries to scroll downwards on the child frame, using the
downward velocity for the parent APZC will interfere the overscroll animation on
the parent APZC, in fact the velocity stops the overscroll animation [1], but
leaves the overscroll amount there, thus it causes new overscroll animations
repeatedly.

[1] https://searchfox.org/mozilla-central/rev/4615b544a0f7166233c409c619b426c4025467a7/gfx/layers/apz/src/Overscroll.h#93-96

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: A gtest to replicate this issue

Here is a patch to add a gtest which just outputs an AxisPhysicsMSDModel's positions.

The AxisPhysicsMSDModel has the same parameters for our OverscrollAnimation. It starts with -12.159691 overscroll position, and with 1.260920 velocity, both values are the ones I got on my macbook. These values means that something like the root APZC is overscrolled at the top and the user flings strongly downward. Then the gtest samples new positions with 16.6ms delta until the position is lower than 3.0. Then after that, the gtest sets the velocity to 1.260920 again;

The gtest outputs;

4.314508
13.960362
19.118424
(snip)
3.198266
2.757841
Drive! <-- sets the velocity again
18.884461
27.698922
31.772487
...

This is actually what's going on.

I did check our AxisPhysicsModel implementation, and it looks fine.

So I believe using zero velocity for ancestor APZCs (that I actually did in D138091) is a way to fix this bug.

That said, as you guys may notice, this case can also happen in the case of a single scroller, I've actually confirmed it in the case where there's only the root (content) APZC and fling downwards during overscrolling at the top edge. But that is less noticeable, given that we haven't yet been reported the case, which means it's quite hard to notice even if it happens on Mac.

Unfortunately as of now I have no idea how to fix the latter case.

Comment 19

[Botond Ballo [:botond]]

Thanks! This illustrates the problem to me very clearly: if we are overscrolled at the top (mOverscroll < 0), the overscroll animation should never be allowed to change mOverscroll to be positive (which means "overscrolled at the bottom").

I guess there is nothing in the code currently enforcing that, but it should be straightforward to add:

• Track which end of the scroll range we are currently overscrolled at. This can be represented as a SideBits. It looks like OverscrollAnimation already receives this in the constructor, it just needs to store it, and then pass it to Axis in StartOverscrollAnimation().
• In Axis::SampleOverscrollAnimation(), if the physics model changes mOverscroll to a "past the zero" value (e.g. to a positive value when the side bits indicate we are overscrolled at the top), then clamp mOverscroll to zero and enter the finished branch.

Let me know if that makes sense.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Thank you, Botond. I will give it a shot. A reason I didn't do the way is, I guess it may be seen as if overscroll animation is clashed at the boundary. Maybe it's also un-noticeable.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69fe0ae3f5ae
Clamp overscroll animation positions to zero to not get across over zero. r=botond

Comment 22

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/69fe0ae3f5ae

Comment 23

[Timothy Nikkel (:tnikkel)]

ni myself to check a few testcases with this fixed to see if I need to file any followups for other issues.

Comment 24

[Timothy Nikkel (:tnikkel)]

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

ni myself to check a few testcases with this fixed to see if I need to file any followups for other issues.

I don't remember what specific test cases I was thinking about anymore. I don't remember seeing this problem recently. Clearing needinfo.