Closed Bug 1448439 Opened 7 years ago Closed 7 years ago

Copy Chrome's fling behavior on Android

Categories

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

Product:
Core
Component:
Panning and Zooming
Platform:
Unspecified
Android
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified

People

(Reporter: cpeterson, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Keywords: parity-chrome, Whiteboard: [geckoview:klar:p2][gfx-noted])

Attachments

(9 files)

Bug 1448439 - Allow the concrete type of fling animations to be determined dynamically.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1448439 - Rename AndroidFlingAnimation to StackScrollerFlingAnimation.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1448439 - Move Axis::FlingApplyFrictionOrCancel() to GenericFlingAnimation.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1448439 - Extract a FlingPhysics abstraction from GenericFlingAnimation.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1448439 - Add an AndroidFlingPhysics class containing an implementation of Chrome's fling physics on Android.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1448439 - Add a pref to get APZ to use the Chrome fling physics on Android.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1448439 - Expose some of the tunable parameters in AndroidFlingPhysics as prefs.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1448439 - Fix unified build bustage.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1448439 - Use an accurate pixels per pinch value in AndroidFlingPhysics.
59 bytes, text/x-review-board-request
kats
: review+
Details
Fennec/GeckoView's current scrolling behavior has its roots in old Firefox OS code and does not copy Android's native scrolling behavior or Chrome's. Fennec's current scrolling is unpopular, but when Fennec experimented with using Android's native scrolling behavior in the past, users didn't like it either. We should copy Chrome's scrolling since it is the de facto standard for web content on Android. Here is Chrome's scrolling physics code for Android: https://cs.chromium.org/chromium/src/ui/events/android/scroller.cc
(In reply to Chris Peterson [:cpeterson] from comment #0) > and does not copy Android's native scrolling behavior [...] > when Fennec experimented with using Android's native scrolling behavior in the past Er, we actually use Android's scroll physics since bug 1229462 and in fact Chrome's StackScroller since bug 1280666, don't we?
Priority: -- → P3
Whiteboard: [geckoview:klar] → [geckoview:klar][gfx-noted]
It's not exactly the same. I just tested by opening a long page on Chrome. With a single aggressive swipe, I can scroll from top to bottom or from bottom to top. With Firefox (Nightly), it takes 2-3 swipes. Personally, I have no problem with Firefox's scrolling behavior, but it's a very common complaint from newcomers who perceive it as a performance problem when they are accustomed to Chrome's physics.
Firefox's scrolling feels sticky on Android. The heaviness makes it feel slow. Compared to Focus or others, it can take four times as many swings to scroll through the page. When the user scrolls the page while reading an article, they don't swing and release the finger. They keep the finger on the screen and slowly swipe it upwards. The quick swing-and-release is done in order to travel long distances. This is where the weakness is. It's very common to see people complain about this on internet forums. A person recommends Firefox, few others comment about how "slow the scrolling is". When I recommend them to change the fling values as described here https://www.reddit.com/r/firefox/comments/670qny/how_to_change_firefox_scrolling_speed_on_android/ Many of them report having a better experience. Others report janky scroll, which is outside of the scope of this bug.
Botond says he will be working on this bug, building upon his prototype from bug 1448376. :)
Assignee: nobody → botond
If you fixed bug 1401526 at the same time I think that would help improve the perception of scrolling. Bug 1401526 causes a big scroll jank during the fling as the toolbar hides. Chromesque fling physics + no jank on fling = a lot of happy users???
I'm going to narrow the scope of this bug to deal with fling physics in particular, as that is what the investigation in bug 1448376 concerned. There may be other aspects of "scroll behaviour" that differ between us and Chrome, which would need to be investigated separately. I've started implementation work on this. Will provide an update on Friday.
Summary: Copy Chrome's scrolling behavior on Android → Copy Chrome's fling behavior on Android
Botond, how risky is this scrolling change? We only have two weeks left in the Nightly 61 dev cycle. Do you want to wait to land this change in Nightly 62 if it might cause new scrolling bugs that would need to be fixed in Beta 61?
Flags: needinfo?(botond)
I am developing the implementation of the Chrome fling physics behind a pref, with the old physics code still there for now. I was hoping someone from the UX team could try the new physics, and perhaps adjust the tuneable parameters, prior to enabling it. I would be fine with enabling the pref in the 61 cycle (or uplifting a patch to enable the pref to 61). I don't expect much regression risk because I plan to stick to the old code as much as possible in terms of handling edge cases, and only swap out the calculations. However, if for some reason there do end up being more regressions than we want to fix in Beta 61, we can always just disable the pref again.
Flags: needinfo?(botond)
This is a first draft of the implementation. It needs testing and is not ready for review yet.
I've been testing the patches locally, and after fixing some minor bugs in the implementation, the results look promising (that is, flings are noticeably faster). I think we're ready to land the implementation behind a pref. Will post a revised patch series for review soon.
Comment on attachment 8969823 [details] Bug 1448439 - Allow the concrete type of fling animations to be determined dynamically. https://reviewboard.mozilla.org/r/238652/#review245360
Attachment #8969823 - Flags: review?(bugmail) → review+
Comment on attachment 8969851 [details] Bug 1448439 - Fix unified build bustage. https://reviewboard.mozilla.org/r/238696/#review245362
Attachment #8969851 - Flags: review?(bugmail) → review+
Comment on attachment 8969824 [details] Bug 1448439 - Rename AndroidFlingAnimation to StackScrollerFlingAnimation. https://reviewboard.mozilla.org/r/238654/#review245364 ::: gfx/layers/apz/src/AndroidAPZ.h:34 (Diff revision 3) > - AndroidFlingAnimation(AsyncPanZoomController& aApzc, > + StackScrollerFlingAnimation(AsyncPanZoomController& aApzc, > PlatformSpecificStateBase* aPlatformSpecificState, > const RefPtr<const OverscrollHandoffChain>& aOverscrollHandoffChain, > bool aFlingIsHandoff /* ignored */, > const RefPtr<const AsyncPanZoomController>& aScrolledApzc); nit: fix indent
Attachment #8969824 - Flags: review?(bugmail) → review+
Comment on attachment 8969825 [details] Bug 1448439 - Move Axis::FlingApplyFrictionOrCancel() to GenericFlingAnimation. https://reviewboard.mozilla.org/r/238656/#review245366
Attachment #8969825 - Flags: review?(bugmail) → review+
Comment on attachment 8969826 [details] Bug 1448439 - Extract a FlingPhysics abstraction from GenericFlingAnimation. https://reviewboard.mozilla.org/r/238658/#review245368 ::: gfx/layers/apz/src/GenericFlingAnimation.h:124 (Diff revision 4) > virtual bool DoSample(FrameMetrics& aFrameMetrics, > const TimeDuration& aDelta) override > { > - float friction = gfxPrefs::APZFlingFriction(); > - float threshold = gfxPrefs::APZFlingStoppedThreshold(); > - > + ParentLayerPoint velocity; > + ParentLayerPoint offset; > + FlingPhysics::Sample(aDelta, &velocity, &offset); Not clear to me why you chose to make offset a returned thing from FlingPhysics::Sample instead of leaving it in this function (since you have both velocity and aDelta and can easily compute it here). But presumably it will be useful in a future patch. Leaving this comment so I don't forget.
Attachment #8969826 - Flags: review?(bugmail) → review+
> Not clear to me why you chose to make offset a returned thing from > FlingPhysics::Sample instead of leaving it in this function (since you have > both velocity and aDelta and can easily compute it here). But presumably it > will be useful in a future patch. Leaving this comment so I don't forget. Ah, I see - it's needed in the next patch.
Comment on attachment 8969828 [details] Bug 1448439 - Add a pref to get APZ to use the Chrome fling physics on Android. https://reviewboard.mozilla.org/r/238662/#review245376
Attachment #8969828 - Flags: review?(bugmail) → review+
Comment on attachment 8969829 [details] Bug 1448439 - Expose some of the tunable parameters in AndroidFlingPhysics as prefs. https://reviewboard.mozilla.org/r/238664/#review245378 ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:145 (Diff revision 4) > + * \li\b apz.android.chrome_fling_physics.friction > + * A tunable parameter for Chrome fling physics on Android that governs > + * how quickly a fling animation slows down due to friction (and therefore > + * also how far it reaches). > + * > + * \li\b apz.android.chrome_fling_physics.inflexion > + * A tunable parameter for Chrome fling physics on Android that governs > + * the shape of the fling curve. It might be good to mention valid ranges for these two prefs. I assume friction should be in the range 0-1 but not sure about inflexion. In some places it seems like we use 1.0 - inflexion which implies that it should also be less than 1?
Attachment #8969829 - Flags: review?(bugmail) → review+
Comment on attachment 8969827 [details] Bug 1448439 - Add an AndroidFlingPhysics class containing an implementation of Chrome's fling physics on Android. https://reviewboard.mozilla.org/r/238660/#review245370 I don't fully understand the physics here but presumably it works. Have a comment about the loop below though that I think we should address. Thanks for the nicely-split patch series! ::: gfx/layers/apz/src/AndroidFlingPhysics.h:13 (Diff revision 4) > +#define mozilla_layers_AndroidFlingPhysics_h_ > + > +#include "AsyncPanZoomController.h" > +#include "Units.h" > +#include "gfxPrefs.h" > +#include "mozilla/Assertions.h" Alpha sort ::: gfx/layers/apz/src/AndroidFlingPhysics.cpp:26 (Diff revision 4) > +static double ComputeDeceleration(float aFriction) > +{ > + const float kGravityEarth = 9.80665f; > + return kGravityEarth // g (m/s^2) > + * 39.37f // inch/meter > + * 160.f // pixels/inch Do we want to use the GetDPI function here instead? Might be a little hard since I made it non-static; it might not be worth it. ::: gfx/layers/apz/src/AndroidFlingPhysics.cpp:79 (Diff revision 4) > + for (int i = 0; i < kNumSamples; i++) { > + const float alpha = static_cast<float>(i) / kNumSamples; > + > + float xMax = 1.0f; > + float x, tx, coef; > + while (true) { This scares me a little. It looks like it's doing some sort of binary search on x to find the point at which tx == alpha, where tx is a f(x). This sort of binary search probably has an assumption about the properties of f(x) (e.g. monotonicity), which might not hold for certain values of inflexion. So it might be possible for a user to fiddle with their inflexion pref and trigger an infinite loop here. It would be good to guard against this somehow, either by adding an upper bound on the number of loop iterations or ensuring that tx is getting closer to alpha with every iteration. ::: gfx/layers/apz/src/AndroidFlingPhysics.cpp:83 (Diff revision 4) > + if (FuzzyEqualsAdditive(tx, alpha)) > + break; > + if (tx > alpha) > + xMax = x; > + else > + xMin = x; braces
Attachment #8969827 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #46) > ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:145 > (Diff revision 4) > > + * \li\b apz.android.chrome_fling_physics.friction > > + * A tunable parameter for Chrome fling physics on Android that governs > > + * how quickly a fling animation slows down due to friction (and therefore > > + * also how far it reaches). > > + * > > + * \li\b apz.android.chrome_fling_physics.inflexion > > + * A tunable parameter for Chrome fling physics on Android that governs > > + * the shape of the fling curve. > > It might be good to mention valid ranges for these two prefs. Good point, will do. > I assume friction should be in the range 0-1 I don't think there's anything that mathematically requires it to be less than 1 given how it's used in the equations. However, seeing as the default value is 0.015, staying within the 0-1 range is probably a good guideline. > but not sure about inflexion. In some > places it seems like we use 1.0 - inflexion which implies that it should > also be less than 1? Yeah, I believe inflexion should be in the [0,1] range. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #47) > I don't fully understand the physics here but presumably it works. Yeah, there's definitely a good amount of cargo-cult copying from the Chrome code happening here. In my defence, the bug's specification is "copy Chrome's fling behavior", and there's only so much time I want to spend reverse engineering mathematical models from lightly commented Chrome source code. I provided a permalink to the file from which I copied the calculations, so that someone who's really interested can do archaeolgy. > ::: gfx/layers/apz/src/AndroidFlingPhysics.h:13 > (Diff revision 4) > > +#define mozilla_layers_AndroidFlingPhysics_h_ > > + > > +#include "AsyncPanZoomController.h" > > +#include "Units.h" > > +#include "gfxPrefs.h" > > +#include "mozilla/Assertions.h" > > Alpha sort As discussed on IRC, we'll stick to case-sensitive sorting (which this is) for new code, as that is clang-format's default behaviour. > ::: gfx/layers/apz/src/AndroidFlingPhysics.cpp:26 > (Diff revision 4) > > +static double ComputeDeceleration(float aFriction) > > +{ > > + const float kGravityEarth = 9.80665f; > > + return kGravityEarth // g (m/s^2) > > + * 39.37f // inch/meter > > + * 160.f // pixels/inch > > Do we want to use the GetDPI function here instead? Might be a little hard > since I made it non-static; it might not be worth it. Good catch! I also realized that if we intend this to be an accurate representation of pixels per screen inch, we also need to factor in the conversion between ParentLayer and Screen pixels. I'll fix this in a new patch (in this bug) to avoid too much rebasing. > ::: gfx/layers/apz/src/AndroidFlingPhysics.cpp:79 > (Diff revision 4) > > + for (int i = 0; i < kNumSamples; i++) { > > + const float alpha = static_cast<float>(i) / kNumSamples; > > + > > + float xMax = 1.0f; > > + float x, tx, coef; > > + while (true) { > > This scares me a little. It looks like it's doing some sort of binary search > on x to find the point at which tx == alpha, where tx is a f(x). This sort > of binary search probably has an assumption about the properties of f(x) > (e.g. monotonicity), which might not hold for certain values of inflexion. > So it might be possible for a user to fiddle with their inflexion pref and > trigger an infinite loop here. It would be good to guard against this > somehow, either by adding an upper bound on the number of loop iterations or > ensuring that tx is getting closer to alpha with every iteration. Good catch here as well! I ran some experiments and it looks like the approximation indeed diverges for negative values of inflexion. That would be a lame way for Firefox to get into an infinite loop :) For positive values of inflexion, the approximation converges in < 20 iterations for values in the range [0,1], and < 60 iterations for even outrageously large values; I'll add an iteration limit of 100.
(In reply to Botond Ballo [:botond] from comment #48) > Good catch here as well! I ran some experiments and it looks like the > approximation indeed diverges for negative values of inflexion. That would > be a lame way for Firefox to get into an infinite loop :) > > For positive values of inflexion, the approximation converges in < 20 > iterations for values in the range [0,1], and < 60 iterations for even > outrageously large values; I'll add an iteration limit of 100. I ended up clamping the inflexion value to [0,1], since values outside that range don't really make sense and can produce weird effects like the fling jumping right to its destination. I still kept the iteration limit for good measure.
Comment on attachment 8971381 [details] Bug 1448439 - Use an accurate pixels per pinch value in AndroidFlingPhysics. https://reviewboard.mozilla.org/r/240134/#review245968
Attachment #8971381 - Flags: review?(bugmail) → review+
[geckoview:klar:p2] because we really want this fix for Klar+GeckoView, but it is not a blocker.
status-firefox-esr60: --- → wontfix
Whiteboard: [geckoview:klar][gfx-noted] → [geckoview:klar:p2][gfx-noted]
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/79bbfba09e52 Fix unified build bustage. r=kats https://hg.mozilla.org/integration/autoland/rev/aa70fd0d1b3c Allow the concrete type of fling animations to be determined dynamically. r=kats https://hg.mozilla.org/integration/autoland/rev/4c88aea1d5d0 Rename AndroidFlingAnimation to StackScrollerFlingAnimation. r=kats https://hg.mozilla.org/integration/autoland/rev/11540b54a580 Move Axis::FlingApplyFrictionOrCancel() to GenericFlingAnimation. r=kats https://hg.mozilla.org/integration/autoland/rev/da26dcf12417 Extract a FlingPhysics abstraction from GenericFlingAnimation. r=kats https://hg.mozilla.org/integration/autoland/rev/f6f331881678 Add an AndroidFlingPhysics class containing an implementation of Chrome's fling physics on Android. r=kats https://hg.mozilla.org/integration/autoland/rev/9ee2b45eeb3a Add a pref to get APZ to use the Chrome fling physics on Android. r=kats https://hg.mozilla.org/integration/autoland/rev/68f4c6d26043 Expose some of the tunable parameters in AndroidFlingPhysics as prefs. r=kats https://hg.mozilla.org/integration/autoland/rev/8c7c5f346006 Use an accurate pixels per pinch value in AndroidFlingPhysics. r=kats
Re Comment 6, the new Chromesque scrolling seems to have fixed bug 1401526. I don't understand how. So 2 for the price of 1, huzzah! I like the new scrolling a lot, will see if I can break it.
(In reply to Mark from comment #65) > Re Comment 6, the new Chromesque scrolling seems to have fixed bug 1401526. > I don't understand how. So 2 for the price of 1, huzzah! I like the new > scrolling a lot, will see if I can break it. Thanks for verifying the fix, Mark. Interesting to hear that this change also fixed bug 1401526! I'll close that bug. Feel free to reopen it if you can still reproduce the content jumping problem.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
Blocks: 1401526
This feels a lot better than before, but it still feels different from Chrome. I've filed bug 1457586 about the difference I'm seeing.
Depends on: 1457586
I like it a lot, major step forward. The fix of bug 1401526 seems real as well, FF Android feels much classier now. Thanks
Depends on: 1458653
Blocks: android-scrolling
No longer blocks: 1425739
No longer blocks: 1230176
See Also: → 1756275
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: