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

RESOLVED FIXED in Firefox 42

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mstange, Assigned: botond)

Tracking

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
(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.
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
Created 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
Attachment #8637519 - Flags: review?(chrislord.net)
(Assignee)

Updated

3 years ago
Assignee: nobody → botond

Comment 5

3 years ago
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.
Attachment #8637519 - Flags: review?(chrislord.net) → review+

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
(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

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
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
(Assignee)

Comment 10

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

Comment 11

3 years ago
(Try push is in comment 4.)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d13b99696fc4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.