1343775 - Can get stuck in overscroll after low-velocity pan

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

STR:
  1. Enable apz.overscroll.enabled
  2. Pan slowly into overscroll
  3. Release the finger

Expected results:
  The page snaps back from overscroll.

Actual results:
  The page remains stuck in overscroll.

Note that we're not currently shipping overscrolling on any platform (except Android where it involves a different codepath), so this bug doesn't currently affect any platform in the default configuration. However, the underlying code is still in the tree (and we plan to enable it for Mac in the near future), so we should still fix it. It would make a good mentored bug.

This bug is a regression introduced by bug 1260905, which added an early-exit path to the code that starts a fling in OnTouchEnd(); we were relying on a fling animation starting unconditionally after a pan, and then the fling transitioning into an overscroll animation, to relieve overscroll.

Comment 1

[Gregory Moore [:gmoore]]

Hi, I'd like to start working on this and was wondering if I could get some more information. From what I can tell so far, we should be changing the code here:
https://dxr.mozilla.org/mozilla-central/rev/39607304b774591fa6e32c4b06158d869483c312/gfx/layers/apz/src/AsyncPanZoomController.cpp#1275
so that it stops early-exiting and starts a fling animation (and then add an automated test to make sure it's fixed). So my main question would just be how to detect that we shouldn't early-exit and should actually start the fling animation. Please let me know, thanks.

Comment 2

[Botond Ballo [:botond]]

Great, thanks! I've assigned the bug to you.

Let's start with writing the testcase, and making sure that it's failing for you (since in bug 1204502, you weren't seeing the failure that I was).

The failing scenario for me was basically:

  - Pan into overscroll, with a low velocity. "Low" here means "lower than
    the apz.fling_min_velocity_threshold pref" [1], whose default value is 0.5 [2].
    (This should then trigger the mentioned early-exit path.)
    
    The velocity of the fling that follows the pan will be calculated as
    roughly the distance of the pan divided by the time taken by the pan.

    For pans performed using the Pan() function in gtests, the time taken
    will be 100 ms, because the Pan() function puts a 50 ms pause between
    the touch events it generates [3], and it generates four touch events
    which therefore have three pauses between them, two of which are
    counted towards the velocity (the first pause between the touch-start
    and the first touch-move is not counted).

    The distance of the pan is something you control based on the arguments
    to Pan().

  - Optional but good to check: use IsOverscrolled() to check that we are 
    overscrolled at the end of the pan.

  - Use AdvanceAnimationsUntilEnd() to wait for the overscroll to be relieved.

  - Check !IsOverscrolled() to ensure that the overscroll has in fact been
    relieved. This is the assertion we're expecting to fail prior to our fix.

We can write this test case in TestBasic.cpp [4] because it doesn't require multiple layers, and so we don't need to deal with setting up a layer tree.

Give that a try please, and post the resulting testcase. If you're not able to get a testcase that fails (which may happen, due to the as-yet-unknown reason why the test in bug 1204502 didn't fail for you), post the testcase anyways. I'll check if it fails for me, and if it does, we can investigate why it _doesn't_ fail for you.
    
[1] http://searchfox.org/mozilla-central/rev/c2a60adfc7b16761cbbfcefa2093fa402ba1aa69/gfx/layers/apz/src/AsyncPanZoomController.cpp#250
[2] http://searchfox.org/mozilla-central/rev/c2a60adfc7b16761cbbfcefa2093fa402ba1aa69/modules/libpref/init/all.js#657
[3] http://searchfox.org/mozilla-central/rev/c2a60adfc7b16761cbbfcefa2093fa402ba1aa69/gfx/layers/apz/test/gtest/APZTestCommon.h#457
[4] http://searchfox.org/mozilla-central/rev/c2a60adfc7b16761cbbfcefa2093fa402ba1aa69/gfx/layers/apz/test/gtest/TestBasic.cpp

Comment 3

[Gregory Moore [:gmoore]]

Attachment: Added new test case to TestBasic.cpp that (currently) fails.

Great, thanks for all of the information. I created a new test case that is failing for me (and I believe is doing the appropriate actions and failing for the right reason). 

When I run it the second assertion:
> EXPECT_FALSE(apzc->IsOverscrolled());
will fail. If I make the pan longer/faster however (e.g., start at y = 10 but end at y = 60, so the velocity is greater or equal to 0.5 (apz.fling_min_velocity_threshold pref)), then the test case passes. So I think the test is working correctly.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8848821 [details] [diff] [review]
Added new test case to TestBasic.cpp that (currently) fails.

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

Thanks, this looks good!

::: gfx/layers/apz/test/gtest/TestBasic.cpp
@@ +315,5 @@
>  }
>  
> +// Tests that the page doesn't get stuck in an
> +// overscroll animation after a low-velocity pan.
> +TEST_F(APZCBasicTester, OverSrollAfterLowVelocityPan_Bug1343775) {

typo: "Sroll"

Comment 5

[Botond Ballo [:botond]]

Now, for the fix. 

To summarize the issue: previously, the assumption was that every pan would be followed by a fling animation (which is the page continuing to scroll after the finger left it due to the "momentum" of the pan), even if it's very brief, and we would check if we need to relieve overscroll at the end of the fling animation.

Bug 1260905 broke that assumption by introducing an early-exit path in AsyncPanZoomController::OnTouchEnd() that would not start the fling animation if the velocity was sufficiently low.

The simplest way to fix this that I can think of, is to run the code to relieve overscroll in the early-exit path. We already have a case elsewhere in OnTouchEnd() where we need to do that [1], so just doing the same thing in the early-exit case should work.

[1] http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/gfx/layers/apz/src/AsyncPanZoomController.cpp#1240

Comment 6

[Gregory Moore [:gmoore]]

Attachment: Fixed bug and updated test case a little.

(In reply to Botond Ballo [:botond] from comment #5)
> The simplest way to fix this that I can think of, is to run the code to
> relieve overscroll in the early-exit path. We already have a case elsewhere
> in OnTouchEnd() where we need to do that [1], so just doing the same thing
> in the early-exit case should work.
Oh okay, I see. That was simpler than I thought it would be. :)

I added a line that relieves the overscroll, and now the test is passing for me. Let me know if there are any problems with the patch or any cases I didn't think of. Thanks.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8848931 [details] [diff] [review]
Fixed bug and updated test case a little.

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

Looks good, thanks! I can confirm that the test fails for me without the fix, and passes with it.

Comment 8

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3afe6e8ee678ea1958540d383708ce5d966dfb62

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd44b9b13bf9
Relieve overscroll after low-velocity pan so page doesn't get stuck in overscroll. r=botond

Comment 10

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

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

Comment 11

[Botond Ballo [:botond]]

Gregory, nice work here and in bug 1204502! Let me know if you're interested in working on another bug and would like me to suggest one.

Comment 12

[Gregory Moore [:gmoore]]

(In reply to Botond Ballo [:botond] from comment #11)
> Gregory, nice work here and in bug 1204502! Let me know if you're interested
> in working on another bug and would like me to suggest one.
Thanks! Also thanks for all the assistance with these bugs. Yeah, I am definitely interested in working on another bug. If you have any, please let me know, thanks.

Comment 13

[Botond Ballo [:botond]]

(In reply to Gregory Moore [:gmoore] from comment #12)
> Yeah, I am
> definitely interested in working on another bug. If you have any, please let
> me know, thanks.

Bug 1180799 might be a nice one. It's a bit more involved, but much of the work is already done; it should just be a matter of taking the partially finished patch attached to the bug, and finishing it as described in bug 1180799 comment 16.