1042103 - Don't ignore touch events while in an overscroll animation

Status: RESOLVED FIXED

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

Description

[Mason Chang [Inactive] [:mchang]]

While showing the overscroll animation, we ignore touch events so we can't scroll down or up to quickly get out of the overscroll. This creates a situation that is unresponsive for the user. Our partner also brought up this issue. We should change it so that we can still scroll away from an overscroll animation. I talked to :botond, and he said this was a UX design decision. 

UX team, we can please change this or what was the original reason for this? Thanks!

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Mason, it may be better to bring these topics up during the gfx-ux meeting (graphics vidyo, Fridays 11am PDT) and have a live discussion.
Also, don't think this is perf in the definition we've been using.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Right now, this is a conversation with UX about a potential change of the existing feature; once that resolves itself into "we are to make a change in APZ", please change the product/component.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Could this also explain bug 1036307?

Comment 4

[Mason Chang [Inactive] [:mchang]]

Hi Botond, since UX is taking a while to look at this, is it possible to get a patch to process touch events even though we're in an overscrolled position? The partner would like to be able to test this on their own. Thanks!

Comment 5

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

Mason, if you're free at 2pm eastern today you should join the ux/gfx meeting in the Graphics vidyo room. There's a bunch of stuff the partner brought up that should be run by UX and it would be faster if you (or somebody else who was there) could explain it to Gordon directly. If not I can bring these items up there.

Comment 6

[Gordon Brander :gordonb]

IIRC this was a workaround because "catching" the overscroll with a tap was too high of a technical risk for 2.0. We hope to continually improve on scrolling behavior and physics over subsequent releases.

Comment 7

[Mason Chang [Inactive] [:mchang]]

After discussion with UX, we will be re-examining scrolling behavior and scrolling physics around release 2.2. We can look at this bug again during that time. 

Another feature that other devices have is that a user can catch the overscroll and hold it in position. Since we currently have a 200ms animation to prevent catching, this is perceived by UX to be an edge case as a user probably both doesn't want to hold it in position or do something within 200ms of hitting an overscroll animation.

Comment 8

[Botond Ballo [:botond]]

Since bug 1066888 has landed, a number of people have expressed feedback that not responding to touch events while the animation is in progress is a poor UX.

I plan to look at fixing this in the near future. In the meantime, Gordon, would it be possible to tweak the prefs to make the animation a bit shorter?

Comment 9

[Benoit Girard (:BenWa)]

Requesting 2.2+:

I hit this bug all the time while testing and it make the phone appear very janky and unresponsive as it ignore my touch events while in this state. This give a very poor user experience.

This downside of this bug is larger than the upside of an overscroll animation. I feel strongly that this should be a release blocker. We should either fix this OR disable overscroll animations until we're ready to fix this.

Comment 10

[Benoit Girard (:BenWa)]

[Blocking Requested - why for this release]:

Actually the problem is still there for 2.1 so we should consider it for that release as well. Even when you shorten the animation, as I tried in a build, input events are still ignored during the overscroll. This is no obvious to the user and looks like the phone is not responsive.

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> In the meantime, Gordon,
> would it be possible to tweak the prefs to make the animation a bit shorter?

After pulling the latest code, I see this was already done. Thanks Gordon!

(In reply to Benoit Girard (:BenWa) from comment #10)
> [Blocking Requested - why for this release]:
> 
> Actually the problem is still there for 2.1 so we should consider it for
> that release as well. Even when you shorten the animation, as I tried in a
> build, input events are still ignored during the overscroll. This is no
> obvious to the user and looks like the phone is not responsive.

We should discuss this in Portland.

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Benoit Girard (:BenWa) from comment #10)
> [Blocking Requested - why for this release]:
> 
> Actually the problem is still there for 2.1 so we should consider it for
> that release as well. Even when you shorten the animation, as I tried in a
> build, input events are still ignored during the overscroll. This is no
> obvious to the user and looks like the phone is not responsive.

I think its too late to any changes in 2.1 and leaning on getting these improved in 2.2, can we get more information for QA to see what events are exactly being ignored and see if these are worth release blocking issues (regressions), which is what we are down to in 2.1 at this point. Also NI'ing milan for more input here given this issue has been existed for a while..

Comment 13

[Botond Ballo [:botond]]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> can we get more information for QA to see what events are
> exactly being ignored and see if these are worth release blocking issues

I can try to describe the issue in more detail:

When you pan into overscroll, stretching the page, and release the finger, an animation kicks in that restores you to a non-overscrolled state. While this animation is in progress, touch events (e.g. pan gestures, taps) on the element being scrolled are ignored.

In 2.1, the animation to recover from overscroll is very brief, as it stops as soon as the page returns to its "resting state".

In 2.2, the animation to recover from overscroll is longer, as the page briefly oscillates around its resting state before coming to a halt.

Accordingly, this issue affects 2.2 to a greater degree than 2.1.

Comment 14

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

While it would be nice to fix this bug, we have other deliverables for the 2.2 timeframe that take precedence. At this point we should assume that this will not get fixed in the near future (and certainly will not be fixed in 2.1). If Gordon wants to disable overscroll entirely instead (on 2.1 and/or on 2.2) then we can do that for now and revisit this issue when we have more time.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm with Kats on this one - pick one of the solutions we already had and run with it.  No time to keep coming up with the new ones.

Comment 16

[bhavana bajaj [:bajaj]]

Moving this to 2.2 and :milan/:kats can prioritize as they see fit.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

I'd rather not do this at all, I'm not convinced it's worth it.  If there are issues with overscroll taking too long, let's fix that.

Comment 18

[Gordon Brander :gordonb]

This behavior wasn't really by design... IIRC it was a byproduct of a shortcut we took earlier. But we do what we can with what we have. If re-prioritization means we have to come up with a short-term fix for 2.1.1. we can do that.

Comment 19

[Chris Lord [:cwiiis]]

I think this issue is exacerbated by the weird behaviour that when one axis overscrolls, it waits until all scrolling is finished before snapping back (you can trigger some very weird behaviour on a page that has both horizontal and vertical overscroll) - I'll file another bug for this.

Comment 20

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

(In reply to Milan Sreckovic [:milan] from comment #17)
> I'd rather not do this at all, I'm not convinced it's worth it.  If there
> are issues with overscroll taking too long, let's fix that.

I'd disagree with this - I think this bug is worth fixing eventually. We just don't have the cycles for it at the moment, and a shorter-term fix would be to roll back to 2.1 overscroll behaviour (kinda risky) or just disable overscroll in 2.2 entirely (easy to do).

That being said, Chris, if you're willing to fix this bug we can probably guide you through it. It might turn out to be non-trivial but it might also turn out to be rather simple because a fair amount of the input and transforms code has changed in the last little while.

Comment 21

[Chris Lord [:cwiiis]]

Just to have it on record, I'm willing to take a shot at this - but I may not have the time to do it imminently (PTO from 23rd-9th). I'd love to have this fixed though.

Comment 22

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

Awesome! Maybe at some point before the 9th Botond and/or I will take a quick look and braindump what we think should be the approach to tackle this, and you can take it when you get back from PTO.

Comment 23

[Gordon Brander :gordonb]

That would be amazing, Chris!

(Cross-post from https://bugzilla.mozilla.org/show_bug.cgi?id=1113068#c2 for context)

From a design standpoint, the bounce animation could be much much better than what we have in 2.1 (assuming bug 1113068 and bug 1042103 land).

However, I'm most concerned about saving the other scrolling work we have lumped in with the overscroll changes. My understanding is that if we roll back to 2.1 behavior we lose fling curving, etc.

Comment 24

[Botond Ballo [:botond]]

(In reply to Gordon Brander :gordonb from comment #23)
> However, I'm most concerned about saving the other scrolling work we have
> lumped in with the overscroll changes. My understanding is that if we roll
> back to 2.1 behavior we lose fling curving, etc.

I don't think that's necessarily the case. I can see us removing the oscillation from the spring animation without reverting other changes that have been made to scrolling behaviour since 2.1.

Comment 25

[Gordon Brander :gordonb]

(In reply to Botond Ballo [:botond] from comment #24)
> I don't think that's necessarily the case. I can see us removing the
> oscillation from the spring animation without reverting other changes that
> have been made to scrolling behaviour since 2.1.

Not ideal -- it would be great to have working oscillation -- but that scenario is way better than rolling back to 2.1 behavior.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Hard stop to solve this in 37 train, without uplifts.  That means done with the fixes and all the potential fallout and follow up by Jan 12.   (2015 :)

Comment 27

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Awesome! Maybe at some point before the 9th Botond and/or I will take a
> quick look and braindump what we think should be the approach to tackle
> this, and you can take it when you get back from PTO.

Here's an outline of what I think needs to be done:

  - The function used by APZ to perform hit testing is
    APZCTreeManager::GetTargetAPZC(ScreenPoint, HitTestResult*).

    Currently, if a touch hits an overscrolled APZC, this function
    returns nullptr, and it returns HitTestResult::OverscrolledAPZC
    via its out-parameter.

    We'll want to remove the logic in the implementation of
    GetTargetAPZC (and its helper, GetAPZCAtPoint) for dealing with
    overscroll, treating an overscrolled APZC the same as any other
    APZC. (The HitTestResut::OverscrolledAPZC enumerator can be
    removed.)

    At the call sites of GetTargetAPZC which check for the hit test
    result OverscrolledAPZC and ignore the event [1] [2], we'll
    want to remove those checks.

  - In AsyncPanZoomController::OnTouchStart(), we'll want to handle
    the case OVERSCROLL_ANIMATION. The handling should be the same
    as for other animations (fling, animating zoom, smooth scroll),
    i.e. we want to cancel the animation, except we don't want 
    the CancelAnimation() call to clear the overscroll [3]. We may
    need to add a flag to CancelAnimation() to not clear the
    overscroll in this case.

    One thing I haven't considered is what behaviour we want in the
    case when the overscroll animation is in a 'compression' phase.
    However, given that the compression phase will be going away
    in bug 1096513, we may not have to worry about this.

It's possible that other things I haven't thought of will come up when implementing this; if that happens, please feel free to post here to discuss.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=3f980229dfc1#702
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=3f980229dfc1#843
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=3f980229dfc1#2161

Comment 28

[Botond Ballo [:botond]]

Oh, forgot a rather important bit: when transforming event coordinates, we need to take into account the overscroll transform.

This involves adjusting the following use sites of AsyncPanZoomController::GetCurrentAsyncTransform() to also multiply in GetOverscrollTransform():

  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=3f980229dfc1#1403
  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=3f980229dfc1#1563
  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=3f980229dfc1#1595

It might make sense to just have GetCurrentAsyncTransform() itself include the overscroll transform in its return value, but then we need to make sure to adjust this code in AsyncCompositionManager [1], which looks at GetCurrentAsyncTransform() via SampleContentTransformForFrame() and currently specifically assumes that it does _not_ contain the overscroll transform.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=3f980229dfc1#597

Comment 29

[Chris Lord [:cwiiis]]

Going to have a look at this, and marking it as dependent on bug 1096513 (which removes underscroll, making this a bit easier).

Comment 30

[Chris Lord [:cwiiis]]

Attachment: Don't ignore events during overscroll

So... All this patch does is cancel the overscroll animation on touch-start (and not ignore the events during overscroll).

I was just about to delve in and do the harder work of retaining overscroll on touch-start, but actually this already feels fine... Overscroll is so transient and so small that it's practically impossible to notice that this cancels it, and it has the benefit that you can't get into silly situations where you can get loads of overscroll (iOS and Android cap the total of overscroll, but I don't think we do anywhere - there's likely a limit based on our spring simulation, but it's probably huge).

I would actually say, let's commit this first, and handle retaining overscroll on touch-start if and when that becomes an issue - because with the current settings, I really can't tell (and I'd like to think I'm quite sensitive to these things).

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8550345 [details] [diff] [review]
Don't ignore events during overscroll

Review of attachment 8550345 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

(In reply to Chris Lord [:cwiiis] from comment #30)
> I was just about to delve in and do the harder work of retaining overscroll
> on touch-start, but actually this already feels fine... Overscroll is so
> transient and so small that it's practically impossible to notice that this
> cancels it, 

The prefs were tweaked to make overscroll really short precisely to work around this bug (see bug 1066888 comment 20). My understanding is that the animation we really want is a bit longer, and the idea is to make it so once this bug is fixed.

That said, this is definitely an improvement to the status quo, so it makes sense to land this as-is. We can then talk to Gordon and see how to proceed.

> (iOS and Android cap the total of
> overscroll, but I don't think we do anywhere - there's likely a limit based
> on our spring simulation, but it's probably huge).

We cap it to a percentage of the composition length, currently 15% [1], though that too may increase when we revisit the prefs.

In addition, additional clamping is provided by the spring physics during a fling, and by "resistance" during a pan [2].

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js?rev=f925d9830745#1024
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp?rev=d65c9aa7d0ad#167

Comment 32

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Allow overscroll to be retained between input blocks

This applies on top of the first patch, but implements fully what has been spoken about in this bug (to allow overscroll to be retained between input blocks).

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8551383 [details] [diff] [review]
Part 2 - Allow overscroll to be retained between input blocks

Review of attachment 8551383 [details] [diff] [review]:
-----------------------------------------------------------------

Great - this is actually looking pretty clean!

A few comments:

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1276,1 @@
>      SetState(NOTHING);

Don't SetState(NOTHING) if we started an overscroll animation (SnapBackIfOverscrolled() returned true) - we want the state to remain OVERSCROLL_ANIMATION in that case.

@@ +2180,5 @@
>    // Setting the state to nothing and cancelling the animation can
>    // preempt normal mechanisms for relieving overscroll, so we need to clear
>    // overscroll here.
> +  if (!(aFlags & EXCLUDE_OVERSCROLL)
> +      && (mX.IsOverscrolled() || mY.IsOverscrolled())) {

(APZC now has a method IsOverscrolled() too, we could use that here.)

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +301,2 @@
>     */
> +  void CancelAnimation(const uint32_t aFlags = 0);

I'd prefer making this parameter (and the corresponding one in OverscrollHandoffChain::CancelAnimations) have type CancelAnimationFlags. We can move the enum to OverscrollHandoffState.h to make this happen.

Also toplevel consts on parameter types are superfluous.

::: gfx/layers/apz/src/Axis.cpp
@@ +167,5 @@
>  }
>  
>  void Axis::OverscrollBy(ParentLayerCoord aOverscroll) {
>    MOZ_ASSERT(CanScroll());
> +  StopSamplingOverscrollAnimation();

Might it be cleaner to call this in CancelAnimation() instead? Feels like that would be "closer to the source".

(Once upon a time AsyncPanZoomAnimation had a virtual "Cancel" method (which CancelAnimation() called). OverscrollAnimation's implementation of that would have been a particularly nice place to put this.)

@@ +205,5 @@
>  
> +void Axis::StopSamplingOverscrollAnimation() {
> +  ParentLayerCoord overscroll = GetOverscroll();
> +  ClearOverscroll();
> +  mOverscroll = overscroll;

Heh, clever :)

Comment 34

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #33)
> Comment on attachment 8551383 [details] [diff] [review]
> Part 2 - Allow overscroll to be retained between input blocks
> 
> Review of attachment 8551383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great - this is actually looking pretty clean!
> 
> A few comments:
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1276,1 @@
> >      SetState(NOTHING);
> 
> Don't SetState(NOTHING) if we started an overscroll animation
> (SnapBackIfOverscrolled() returned true) - we want the state to remain
> OVERSCROLL_ANIMATION in that case.

Whoops, nice spot.

> @@ +2180,5 @@
> >    // Setting the state to nothing and cancelling the animation can
> >    // preempt normal mechanisms for relieving overscroll, so we need to clear
> >    // overscroll here.
> > +  if (!(aFlags & EXCLUDE_OVERSCROLL)
> > +      && (mX.IsOverscrolled() || mY.IsOverscrolled())) {
> 
> (APZC now has a method IsOverscrolled() too, we could use that here.)

Will do.

> ::: gfx/layers/apz/src/AsyncPanZoomController.h
> @@ +301,2 @@
> >     */
> > +  void CancelAnimation(const uint32_t aFlags = 0);
> 
> I'd prefer making this parameter (and the corresponding one in
> OverscrollHandoffChain::CancelAnimations) have type CancelAnimationFlags. We
> can move the enum to OverscrollHandoffState.h to make this happen.
> 
> Also toplevel consts on parameter types are superfluous.

Will do. I wanted to forward-declare the enum (which we can do now we've updated the minimum compiler to gcc 4.6 and newer msvc), but I couldn't do both that and have a default argument :(

> ::: gfx/layers/apz/src/Axis.cpp
> @@ +167,5 @@
> >  }
> >  
> >  void Axis::OverscrollBy(ParentLayerCoord aOverscroll) {
> >    MOZ_ASSERT(CanScroll());
> > +  StopSamplingOverscrollAnimation();
> 
> Might it be cleaner to call this in CancelAnimation() instead? Feels like
> that would be "closer to the source".
> 
> (Once upon a time AsyncPanZoomAnimation had a virtual "Cancel" method (which
> CancelAnimation() called). OverscrollAnimation's implementation of that
> would have been a particularly nice place to put this.)

I thought this, but I decided to leave it in Axis because it has the knowledge of when mOverscroll might be modified, and I guess it's an implementation detail in a way. I don't like the idea that you could call what are meant to be public functions on Axis and end up in an odd state... I'll leave this unless you say otherwise - obviously your call here, you know the code a lot better than I do and you'll be maintaining it I imagine :)

Comment 35

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #34)
> > ::: gfx/layers/apz/src/Axis.cpp
> > @@ +167,5 @@
> > >  }
> > >  
> > >  void Axis::OverscrollBy(ParentLayerCoord aOverscroll) {
> > >    MOZ_ASSERT(CanScroll());
> > > +  StopSamplingOverscrollAnimation();
> > 
> > Might it be cleaner to call this in CancelAnimation() instead? Feels like
> > that would be "closer to the source".
> > 
> > (Once upon a time AsyncPanZoomAnimation had a virtual "Cancel" method (which
> > CancelAnimation() called). OverscrollAnimation's implementation of that
> > would have been a particularly nice place to put this.)
> 
> I thought this, but I decided to leave it in Axis because it has the
> knowledge of when mOverscroll might be modified, and I guess it's an
> implementation detail in a way. I don't like the idea that you could call
> what are meant to be public functions on Axis and end up in an odd state...
> I'll leave this unless you say otherwise - obviously your call here, you
> know the code a lot better than I do and you'll be maintaining it I imagine
> :)

Good point!

On the flip side, someone might add a new Axis function that accesses mOverscroll, but forget to call StopSamplingOverscrollAnimation() before that access.

As there is a case for either approach, I'm cool with leaving it as-is.

Comment 36

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Allow overscroll to be retained between input blocks v3

Addressed the first couple of comments - I didn't move the enum because OverscrollHandoffChain.h isn't included in AsyncPanZoomController.h, so it wouldn't solve the problem.

I didn't move the calls to StopSamplingOverscrollAnimation for the reason I mentioned in the comment above, but I'm happy to change this if you'd prefer.

Comment 37

[Botond Ballo [:botond]]

Comment on attachment 8551424 [details] [diff] [review]
Part 2 - Allow overscroll to be retained between input blocks v3

Review of attachment 8551424 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

> I didn't move the enum because
> OverscrollHandoffChain.h isn't included in AsyncPanZoomController.h, so it
> wouldn't solve the problem.

How about APZUtils.h? It just seems like a shame to introduce an enum and then not use it in a signature.

Comment 38

[Chris Lord [:cwiiis]]

Moved enum to APZUtils.h and pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/886bde607d7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/992050dc1e5f

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/886bde607d7c
https://hg.mozilla.org/mozilla-central/rev/992050dc1e5f

Comment 40

[Chris Lord [:cwiiis]]

Comment on attachment 8550345 [details] [diff] [review]
Don't ignore events during overscroll

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: During overscroll, the user cannot interact with the page
Testing completed: On master and my dog-food device for a week, no obvious problems
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

Comment 41

[Chris Lord [:cwiiis]]

Comment on attachment 8551424 [details] [diff] [review]
Part 2 - Allow overscroll to be retained between input blocks v3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: A jarring transition may be observed when scrolling during an overscroll animation
Testing completed: On master and my dog-food device for a week, no obvious problems
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

Comment 42

[bhavana bajaj [:bajaj]]

Comment on attachment 8551424 [details] [diff] [review]
Part 2 - Allow overscroll to be retained between input blocks v3

Flagging no-jun to get some testing around this and keep an eye on any fallouts we may observe.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8f22f5dfb661
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8c0722089f9f

Comment 44

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

\o/ Thanks, Chris!

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Non-unified bustage followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8138b61835ca
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a3d027cf2e97

Comment 46

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8138b61835ca

Comment 47

[No-Jun Park [:njpark]]

I did some checks on 2.2 and 3.0 and did not find graphical regressions particularly related to the overscroll fix.

Comment 48

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #35)
> (In reply to Chris Lord [:cwiiis] from comment #34)
> > > ::: gfx/layers/apz/src/Axis.cpp
> > > @@ +167,5 @@
> > > >  }
> > > >  
> > > >  void Axis::OverscrollBy(ParentLayerCoord aOverscroll) {
> > > >    MOZ_ASSERT(CanScroll());
> > > > +  StopSamplingOverscrollAnimation();
> > > 
> > > Might it be cleaner to call this in CancelAnimation() instead? Feels like
> > > that would be "closer to the source".
> > > 
> > > (Once upon a time AsyncPanZoomAnimation had a virtual "Cancel" method (which
> > > CancelAnimation() called). OverscrollAnimation's implementation of that
> > > would have been a particularly nice place to put this.)
> > 
> > I thought this, but I decided to leave it in Axis because it has the
> > knowledge of when mOverscroll might be modified, and I guess it's an
> > implementation detail in a way. I don't like the idea that you could call
> > what are meant to be public functions on Axis and end up in an odd state...
> > I'll leave this unless you say otherwise - obviously your call here, you
> > know the code a lot better than I do and you'll be maintaining it I imagine
> > :)
> 
> Good point!
> 
> On the flip side, someone might add a new Axis function that accesses
> mOverscroll, but forget to call StopSamplingOverscrollAnimation() before
> that access.
> 
> As there is a case for either approach, I'm cool with leaving it as-is.

Heh, we ended up being bitten by this in bug 1152051, although for a different reason: 

  - CancelAnimation() had been modified not to call ClearOverscroll() in some cases.

  - Such a call would clear the animation, but not the animation state stored in Axis.

  - A subsequent animation would then do silly things like use the original
    animation's mFirstOverscrollAnimationSample as its own.