Closed Bug 1458653 Opened 3 years ago Closed 20 days ago

Two short flings can be faster on Fennec than Chrome

Categories

(Core :: Panning and Zooming, defect, P1)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
84 Branch
Tracking Status
relnote-firefox --- ?
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox67 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox84 --- fixed

People

(Reporter: kbrosnan, Assigned: mstange)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

(Whiteboard: [gfx-noted] [geckoview:p3] [qf-][fenix:p1])

Attachments

(5 files, 10 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Using the new fling physics. I see some flings that are faster than expected. Especially short flings when reading articles. I believe that the issue is that we have some small amount of velocity left when flinging again.
I'm going to mark this as dependent on bug 1457586, because it requires better understanding how Chrome determines the initial fling velocity.
Depends on: 1457586
Priority: -- → P3
Whiteboard: [gfx-noted]
Summary: Two short flings can be faster on Firefox than Chrome → Two short flings can be faster on Fennec than Chrome
Whiteboard: [gfx-noted] → [gfx-noted][geckoview:klar]
Whiteboard: [gfx-noted][geckoview:klar] → [gfx-noted][geckoview:klar:p3]
glamon (UX) says he was not able to reproduce this fling issue in Fennec 61.
Whiteboard: [gfx-noted][geckoview:klar:p3] → [gfx-noted][geckoview:p3]

This is likely to be related to the "fling acceleration" feature that was added in bug 907123.

The behaviour of this can be configured using a few prefs:

 * \li\b apz.fling_accel_interval_ms
 * The time that determines whether a second fling will be treated as
 * accelerated. If two flings are started within this interval, the second one
 * will be accelerated. Setting an interval of 0 means that acceleration will
 * be disabled.\n
 * Units: milliseconds
 *
 * \li\b apz.fling_accel_min_velocity
 * The minimum velocity of the second fling for it to be considered for fling
 * acceleration.
 * Units: screen pixels per milliseconds
 *
 * \li\b apz.fling_accel_base_mult
 * \li\b apz.fling_accel_supplemental_mult
 * When applying an acceleration on a fling, the new computed velocity is
 * (new_fling_velocity * base_mult) + (old_velocity * supplemental_mult).
 * The base_mult and supplemental_mult multiplier values are controlled by
 * these prefs. Note that "old_velocity" here is the initial velocity of the
 * previous fling _after_ acceleration was applied to it (if applicable).

Possibly we could pick better defaults for these prefs. (UX input on this would be helpful.) We could also take inspiration from how Chrome handles fling acceleration (some brief testing suggests it does it too, but possibly with different parameters).

Vesta, what do you recommend we do for these "web platform UX" issues? I'm not sure whether the responsibility for making decisions about scrolling behavior (and things like default font sizes) belongs to a Fenix UX designer or the Gecko platform engineers.

Related Fenix bug ("Scrolling physics is way too fast on small swipes"): https://github.com/mozilla-mobile/fenix/issues/3744

Depends on: 1590901

I am tagging Tif (Fenix UX lead) and Mike Comella (Fenix front-end performance lead) here for their feedback.

Flags: needinfo?(vzare) → needinfo?(tshakespeare)
Flags: needinfo?(michael.l.comella)

Adding to Performance triage: this issue affects Fenix and falls under one of the four core areas we're concerned about: responsiveness.

Flags: needinfo?(michael.l.comella)
Whiteboard: [gfx-noted][geckoview:p3] → [gfx-noted][geckoview:p3][qf]

(In reply to Botond Ballo [:botond] from comment #5)

 * \li\b apz.fling_accel_base_mult
 * \li\b apz.fling_accel_supplemental_mult
 * When applying an acceleration on a fling, the new computed velocity is
 * (new_fling_velocity * base_mult) + (old_velocity * supplemental_mult).
 * The base_mult and supplemental_mult multiplier values are controlled by
 * these prefs. Note that "old_velocity" here is the initial velocity of the
 * previous fling _after_ acceleration was applied to it (if applicable).

Possibly we could pick better defaults for these prefs.

I don't think we can achieve more gradual acceleration just by tweaking prefs. I think we actually need to change the code.

Explanation:
The current values of both base_mult and supplemental_mult are 1.0. That means
fling_velocity = 1.0 * finger_velocity + 1.0 * previous_fling_velocity_including_acceleration.
If you do a series of flings, all with the same finger velocity (call it v), you get the following fling velocities:
1v, 2v, 3v, 4v, 5v, ...
So every fling adds 1 to the multiplier.
I think what we'd like to have instead is something like this:
1v, 1.5v, 2.25v, 3.375v, ...
I.e. some kind of multiplier that gets multiplied on every subsequent fling, but doesn't kick in with 2x immediately.
However, this behavior cannot be achieved by tweaking the two prefs. And the math for this is somewhat interesting:
First, let's see what happens if we reduce base_mult to 0.8, and look at the case from above:
0.8v, 1.6v, 2.4v, ...
We've slowed down the initial fling and maintained the same growth behavior.
So we can't reduce base_mult. What happens if we reduce supplemental_mult instead, say to 0.5?
1v, 1.5v, 1.75v, 1.875v, 1.9375v, 1.96875v, ...
We now have a sequence that converges to 2v. That's also not what we want! We want the user to feel like they're speeding up the fling with every subsequent fling motion. But with a convergent sequence, the user will feel their efforts have diminishing effects. When a user flings for 4 or 5 times, what they really want to do, in most cases, is to get to the end of the page. With a convergent multiplier, getting to the end of the long page will still take too long.
In fact, the converging behavior happens for every value of supplemental_mult that's less than one. We're dealing with a geometric series here, it converges to base_mult * v / (1 - supplemental_mult).
(Examples: (1.0, 0.5) -> 2v, (1.0, 0.6) -> 2.5v, (1.0, 0.7) -> 3.3v, , (1.0, 0.8) -> 5v)

I haven't looked at Chrome's code yet. But I have spent a few minutes playing with it. Chrome seems to be basing the acceleration on the current velocity of the page, and not on the last finger motion. If you initiate a very long fling, as long as the page is still moving, it will accelerate your fling even if more than a second passed between the two flings.

(I'm setting this bug's qf classification to qf-. It's not a performance issue, it's a UX issue.)

Whiteboard: [gfx-noted][geckoview:p3][qf] → [gfx-noted][geckoview:p3][qf-]

Snorp, as a UX issue that particularly affects GV users, how can we get this triaged?

fwiw, this is in the "Fenix: GeckoView" board in the "Top 10" column so it seems important for Fenix.

Flags: needinfo?(snorp)

(In reply to Michael Comella (:mcomella) [needinfo or I won't see it] from comment #11)

Snorp, as a UX issue that particularly affects GV users, how can we get this triaged?

I will reset this bug's [geckoview] whiteboard tag so the GV team will discuss it our our next GV triage meeting. I've also shared this issue with the Fenix UX team.

Flags: needinfo?(snorp)
Whiteboard: [gfx-noted][geckoview:p3][qf-] → [gfx-noted] [geckoview] [qf-]

Kevin confirmed this bug is still a problem. James says he can reproduce this bug pretty easily, though Agi says he can also reproduce on Chrome.

Whiteboard: [gfx-noted] [geckoview] [qf-] → [gfx-noted] [geckoview:p3] [qf-]
Priority: P3 → P1

Emily - just looking for clarification: what does a top Fenix priority mean? Is the idea that this should be done for 75?

Flags: needinfo?(etoop)
Flags: needinfo?(tshakespeare) → needinfo?(bram)

Top Priority means that it is the most important Gecko bug that is currently not being worked on already. If you are working on other Fenix issues, those should be fixed first, but this is the next issue in line for them. If it can be done for 75, that would be great, but as soon as possible after is OK if other commitments are competing.

Flags: needinfo?(etoop)

Markus, I believe you had ideas for a fix for this?

Flags: needinfo?(mstange)
Blocks: 1612927

I'm going to give this one a try.
I think I want the speedup to be dependent on the existing velocity at the time of touchdown, and not just dependent on the number of consecutive flings and the velocity at the time of touchup.

Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)

mstange, let me know once you have an APK for us to test.

If related: esmyth, amylee and I confirmed last week that the scrolling behaviour addressed on the PR (https://github.com/mozilla-mobile/fenix/pull/7878) is similar enough to other Android apps, that we thought that it’s all good.

Flags: needinfo?(bram)
Whiteboard: [gfx-noted] [geckoview:p3] [qf-] → [gfx-noted] [geckoview:p3] [qf-][fenix:p1]
Blocks: 1626646

Is there an update on the progress of this issue? Fenix would like to see this fixed in Q2.

@Markus: Are you working on this or would you need to hand this over to someone as this is a priority for Fenix?

Flags: needinfo?(mstange)

I'm still planning to work on this. I have studied the Chrome code a bit and I think we can get Chrome-like behavior just by tweaking a few numbers, after all.

Flags: needinfo?(mstange)

@Amoya: After getting user feedback with the initial Fenix releases, is this still a priority for Fenix?

Flags: needinfo?(amoya)

@ktaeleman Just checked with the Fenix team and yes, this is still a priority.

Flags: needinfo?(amoya)

I've compared our code and Chrome's code for fling acceleration (called "fling boosting" in Chrome).

Here's how the behaviors differ:

  • In Firefox, when the finger is lifted and a fling is initiated from the current scroll gesture, the following criteria determine whether fling boosting should be applied:
    • Did less than 750ms elapse between the last fling and this fling?
    • Is this fling pointed in the same direction as the last fling?
  • In Chrome, the criteria are very different. Chrome does not take into account the time that has elapsed between the two flings. Instead, the criteria are:
    • Was the velocity of the previous ongoing fling, at the point of interruption, higher than a certain threshold?
    • Is the new fling starting velocity also higher than that threshold?
    • Did the velocity of the current scroll gesture never fall below a certain (different) threshold?
    • Did no more than 50ms elapse between any two subsequent touch events during the current scroll gesture?
    • Is this fling pointed in the same direction as the last fling?
    • (A few others: same source device, no modifier keys pressed)

I will try to implement something that's closer to the Chrome model. In particular, I think it's very important to take into account the fling velocity at the time the previous fling was interrupted.

It's great to hear this is coming to an attention being worked on.
For those who experience this, you may try adjusting "apz.fling_accel_interval_ms" to about 250ms as a workaround for now.
Just like how Markus elaborated on current implementation, the two short flings will not trigger abrupt page scroll unless they are within 250ms from each other.
As of now, this seem to be a good balance between "wanting to scoll quickly" vs "casual scrolling".
But really, can't wait for a different implementation is adopted.
Looking forward to this.

Would your scrolling improvements also apply to Firefox for desktop?

I think they may affect touch screen scrolling on desktop. They will not affect touch pad or mouse wheel scrolling.

(In reply to bndki34 from comment #26)

For those who experience this, you may try adjusting "apz.fling_accel_interval_ms" to about 250ms as a workaround for now.

I agree, this is a good workaround, until the new behavior is implemented.

(In reply to Poopooracoocoo from comment #27)

Would your scrolling improvements also apply to Firefox for desktop?
Yes, theoretically it should have the same effect on desktop Firefox as well.
I did test with my laptop's touchscreen, and in practice, it's nowhere close to a satisfying scrolling experience.
It takes a lot more swiping gestures to scroll the same amount as that of a mobile browser.
And I believe it has to do with other values, and I have yet to mess around with those.

(In reply to Markus Stange [:mstange] from comment #28)

I agree, this is a good workaround, until the new behavior is implemented.
Sadly for mobiles, about:config is not available in stable version (classic) of Firefox and only accessible in beta or nightly versions.
With no foreseeable plan on allowing access to it in the future, a new implementation would be the only way to fix it on the stable version.
I really appreciate you working on this.

This should prevent most cases of unintended fling acceleration. For example,
when scrolling paragraph-by-paragraph via a series of short flings, we would
often accelerate the next fling even if the previous fling had already slowed
down almost to a stop. With this change, we will no longer trigger acceleration
in that scenario.

This also matches Chrome's behavior.

This patch does not add a new pref for the new velocity threshold.
Chrome also shares the same velocity threshold for both velocities.

Depends on D92138

This avoids unexpected acceleration after a "fling, fling, touch-pause-fling" sequence.

Depends on D92139

This code is unused by default because apz.android.chrome_fling_physics.enabled
defaults to true. It should probably be removed.

The comparison velocity.Length() >= StaticPrefs::apz_fling_accel_interval_ms()
was accidentally broken in bug 1560837 and is now comparing a vector length to
a value in milliseconds. This has effectively turned it into an if (false)
because velocities higher than 750 pixels per millisecond are very hard to achieve.
Until this code gets removed, this patch just makes a simplification to it
which preserves the effective (accidental) behavior, so that future patches can
change prefs as needed and don't need to maintain this unused code path.

Depends on D92140

The other fling acceleration restrictions make it so that there is no need for
this additional restriction anymore.
Removing this restriction allows a fling to be accelerated even if the previous
fling has been "spinning" for a while (a second or more), provided that the page
is still moving fast enough.
This matches Chrome's behavior.

Depends on D92141

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

With these patches, the fling behavior feels good to me. It also seems to match Chrome's behavior rather closely, from what I can tell.
I'll link to the GeckoView-example.apk builds from the try push once they are available.

As a follow-up, I'll try to write some tests, but I'm not sure how well that's going to work.

GeckoView-example.apk builds: 32 bit, 64 bit

Bram, would you like to test? What you should be seeing is the following:

  • The following sequences should no longer cause fling acceleration:
    • short fling, brief pause, short fling
    • fling, fling, touch-pause-fling
    • fling down, fling down, scroll-up-down-fling
  • But it should still be possible to trigger fling acceleration when desired, with a sequence of fast flings. Fling acceleration should not be harder to trigger than in Chrome.

I've found that it is sometimes hard to trigger fling acceleration with these patches even when fling acceleration is desired. When I run into this, it's usually because the finger starts too slow and then accelerates during the touch scroll. It's a lot easier if the finger accelerates just before it touches the screen, so that it's already in motion when the touch starts.
This behavior is, of course, caused by the minimum touch velocity restriction that these patches add. But I think that that's a good restriction and I think the new behavior feels fine.

Flags: needinfo?(bram)

(sorry, i know i'm just a random user and not who you wanted to test) Wow! That is so nice for just an example browser. You can even swipe down on the menu button to instantly select a menu as it's a native Android menu. The address bar is similarly nice to use as its a native text box. Anyway, great job! The scrolling is so much nicer!!

I'll share my two cents as well. Overall, it's a great improvement in terms of casual scrolling experience.

  • The following sequences should no longer cause fling acceleration:
    • short fling, brief pause, short fling
    • fling, fling, touch-pause-fling
    • fling down, fling down, scroll-up-down-fling

In most cases, these are true. I do notice the fling acceleration is no longer dependent on each flings, only dependent on the speed of the a single fling. While this improvement closely resembles that of Chrome, I have to say there's something a bit different from it.

I've found that it is sometimes hard to trigger fling acceleration with these patches even when fling acceleration is desired. When I run into this, it's usually because the finger starts too slow and then accelerates during the touch scroll. It's a lot easier if the finger accelerates just before it touches the screen, so that it's already in motion when the touch starts.

And I agree with this point. Because now that flings are no longer dependent on each other (at least from my experience), it's harder or no longer possible to build up acceleration by "flicking in quicker succession." Now, we need to actually flick the screen with a faster motion in each gesture. In terms of practicality, this is far from expected experience. As you already mentioned, we have a natural tendency to go "1. touch the screen" then "2. perform fling gesture" (which makes it difficult to register fast initial speed), rather than "1. perform fling gesture" and "2. touch the screen in the meanwhile."

Otherwise, the fix is a good step in the right direction, I can finally casually scroll without abrupt acceleration. Thanks and great work.

Depends on: 1669564

Comment on attachment 9179110 [details]
Bug 1458653 - Add a missing #include. r=kats

Revision D92137 was moved to bug 1669564. Setting attachment 9179110 [details] to obsolete.

Attachment #9179110 - Attachment is obsolete: true

Comment on attachment 9179111 [details]
Bug 1458653 - Move fling acceleration logic into a new FlingAccelerator class. No functional changes. r=kats

Revision D92138 was moved to bug 1669564. Setting attachment 9179111 [details] to obsolete.

Attachment #9179111 - Attachment is obsolete: true

Comment on attachment 9179114 [details]
Bug 1458653 - Simplify an unused code path. r=kats

Revision D92141 was moved to bug 1669564. Setting attachment 9179114 [details] to obsolete.

Attachment #9179114 - Attachment is obsolete: true

Hi Markus, I just had a chance to test this behaviour.

I agree that your fix has solved our “abrupt acceleration on 2x flings” problem specifically, and our “low scroll inertia” problem in general.

While higher inertia means that it’s harder to reach a dramatically faster scroll speed, it has made us a better Android citizen (this was my initial concern: that small behaviours like this all combine to make Firefox feel non-native).

Our scrolling style is now comparable not just to Chrome, but also to other native Android apps. Example: Settings → System → Advanced → Developer options.

I’d give this fix an r+. Thanks heaps for working on this!

Flags: needinfo?(bram)

Thanks for the review, Bram!

I'm planning to make a small change to address this issue:

(In reply to bndki34 from comment #40)

it's harder or no longer possible to build up acceleration by "flicking in quicker succession." Now, we need to actually flick the screen with a faster motion in each gesture. In terms of practicality, this is far from expected experience. As you already mentioned, we have a natural tendency to go "1. touch the screen" then "2. perform fling gesture" (which makes it difficult to register fast initial speed), rather than "1. perform fling gesture" and "2. touch the screen in the meanwhile."

I read the Chrome code some more and realized that they weren't applying the "minimum touch velocity" restriction the same way I was - they weren't applying it while the finger was in the "touch slop" radius, i.e. between touch start and the touch move that crosses the touch slop threshold. The way they catch pauses inside the touch slop radius is with a restriction on the time that's allowed to pass between touch start and scroll start (50ms). So I'm just going to do the same.

Furthermore, I need to address a bug with nested scroll frames ("handoff" and "scrollgrab"): The current patches assume that the APZC that receives the touch events is the same APZC as the one that runs the fling scroll animation, but this isn't necessarily the case.
And I still need to write tests.

Blocks: 1309021
Attachment #9179112 - Attachment is obsolete: true
Attachment #9179113 - Attachment is obsolete: true
Attachment #9179115 - Attachment is obsolete: true
Attachment #9179128 - Attachment is obsolete: true

This should prevent most cases of unintended fling acceleration. For example,
when scrolling paragraph-by-paragraph via a series of short flings, we would
often accelerate the next fling even if the previous fling had already slowed
down almost to a stop. With this change, we will no longer trigger acceleration
in that scenario.

This also matches Chrome's behavior.

This patch does not add a new pref for the new velocity threshold.
Chrome also shares the same velocity threshold for both velocities.

Depends on D95468

This avoids unexpected acceleration after a "fling, fling, touch-pause-fling" sequence.

The touch start timestamp is captured in GenericFlingAnimation::Cancel,
and the pan start timestamp is captured when we go from TOUCHING to PANNING.

Depends on D95469

This avoids unexpected acceleration after a "fling, fling, pan-pause-fling" sequence.

Depends on D95470

The other fling acceleration restrictions make it so that there is no need for
this additional restriction anymore.
Removing this restriction allows a fling to be accelerated even if the previous
fling has been "spinning" for a while (a second or more), provided that the page
is still moving fast enough.
This matches Chrome's behavior.

Depends on D95471

An APZC that is scrollable in some direction always accepts a fling, even
if it is not scrollable in the direction of the fling. The actual fling
handoff only happens at the next animation step.

The test that this patch is removing was testing the following:
With immediate handoff enabled, start a fling with a 30-pixel pan
in an APZC tree that has a scroll-grabbing parent APZC with 20 pixels
of scroll range. The scrollgrab parent consumes 20 of the 30 pixels
during the pan, before the fling even starts. The child consumes the
remaining 10 pixels of the pan. Then the fling starts.
The parent accepts that fling! But it can't scroll anywhere. So,
once DoSample is called, the fling is handed off to the child.
Then, another 30 pixel pan is initiated, where the same happens:
The child APZC is scrolled by 30 pixels, then the fling is initiated,
the fling is accepted by the parent, and then handed off to the child.

The fling acceleration patches in this bug make it so that flings are only
accelerated if the new fling animation is initiated in the same APZC that was
running the old fling animation when it was interrupted.
This condition is not met in this test: The fling is initiated in the parent
APZC, but at the point of interruption, the fling had been handed off to the
child and was running in the child.

Depends on D95472

Depends on: 1674279

We capture this timestamp in GenericFlingAnimation::Cancel, and then
FlingAccelerator compares it to aPanStartTime which is an event timestamp.
To avoid unintended behavior when events are delayed, both of these should be
event timestamps.

Depends on D95473

Attachment #9185376 - Attachment is obsolete: true
Attachment #9185377 - Attachment is obsolete: true
Attachment #9185074 - Attachment is obsolete: true
Attachment #9185071 - Attachment description: Bug 1458653 - Don't accelerate a fling if the fingers paused at the beginning of the touch motion. r=kats → Bug 1458653 - Don't accelerate a fling if the fingers paused at the beginning of the touch motion. r=botond

(In reply to bndki34 from comment #40)

I've found that it is sometimes hard to trigger fling acceleration with these patches even when fling acceleration is desired. When I run into this, it's usually because the finger starts too slow and then accelerates during the touch scroll. It's a lot easier if the finger accelerates just before it touches the screen, so that it's already in motion when the touch starts.

And I agree with this point. Because now that flings are no longer dependent on each other (at least from my experience), it's harder or no longer possible to build up acceleration by "flicking in quicker succession." Now, we need to actually flick the screen with a faster motion in each gesture. In terms of practicality, this is far from expected experience. As you already mentioned, we have a natural tendency to go "1. touch the screen" then "2. perform fling gesture" (which makes it difficult to register fast initial speed), rather than "1. perform fling gesture" and "2. touch the screen in the meanwhile."

Just to close the loop on this, this should be much better now. The "minimum pan velocity" restriction for acceleration is now only enforced once the pan has left the "touch slop radius", and as a result, it's much easier to build up scroll speed than in the initial test build. In my testing it matches Chrome's feel.

Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/3aeafa99a96b
Add some basic fling acceleration tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/9267808fe458
Don't accelerate a fling if the previous fling animation has slowed down below a velocity threshold. r=botond
https://hg.mozilla.org/integration/autoland/rev/b5b969654488
Don't accelerate a fling if the fingers paused at the beginning of the touch motion. r=botond
https://hg.mozilla.org/integration/autoland/rev/2216206c8f75
Don't accelerate a fling if the fingers moved too slowly at any point during the touch scroll motion. r=botond
https://hg.mozilla.org/integration/autoland/rev/2c215fa18600
Do not prevent fling acceleration based on the time that has elapsed between two flings. r=botond,geckoview-reviewers,snorp
Blocks: 1675885

These patches have made it into today's Firefox Nightly build. I've also filed bug 1675885 for a small follow-up tweak.

Regressions: 1677275

Release Note Request (optional, but appreciated)
[Why is this notable]: Fixes a long-standing bug that impacted the user experience on Android during scrolling. (In the corresponding github issue, the previous behavior has been described as "weird" and as "barely usable". I agree with that assessment.)
[Affects Firefox for Android]: Yes
[Suggested wording]: Firefox for Android: Improved scrolling accuracy and control, and fixed cases of unexpected scroll acceleration.
[Links (documentation, blog post, etc)]: This bug and bug 1615858

relnote-firefox: --- → ?

That release note doesn't nearly quantify how great your work on scrolling is for Firefox for Android! <333 As a user, your explanation as to why it's notable is better. I often find myself disappointed with Firefox's release notes and go to Reddit or Ghacks or other tech news sites to find additional information.

You need to log in before you can comment on or make changes to this bug.