1186164 - Axis::ClearOverscrollAnimationState() can change the sign of Axis::GetOverscroll()

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

With my work-in-progress patches for overscroll on OS X, I sometimes hit a case where APZ triggers an overscroll animation when there shouldn't be any overscroll.

STR (using my wip patches which you don't have):
 1. Go to a long page and scroll to a vertical scroll position of ~100.
 2. Scroll upwards really quickly, triggering overscroll at the top of the page
    (mY.GetOverscroll() < 0).
 3. Release the touchpad, so that the overscroll animation kicks in.
 4. While the page is bouncing, start to scroll downwards quickly.
 6. Release the touchpad.

Now the APZC thinks it's overscrolled on the *bottom* of the page and plays an overscroll animation, even though the current scroll position is only a small distance from the top of the page.

What's happening is that the first overscroll at the top of the animation (mY.GetOverscroll() < 0) bounces back and forth and back such that mY.mOverscroll is positive. Then the new pan starts and calls mY.AdjustDisplacement during AsyncPanZoomController::AttemptScroll, which calls Axis::ClearOverscrollAnimationState(), which leaves mOverscroll positive but resets mLastOverscrollPeak and mOverscrollScale, resulting in mY.GetOverscroll() > 0. Now scrolling downwards will not relieve the overscroll and lifting the fingers will trigger another overscroll animation.

Comment 1

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #0)
> What's happening is that the first overscroll at the top of the animation

Er, this part of the sentence doesn't make much sense... I was talking about the overscroll animation of the first overscroll, which happened at the top edge of the page.

Comment 2

[Botond Ballo [:botond]]

Eek!

This is a regression from bug 1152051. That patch replaced calls in Axis::AdjustDisplacement() and Axis::OverscrollBy() of an earlier function, StopSamplingOverscrollAnimation(), with the new function ClearOverscrollAnimationState().

StopSamplingOverscrollAnimation() would set mOverscroll to the value of GetOverscroll() before the state was cleared. ClearOverscrollAnimationState() should be doing that, too.

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1186164 - When clearing the overscroll animation state, make sure GetOverscroll() continues to reflect the correct direction of overscroll. r=Cwiiis

Bug 1186164 - When clearing the overscroll animation state, make sure GetOverscroll() continues to reflect the correct direction of overscroll. r=Cwiiis

Comment 4

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56d1f9666e32

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 8637519 [details]
MozReview Request: Bug 1186164 - When clearing the overscroll animation state, make sure GetOverscroll() continues to reflect the correct direction of overscroll. r=Cwiiis

r+, with comment addressed.

Comment 6

[Chris Lord [:cwiiis]]

https://reviewboard.mozilla.org/r/13921/#review12879

Looks fine, just one comment I'd like answered before pushing.

::: gfx/layers/apz/src/Axis.cpp:240
(Diff revision 1)
>  void Axis::EndOverscrollAnimation() {

Should this also set mOverscroll to zero? Seems pointless having this function if all it does is call another function and nothing else.

Comment 7

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #6)
> https://reviewboard.mozilla.org/r/13921/#review12879
> 
> Looks fine, just one comment I'd like answered before pushing.
> 
> ::: gfx/layers/apz/src/Axis.cpp:240
> (Diff revision 1)
> >  void Axis::EndOverscrollAnimation() {
> 
> Should this also set mOverscroll to zero? 

Nope. There are times when we want the overscroll animation to stop, but the existing overscroll amount to be preserved; namely, when you put a finger down during the overscroll animation (i.e. the thing we implemented in bug 1042103). In this case we call CancelAnimation() with aFlags = ExcludeOverscroll, which destroys the animation (calling EndOverscrollAnimation() from the destructor) but preserves mOverscroll.

> Seems pointless having this
> function if all it does is call another function and nothing else.

My rationale for having a separate function for this was symmetry: we call Axis::StartOverscrollAnimation() from the OverscrollAnimation constructor, so I wanted to have a function Axis::EndOverscrollAnimation() that we call from the OverscrollAnimation destructor (whereas ClearOverscrollAnimationState() is called from a number of places). Not a big deal though, I can remove EndOverscrollAnimation().

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #7)
> (In reply to Chris Lord [:cwiiis] from comment #6)
> > https://reviewboard.mozilla.org/r/13921/#review12879
> > 
> > Looks fine, just one comment I'd like answered before pushing.
> > 
> > ::: gfx/layers/apz/src/Axis.cpp:240
> > (Diff revision 1)
> > >  void Axis::EndOverscrollAnimation() {
> > 
> > Should this also set mOverscroll to zero? 
> 
> Nope. There are times when we want the overscroll animation to stop, but the
> existing overscroll amount to be preserved; namely, when you put a finger
> down during the overscroll animation (i.e. the thing we implemented in bug
> 1042103). In this case we call CancelAnimation() with aFlags =
> ExcludeOverscroll, which destroys the animation (calling
> EndOverscrollAnimation() from the destructor) but preserves mOverscroll.
> 
> > Seems pointless having this
> > function if all it does is call another function and nothing else.
> 
> My rationale for having a separate function for this was symmetry: we call
> Axis::StartOverscrollAnimation() from the OverscrollAnimation constructor,
> so I wanted to have a function Axis::EndOverscrollAnimation() that we call
> from the OverscrollAnimation destructor (whereas
> ClearOverscrollAnimationState() is called from a number of places). Not a
> big deal though, I can remove EndOverscrollAnimation().

The symmetry sounds good, I'd say keep it.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8637519 [details]
MozReview Request: Bug 1186164 - When clearing the overscroll animation state, make sure GetOverscroll() continues to reflect the correct direction of overscroll. r=Cwiiis

Bug 1186164 - When clearing the overscroll animation state, make sure GetOverscroll() continues to reflect the correct direction of overscroll. r=Cwiiis

Comment 10

[Botond Ballo [:botond]]

I ended up inlining ClearOverscrollAnimationState() into EndOverscrollAnimation() (and replacing other calls of ClearOverscrollAnimationState() with calls to EndOverscrollAnimation()). Carrying r+.

Comment 11

[Botond Ballo [:botond]]

(Try push is in comment 4.)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d13b99696fc4

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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