Fennec APZ flywheel physics result in very strange behavior when you do sequence of quick and short pans

RESOLVED FIXED in Firefox 48

Status

()

Core
Panning and Zooming
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: mstange, Assigned: kats)

Tracking

(Blocks: 1 bug, {polish, regression})

Trunk
mozilla50
All
Android
polish, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed, firefox50 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8736493 [details]
sketch of the finger motion

Do a series of short pans. Start each pan with a quick motion but decelerate to almost no motion before you lift the finger. At the fifth pan, rest your finger for about 200ms before lifting it.
As you lift the finger, the page will be scrolling away with a rather surprising speed.
(Reporter)

Comment 1

a year ago
Ah, the sketch doesn't show the final resting. It makes no difference in the effect though.
Blocks: 776030
OS: Unspecified → Android
Hardware: Unspecified → All
Anthony, any thoughts on if this needs further adjusting for Fenenc?
Flags: needinfo?(alam)
Keywords: polish
Whiteboard: [gfx-noted]
I haven't really felt this issue myself so I cant really say we need to adjust the current scrolling UX.
Flags: needinfo?(alam)
This may be fix by Bug 1229462 - Use Android Scroller class for fling animation.
(Reporter)

Comment 5

a year ago
This is still happening, but it's less severe than before.
We still use the APZC's velocity calculation when starting the native fling, and my guess is that the problem is with that velocity computation.
I think the main problem here is just that the touch-start distance threshold is too small. We do end up treating that small movement as a fling instead of just a tap, and the fling can get accelerated if it's within the time threshold. We could increase the distance threshold.
Note that the distance threshold was reduced from 0.2222 to 0.06 in bug 1230077. I think we should bump it up to 0.1 which will help somewhat without reintroducing the laggy feeling. snorp, any objections?
Flags: needinfo?(snorp)
Also the fling acceleration interval is 750ms which maybe is too high? It means that more flings get counted as accelerated flings.
(That was set in bug 1229841 based on :antlam's feedback, so I'm less willing to change that though).
(Reporter)

Comment 10

a year ago
750ms seems way too high. Or maybe there should be a finger velocity threshold that cancels flywheel? In my case, the finger is coming to a stop for maybe 200ms, then moves just a little. This behavior should definitely cancel the flywheel.
(In reply to Markus Stange [:mstange] from comment #10)
> Or maybe there should be a finger velocity
> threshold that cancels flywheel?

Yeah actually that seems like a better idea. If the "accelerated" fling is too slow we should just not accelerate. I'll try that out.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=559c4d8c154b
Created attachment 8762750 [details]
Bug 1260905 - Set a minimum bound on the velocity required for a fling to actually happen.

Review commit: https://reviewboard.mozilla.org/r/59242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59242/
Attachment #8762750 - Flags: review?(rbarker)
Assignee: nobody → bugmail.mozilla
status-firefox49: --- → affected
status-firefox50: --- → affected
Flags: needinfo?(snorp)
Keywords: regression
Blocks: 1206872
Comment on attachment 8762750 [details]
Bug 1260905 - Set a minimum bound on the velocity required for a fling to actually happen.

https://reviewboard.mozilla.org/r/59242/#review56268
Attachment #8762750 - Flags: review?(rbarker) → review+
I had to sprinkle some SCOPED_GFX_PREF dust on the gtests to make sure they pass, as some of the simulate flings and need the pref set to 0 to work properly. Verified locally that the gtests pass with those changes.

Comment 16

a year ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05b34b55809a
Set a minimum bound on the velocity required for a fling to actually happen. r=rbarker

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/05b34b55809a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Markus was saying that this patch helped a bit, although didn't fix the issue entirely. I think it's still worth uplifting, but we might want to file a follow-up to fix it harder. I'll leave that to Markus since he can provide more detail on how frequently he still runs into it and if the behaviour has changed any.
Comment on attachment 8762750 [details]
Bug 1260905 - Set a minimum bound on the velocity required for a fling to actually happen.

Approval Request Comment
[Feature/regressing bug #]: APZ on Fennec
[User impact if declined]: Sometimes slow flings, particularly after doing a number of fast flings, can still trigger flywheel scrolling. This can be annoying to the user.
[Describe test coverage new/current, TreeHerder]: The existing code being modified is exercised by gtests, but the new codepaths added are not. I had to update tests to keep them passing.
[Risks and why]: low risk, adds a new pref. if needed we can just modify the pref value to undo this change.
[String/UUID change made/needed]: none
Attachment #8762750 - Flags: approval-mozilla-beta?
Attachment #8762750 - Flags: approval-mozilla-aurora?
Comment on attachment 8762750 [details]
Bug 1260905 - Set a minimum bound on the velocity required for a fling to actually happen.

This patch fixes the regression and polishes the user experience. Take it in 48 beta 4 and aurora.
Attachment #8762750 - Flags: approval-mozilla-beta?
Attachment #8762750 - Flags: approval-mozilla-beta+
Attachment #8762750 - Flags: approval-mozilla-aurora?
Attachment #8762750 - Flags: approval-mozilla-aurora+

Comment 21

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d80b5568e350
status-firefox49: affected → fixed

Comment 22

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d80b5568e350

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/33403379093f
status-firefox48: affected → fixed
(Assignee)

Updated

10 months ago
Blocks: 1292034

Updated

4 months ago
Depends on: 1343775
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> I had to sprinkle some SCOPED_GFX_PREF dust on the gtests to make sure they
> pass, as some of the simulate flings and need the pref set to 0 to work
> properly. Verified locally that the gtests pass with those changes.

The patch in this bug actually regressed overscrolling behaviour (although no platform is currently using that), and adding "SCOPED_GFX_PREF(APZFlingMinVelocityThreshold, float, 0.0f)" to some of these tests (specifically, StuckInOverscroll_Bug1073250 and StuckInOverscroll_Bug1231228) covered that up. (Adding the pref to the other tests, that are actually testing fling behaviour, is fine.) See bug 1343775 for details.
Oh, whoops! Sorry about that - I should have been more diligent when looking into the test failures. I looked at one or two and just assumed it was the same problem in all of them. Thanks for catching this and filing the follow-up.
You need to log in before you can comment on or make changes to this bug.