Closed
Bug 1292034
Opened 8 years ago
Closed 8 years ago
Scrolling quickly, then slowly, moves too fast (vertical speed conservation)
Categories
(Firefox for Android Graveyard :: Toolbar, defect, P2)
Tracking
(firefox48 wontfix, firefox49 wontfix, firefox50 verified, firefox51 verified, firefox52 verified)
VERIFIED
FIXED
Firefox 52
People
(Reporter: nyanpasu64, Assigned: kats)
References
Details
Attachments
(3 files)
(deleted),
text/x-review-board-request
|
rbarker
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
rbarker
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
rbarker
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
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.
Reporter | ||
Comment 1•8 years ago
|
||
(yes, I named the title after https://www.youtube.com/watch?v=6md3RA8bH40 )
Comment 2•8 years ago
|
||
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
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
Component: General → Graphics, Panning and Zooming
Ever confirmed: true
Assignee | ||
Comment 3•8 years ago
|
||
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
Comment 4•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bugmail
Assignee | ||
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
(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 12•8 years ago
|
||
mozreview-review |
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 13•8 years ago
|
||
mozreview-review |
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 14•8 years ago
|
||
mozreview-review |
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 15•8 years ago
|
||
mozreview-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 16•8 years ago
|
||
mozreview-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+
Comment 17•8 years ago
|
||
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
Reporter | ||
Comment 18•8 years ago
|
||
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)
Assignee | ||
Comment 19•8 years ago
|
||
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.
Reporter | ||
Comment 20•8 years ago
|
||
"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?
Comment 21•8 years ago
|
||
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)
Assignee | ||
Comment 22•8 years ago
|
||
(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)
Comment 23•8 years ago
|
||
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
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f52a011e8b5
https://hg.mozilla.org/mozilla-central/rev/410050c1e8d8
https://hg.mozilla.org/mozilla-central/rev/7759e701fea1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Assignee | ||
Comment 25•8 years ago
|
||
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)
Comment 28•8 years ago
|
||
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)
Assignee | ||
Comment 29•8 years ago
|
||
(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...
Assignee | ||
Comment 30•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Attachment #8799025 -
Flags: approval-mozilla-beta?
Attachment #8799025 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
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+
Assignee | ||
Comment 33•8 years ago
|
||
The reporter already confirmed in comment 27.
Flags: needinfo?(jimbo1qaz)
Comment 34•8 years ago
|
||
(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+
Comment 37•8 years ago
|
||
bugherder uplift |
Comment 38•8 years ago
|
||
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)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•