|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
134.06 KB, image/png
58 bytes, text/x-review-board-request
|Details | Review|
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.
Ah, the sketch doesn't show the final resting. It makes no difference in the effect though.
Anthony, any thoughts on if this needs further adjusting for Fenenc?
I haven't really felt this issue myself so I cant really say we need to adjust the current scrolling UX.
This may be fix by Bug 1229462 - Use Android Scroller class for fling animation.
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?
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).
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.
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/
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
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.
Pushed by email@example.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
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
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.
(In reply to Kartikaya Gupta (email:firstname.lastname@example.org) 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.