Closed Bug 1292034 Opened 3 years ago Closed 3 years ago

Scrolling quickly, then slowly, moves too fast (vertical speed conservation)

Categories

(Firefox for Android :: Toolbar, defect, P2)

49 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: jimbo1qaz, Assigned: kats)

References

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160606113944

Steps to reproduce:

- Use touchscreen to scroll up/down quickly (the page must begin gliding / inertial scrolling).
- (optional) Touch the screen to stop movement, but do not swipe the other direction.
- Scroll the same direction, but slowly.


Actual results:

The screen glides quickly, using the "stored" inertia from previous scrolls.


Expected results:

The screen glides slowly, using inertia from the current scroll.
(yes, I named the title after https://www.youtube.com/watch?v=6md3RA8bH40 )
I was able to reproduce this using:
Device:Nexus 9(Android 6.0.1)
Build:Firefox for Android - Release - 48, Beta - 49.0b1, Aurora - 50.0a2, Nightly - 51.0a1
Status: UNCONFIRMED → NEW
Component: General → Graphics, Panning and Zooming
Ever confirmed: true
Sounds like an issue that Markus reported before and that we worked on but didn't fix completely. I can't find the old bugs now.
Priority: -- → P2
It was bug 1260905, but we didn't file a follow-up bug for the remaining issue. But this bug perfectly describes my experience.
Assignee: nobody → bugmail
I reproduced this with a bunch of logging and I believe it's happening due to the flywheel behaviour of the StackScroller which we are now using on Android [1]. Ordinarily this wouldn't be a problem but I think there's a bug in the way we are using this code. For example:

1) user starts a fling animation, and velocity starts off high (say ~15 px/ms)
2) we sample a few animation frames, and the velocity comes down to say 13 px/ms
3) user puts their finger down to scroll again. we stop sampling the animation
4) user does a small pan and lifts finger (this gets picked up as a fling)
5) we start a new slow fling animation (say ~1 px/ms)
6) at this point the 1 px/ms velocity is augmented by the leftover velocity from the first fling (13 px/ms) and results in a new velocity of 14 px/ms.

The thing to note above is that the same process happens regardless of how long we spend in step 5, because during step 5 we're not advancing the clock on the first fling animation, and so its velocity isn't slowing down. If it did slow down, it might be at something like 3 px/ms by the time we did step 6, and so the resulting velocity would be a more reasonable 4 px/ms instead of 14 px/ms.

The reason this doesn't happen all the time is because their flywheel detection code is stupid, because it (a) uses Math.signum and (b) checks both axes together. So if your x-axis component changes from negative to 0 or positive, it bypasses the flywheel code. And on long pages that actually happens a lot because you hit the edge of the page horizontally pretty quickly. That's another thing we should fix.

[1] http://searchfox.org/mozilla-central/rev/fd672b97f13aa83af5f04caa7b61bd443fb623e9/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/StackScroller.java#264-267
I kicked off a Fennec try build with fixes for the issues I identified in the previous comment:

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

IMO it scrolls way too fast, but feel free to try it and let me know what you think.
The first two patches here fix the issues identified in comment 5. The third patch adds some more pref-based control over when the flywheel behaviour kicks in, so that it doesn't go bananas all the time. If with these patches the issue in comment 0 is still visible, you should able to bump apz.fling_accel_min_velocity higher and eliminate it.
(Also I kicked off a try build from MozReview, so if you want a build to test it out, you can get it from there).
Comment on attachment 8799026 [details]
Bug 1292034 - Improve the controls over when flywheel/fling acceleration kicks in.

https://reviewboard.mozilla.org/r/84322/#review82938

::: modules/libpref/init/all.js:636
(Diff revision 1)
>  pref("apz.displayport_expiry_ms", 15000);
>  pref("apz.enlarge_displayport_when_clipped", false);
> -pref("apz.fling_accel_base_mult", "1.0");
>  pref("apz.fling_accel_interval_ms", 500);
> +pref("apz.fling_accel_min_velocity", "1.5");
> +pref("apz.fling_accel_base_mult", "1.0");

Prefs are no longer in alphabetical order.
Comment on attachment 8799024 [details]
Bug 1292034 - Make the StackScroller flywheel detection code more robust to real user behaviour when flinging.

https://reviewboard.mozilla.org/r/84318/#review82944
Comment on attachment 8799024 [details]
Bug 1292034 - Make the StackScroller flywheel detection code more robust to real user behaviour when flinging.

https://reviewboard.mozilla.org/r/84318/#review82946
Attachment #8799024 - Flags: review?(rbarker) → review+
Comment on attachment 8799025 [details]
Bug 1292034 - Update the StackScroller with the elapsed time from the previous fling before starting a new one, so that it doesn't use a stale velocity for flywheel.

https://reviewboard.mozilla.org/r/84320/#review82948
Attachment #8799025 - Flags: review?(rbarker) → review+
Comment on attachment 8799026 [details]
Bug 1292034 - Improve the controls over when flywheel/fling acceleration kicks in.

https://reviewboard.mozilla.org/r/84322/#review82950
Attachment #8799026 - Flags: review?(rbarker) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71445b7c3044
Make the StackScroller flywheel detection code more robust to real user behaviour when flinging. r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/80dc73282950
Update the StackScroller with the elapsed time from the previous fling before starting a new one, so that it doesn't use a stale velocity for flywheel. r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/66bbe7f0d664
Improve the controls over when flywheel/fling acceleration kicks in. r=rbarker
Doesn't every other app with scrolling not have this bug?

I'm not entirely sure how Firefox handles scrolling, but shouldn't you zero the velocity (not just pause the animation) as soon as you touch the page rectangle in any way? (Both when tapping or scrolling)
Most mobile scrolling implementations have a feature where if you scroll multiple times it gets faster and faster. In order to do that you can't just zero out the velocity, you need to keep some record of the previous fling properties. That was the code that was causing the problem here.
"if you scroll multiple times it gets faster and faster" Is this real? I always thought scrolling kept the speed of your most recent scroll only, and due to this bug, Firefox used the faster of first and second scroll.

I always keep animation time at 0.5x. It makes Android feel MUCH more snappy. However, Firefox scroll drifting is twice as fast as the speed before releasing the finger. This makes it harder to compare scrolling from 1 or 2 swipes, and may be why we disagree on this behavior.

Is the above a bug?
Backed out for GTest failing APZScrollHandoffTester.ScrollgrabFlingAcceleration1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b213c7ea6e47731caa7fd400877823cdc2bf2f68
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dcfe360d3fd1fe1a5564b02b3f9a65c65f78611
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d8df6f88f6aaee2f856b52e277bfa068e12973

Push with failures: https://treeherder.mozilla.org/logviewer.html#?job_id=37292122&repo=mozilla-inbound
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37292122&repo=mozilla-inbound

[task 2016-10-07T22:38:07.761493Z] 22:38:07     INFO -  TEST-START | APZScrollHandoffTester.ScrollgrabFlingAcceleration1
[task 2016-10-07T22:38:07.761748Z] 22:38:07  WARNING -  TEST-UNEXPECTED-FAIL | APZScrollHandoffTester.ScrollgrabFlingAcceleration1 | Expected: (childVelocityAfterFling2) > (childVelocityAfterFling1 * kAcceleration / 2), actual: 0.30938 vs 15.469 @ /home/worker/workspace/build/src/gfx/layers/apz/test/gtest/TestScrollHandoff.cpp:121
[task 2016-10-07T22:38:07.762050Z] 22:38:07  WARNING -  TEST-UNEXPECTED-FAIL | APZScrollHandoffTester.ScrollgrabFlingAcceleration1 | test completed (time: 1ms)
[task 2016-10-07T22:38:07.762323Z] 22:38:07     INFO -  TEST-START | APZScrollHandoffTester.ScrollgrabFlingAcceleration2
[task 2016-10-07T22:38:07.762575Z] 22:38:07  WARNING -  TEST-UNEXPECTED-FAIL | APZScrollHandoffTester.ScrollgrabFlingAcceleration2 | Expected: (childVelocityAfterFling2) > (childVelocityAfterFling1 * kAcceleration / 2), actual: 0.30938 vs 15.469 @ /home/worker/workspace/build/src/gfx/layers/apz/test/gtest/TestScrollHandoff.cpp:121
[task 2016-10-07T22:38:07.762784Z] 22:38:07  WARNING -  TEST-UNEXPECTED-FAIL | APZScrollHandoffTester.ScrollgrabFlingAcceleration2 | test completed (time: 1ms)
Flags: needinfo?(bugmail)
(In reply to jimbo1qaz from comment #20)
> "if you scroll multiple times it gets faster and faster" Is this real?

Yes.

> I
> always thought scrolling kept the speed of your most recent scroll only, and
> due to this bug, Firefox used the faster of first and second scroll.

Nope. The bug is that it is combining the two scrolls under conditions that it's not supposed to.

> I always keep animation time at 0.5x. It makes Android feel MUCH more
> snappy. However, Firefox scroll drifting is twice as fast as the speed
> before releasing the finger. This makes it harder to compare scrolling from
> 1 or 2 swipes, and may be why we disagree on this behavior.
> 
> Is the above a bug?

I'm not sure I follow. The android setting for animation duration should not affect scrolling in Firefox. Are you saying that while your finger is down on the screen, the page is moving twice as fast as your finger (i.e. the point under your finger moves out from under your finger)?
Flags: needinfo?(bugmail)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f52a011e8b5
Make the StackScroller flywheel detection code more robust to real user behaviour when flinging. r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/410050c1e8d8
Update the StackScroller with the elapsed time from the previous fling before starting a new one, so that it doesn't use a stale velocity for flywheel. r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/7759e701fea1
Improve the controls over when flywheel/fling acceleration kicks in. r=rbarker
Can you give the latest Nightly for a spin and see if the behaviour is any better? You can download the nightly from nightly.mozilla.org. If you still see the unexpected fling speed behaviour try going to about:config and changing the value of the apz.fling_accel_min_velocity pref - I set it to 1.5 by default but it better at something higher like 2.0 or 2.5. If you find a value that works better let me know. Thanks!
Flags: needinfo?(jimbo1qaz)
(Markus, you too ^)
Flags: needinfo?(mstange)
Fixed.
Flags: needinfo?(jimbo1qaz)
So much better! I finally feel like I can trust my fingers again.

I wouldn't call it completely perfect, though. I still think that any 150ms periods of moving the fingers slower than a minimum speed should cancel flywheel scrolling completely, even if the fingers then speed up towards the end of the pan gesture.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #28)
> So much better! I finally feel like I can trust my fingers again.

Great, I'll try to get this uplifted.

> I wouldn't call it completely perfect, though. I still think that any 150ms
> periods of moving the fingers slower than a minimum speed should cancel
> flywheel scrolling completely, even if the fingers then speed up towards the
> end of the pan gesture.

This shouldn't be too hard to implement. I filed bug 1309021 for it, will get to it eventually...
Comment on attachment 8799024 [details]
Bug 1292034 - Make the StackScroller flywheel detection code more robust to real user behaviour when flinging.

Approval Request Comment
[Feature/regressing bug #]: APZ on Fennec
[User impact if declined]: The flywheel behaviour while scrolling rapidly often doesn't trigger as easily as the user would expect, and sometimes triggers when the user doesn't expect it to.
[Describe test coverage new/current, TreeHerder]: tested on m-c, not a lot of automated coverage
[Risks and why]: pretty low risk, the code is well contained and well understood. Affects Fennec only
[String/UUID change made/needed]: none
Attachment #8799024 - Flags: approval-mozilla-beta?
Attachment #8799024 - Flags: approval-mozilla-aurora?
Attachment #8799025 - Flags: approval-mozilla-beta?
Attachment #8799025 - Flags: approval-mozilla-aurora?
Attachment #8799026 - Flags: approval-mozilla-beta?
Attachment #8799026 - Flags: approval-mozilla-aurora?
Hello there, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(jimbo1qaz)
Comment on attachment 8799024 [details]
Bug 1292034 - Make the StackScroller flywheel detection code more robust to real user behaviour when flinging.

Let's stabilize this on Aurora51 (and will uplift to Beta50 once we have the bug opened verify the fix on Nightly52)
Attachment #8799024 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8799025 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8799026 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The reporter already confirmed in comment 27.
Flags: needinfo?(jimbo1qaz)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33)
> The reporter already confirmed in comment 27.

Oops, my bad!
Status: RESOLVED → VERIFIED
Comment on attachment 8799024 [details]
Bug 1292034 - Make the StackScroller flywheel detection code more robust to real user behaviour when flinging.

Fix was verified on Nightly52, Beta50+
Attachment #8799024 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8799025 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8799026 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on both Aurora(51.0a2 / 2016-10-18) and Beta (50.0b8) on a Samsung Galaxy S6 Edge(Android 6.0) and Sony Xperia Z2 tablet(Android 5.1.1)
You need to log in before you can comment on or make changes to this bug.