Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fling-scrolling is slow unless you swipe precisely vertically #120345

Closed
gnprice opened this issue Feb 9, 2023 · 5 comments · Fixed by #122338
Closed

Fling-scrolling is slow unless you swipe precisely vertically #120345

gnprice opened this issue Feb 9, 2023 · 5 comments · Fixed by #122338
Labels
f: scrolling Viewports, list views, slivers, etc. found in release: 3.7 Found to occur in 3.7 found in release: 3.8 Found to occur in 3.8 framework flutter/packages/flutter repository. See also f: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on P2 Important issues not at the top of the work list r: fixed Issue is closed as already fixed in a newer version

Comments

@gnprice
Copy link
Member

gnprice commented Feb 9, 2023

When trying to scroll rapidly through a list, the fastest you can go is much slower if you swipe at an angle and not straight up or down: about 15%-30% less initial speed, and 20%-50% less total distance on a single fling, for a range of likely angles.

Swiping at an angle is the most natural motion for many ways of using a phone, especially when using the thumb to scroll. And Android native scrolling (*) does not share this behavior: swiping at an angle can reach the same top speeds as swiping precisely vertically. As a result, many users are accustomed to scrolling this way, and the way an app responds when they do is their primary experience of trying to scroll in the app.

((*) I mention Android because that's where I've tested. It's possible that iOS or other platforms share Flutter's behavior here, or Android's, or do something else.)

This is one of several issues that combine to produce user feedback that scrolling feels sluggish and unresponsive in the default scrolling behavior used on Android. The symptoms naturally blend together until you investigate closely, so here's the others I know of for cross-reference:

Steps to Reproduce

  1. Run the platform_tests/scroll_overlay app on an Android device. (This may be the same on iOS and other platforms too, but I haven't yet tested that.)

  2. In lib/main.dart, delete the line itemCount: 1000,, in order to avoid Scroll protocol assumes ballistic scroll physics, and ClampingScrollPhysics isn't ballistic #120338. Reload.

  3. Scroll with a fling gesture: swipe rapidly upward and release. Wait a couple of seconds for the scroll to come to a stop.

  4. Observe how far the scroll went, as shown by the numbered items in the Android- and Flutter-driven scrolling lists.

  5. Scroll back to the top.

  6. Repeat step 2, but this time swipe up at an angle. For example, swipe rapidly from about the lower-left corner of the screen to the upper-right corner.

    • You may find this a much more natural gesture than swiping straight upward, especially if you use your thumb. (At least it is for me, and others I've observed with this repro.)
  7. Repeat step 3.

Expected results:

Swiping at top speed should cause the list to scroll equally far whether you do it straight vertically up (which can be natural when using the index finger) or up at an angle (which is natural when using the thumb, and in many postures with the index finger.)

Actual results:

The Flutter list scrolls much less far when swiping at an angle than it does when swiping straight up.

The Android list, on the other hand, scrolls the same distance in both cases, or nearly so. (If it doesn't, try again but swiping faster. There's a certain maximum fling distance no matter how hard you swipe, and it's the same whether straight up or at an angle. The total numbers may get slightly smaller at an angle, because they include the initial portion when your finger was still on the screen performing a drag, and that part is shorter when at an angle.)

Specifically:

  • Swiping straight up, I see "Android 94" and "Flutter 90" at the top of the screen. (The remaining discrepancy in this case is ClampingScrollSimulation decays sooner and goes less far than native Android scrolling #119875.)
  • Swiping diagonally up at roughly a 30-degree angle from vertical (from about one corner to the opposite corner, on my phone in portrait mode), I see "Android 93" next to "Flutter 78".
  • Swiping diagonally up at roughly a 45-degree angle, I see "Android 90" next to "Flutter 51".

This represents about a 19% reduction in the 30-degree case and a 56% reduction in the 45-degree case, compared to straight up. (By my calculations, using the fact that item N is 40+N logical pixels high.)

Logs
$ flutter analyze
Analyzing scroll_overlay...                                             
No issues found! (ran in 0.8s)
$ flutter doctor -v
[✓] Flutter (Channel main, 3.8.0-8.0.pre.13, on Debian GNU/Linux 10 (buster) 4.19.0-23-amd64, locale en_US.UTF-8)
    • Flutter version 3.8.0-8.0.pre.13 on channel main at /home/greg/n/flutter/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 2303f42250 (3 hours ago), 2023-02-08 14:24:12 -0500
    • Engine revision cc4ca6a06a
    • Dart version 3.0.0 (build 3.0.0-215.0.dev)
    • DevTools version 2.21.1

[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.1)
    • Android SDK at /home/greg/Android/Sdk
    • Platform android-33, build-tools 33.0.1
    • Java binary at: /home/greg/lib/android-studio-2020.3.1.24/jbr/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301)
    • All Android licenses accepted.

[✓] Chrome - develop for the web
    • Chrome at google-chrome

[✓] Linux toolchain - develop for Linux desktop
    • clang version 7.0.1-8+deb10u2 (tags/RELEASE_701/final)
    • cmake version 3.13.4
    • ninja version 1.8.2
    • pkg-config version 0.29

[✓] Android Studio (version 2022.1)
    • Android Studio at /home/greg/lib/android-studio-2020.3.1.24
    • Flutter plugin version 71.2.4
    • Dart plugin version 221.6096
    • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301)

[✓] Android Studio (version 2021.2)
    • Android Studio at /opt/android-studio
    • Flutter plugin version 71.1.2
    • Dart plugin version 212.5744
    • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301)

[✓] VS Code (version 1.75.0)
    • VS Code at /usr/share/code
    • Flutter extension can be installed from:
      🔨 https://marketplace.visualstudio.com/items?itemName=Dart-Code.flutter

[✓] Connected device (3 available)
    • Pixel 5 (mobile) • 08171FDD40025F • android-arm64  • Android 13 (API 33)
    • Linux (desktop)  • linux          • linux-x64      • Debian GNU/Linux 10 (buster) 4.19.0-23-amd64
    • Chrome (web)     • chrome         • web-javascript • Google Chrome 109.0.5414.119

[✓] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!
@gnprice
Copy link
Member Author

gnprice commented Feb 9, 2023

Diagnosis

What's going on here is:

  • Both Flutter and Android clamp the velocity to a certain maximum at the start of the fling.
    (And I haven't yet investigated what iOS does.)
  • The total distance the fling travels is a function of its starting velocity. That function isn't quite the same between Flutter and Android (that discrepancy is ClampingScrollSimulation decays sooner and goes less far than native Android scrolling #119875), but it scales at the same rate: Flutter's distance is a consistent 7.2% less than Android's.
  • They both use the same number for that maximum velocity: 8000 logical pixels per second.
  • But they apply the maximum velocity in different ways.
  • As a result:
    • On Android, swiping rapidly straight up or rapidly up at an angle will both produce a velocity of 8000 logical pixels per second, and a fling that goes the maximum distance.
    • But on Flutter, only swiping exactly straight up will do that.
      • Swiping at a 30-degree angle produces a velocity of about 6930 px/sec (13% reduced) and a total distance about 22% shorter than maximum.
      • Swiping at a 45-degree angle produces a velocity of about 5660 px/sec (29% reduced) and a total distance about 45% shorter than maximum.
      • (These theoretically-predicted figures differ a bit from the ones I estimated by measurement above. I think they're within the error bars — my estimates of the angles I was swiping at were from pretty rough eyeballing.)

Those two different versions of clamping to a maximum velocity are:

Android

Android takes the velocity in each axis, x and y, and clamps each of them independently. From RecyclerView.java:

                velocityX = Math.max(-mMaxFlingVelocity, Math.min(velocityX, mMaxFlingVelocity));
                velocityY = Math.max(-mMaxFlingVelocity, Math.min(velocityY, mMaxFlingVelocity));
                mViewFlinger.fling(velocityX, velocityY);

A few lines before that, in fact, it looks at which axes the view can scroll in, and zeroes out the velocity in any axis that won't scroll:

        if (!canScrollHorizontal || Math.abs(velocityX) < mMinFlingVelocity) {
            velocityX = 0;
        }
        if (!canScrollVertical || Math.abs(velocityY) < mMinFlingVelocity) {
            velocityY = 0;
        }
        if (velocityX == 0 && velocityY == 0) {
            return false;
        }

For the case of a view that scrolls in only one axis, this produces the desirable behavior seen above.

Flutter

The current version of Flutter takes the velocity as a two-dimensional vector, and clamps that to a maximum length. From DragGestureRecognizer._checkEnd (if I've correctly traced this up the call graph):

    final VelocityEstimate? estimate = tracker.getVelocityEstimate();
    if (estimate != null && isFlingGesture(estimate, tracker.kind)) {
      final Velocity velocity = Velocity(pixelsPerSecond: estimate.pixelsPerSecond)
        .clampMagnitude(minFlingVelocity ?? kMinFlingVelocity, maxFlingVelocity ?? kMaxFlingVelocity);
      details = DragEndDetails(
        velocity: velocity,
        primaryVelocity: _getPrimaryValueFromOffset(velocity.pixelsPerSecond),
      );

Here estimate.pixelsPerSecond is an Offset, so a 2-d vector, and Velocity.clampMagnitude clamps that to a maximum Euclidean length, as computed using the Pythagorean theorem.

Then the call to _getPrimaryValueFromOffset, which is abstract on DragGestureRecognizer and implemented by its subclasses, picks out the component of the remaining velocity that goes in the relevant axis. That's the part that ScrollDragController.end pays attention to, and passes on to goBallistic.

In the case of something that can scroll in two dimensions, this logic makes more sense than the Android logic and probably produces more natural behavior — it means that the maximum speed is always the same 8000 px/sec, in every direction, whereas the Android logic would have it range from 8000 px/sec up to sqrt(2) times that, or about 11300 px/sec, when travelling at a 45-degree angle to the axes.

But when the view is only scrolling in one axis, the resulting behavior is not physically natural — it's as if you had a physical object mounted on a linear rail, but when you pushed it hard along the rail, it started resisting you much harder and moving more slowly if you also pushed it somewhat to the side. You could build a physical system like that, but it means one with a lot of friction, which is not the metaphor one wants for a good scrolling experience.

More concretely, this behavior produces the user perception of sluggishness described above. And it differs from Android's, which is a fidelity issue.

Toward a solution

Instead, when we're in a DragGestureRecognizer subclass that's looking for drag gestures in a single axis (namely VerticalDragGestureRecognizer or HorizontalDragGestureRecognizer), we should allow the velocity in that axis to reach the maximum even when the input velocity has a component in the other axis too.

I haven't yet looked into what a good way to do that in the code would look like. It may require a change to the API implemented by DragGestureRecognizer subclasses.

@exaby73
Copy link
Member

exaby73 commented Feb 9, 2023

Hello @gnprice. Apologies if I do not understand the issue completely, but could you highlight what this issue adds that isn't already in the issues you mentioned? There's a lot of information you provide - thanks :) - but I'm having a tough time sifting through it.

Also, the steps that you provided to reproduce this issue, I'm afraid that could be hindered by human error. Testing it, it looks like the scroll view sort of follows the finger. Flinging the scroll view, I am not sure if it's Flutter influencing the scroll speed or whether I was slower that time flinging it. I have also seen that sometimes, flinging at an angle scrolls further than flinging vertically, though not by much which is probably because of human error.

Is there a consistent way we can test this behavior? Can, for example, tests help us here to reproduce a consistent scrolling behavior? This would help verify the behavior and provide a definite way to check if the issue is fixed in the future.

@exaby73 exaby73 added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Feb 9, 2023
gnprice added a commit to gnprice/flutter that referenced this issue Feb 10, 2023
Fixes flutter#120338.
Fixes flutter#119875.
Fixes flutter#113424.

Fixes most of flutter#16371, as the combined symptom of those three.
(There's still flutter#120345, and possibly other contributing factors,
remaining.)

This replaces the implementation of ClampingScrollSimulation with
a new version that better matches the Android scroll physics it's
intended to match.

The new implementation is a version of the Android curve which has
been adjusted so that it behaves in a physically reasonable way,
in order to satisfy an invariant which the Flutter scroll protocol
relies on.

Because the old ClampingScrollSimulation didn't satisfy that
invariant either, it suffered from a bug (flutter#120338) where
restarting the simulation with new metrics would have the side
effect of adding friction, making a fling go about 15% less far.
So this fixes flutter#120338.

The new implementation also goes the same total distance as the
Android scroll physics, whereas the existing version would go about
7% less far (stacking with the 15% reduction when the other issue
was triggered.)  That fixes flutter#119875.

Finally, the new implementation comes to a smooth stop, like Android
does, whereas the existing version would speed slightly up before
abruptly stopping.  That fixes flutter#113424.

The new implementation's curve is the unique curve that both goes the
same total distance as Android's for every possible starting velocity,
and meets the Flutter scroll protocol's ballistic invariant.  So in
that sense this is the canonical way of fixing up the Android behavior
to something physically reasonable, or something that works correctly
in Flutter.
@exaby73
Copy link
Member

exaby73 commented Feb 10, 2023

Upon consulting with a colleague of mine, I've decided to label this issue. I've sat down and and reproduced this behavior over and over and I'd say around 80 - 90% of the time I can reproduce this behavior. The other 10 - 20% I'd consider error on my end. Hope to get further insights from the team on this issue :)

@exaby73 exaby73 added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. has reproducible steps The issue has been confirmed reproducible and is ready to work on found in release: 3.7 Found to occur in 3.7 found in release: 3.8 Found to occur in 3.8 and removed waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds in triage Presently being triaged by the triage team labels Feb 10, 2023
@gnprice
Copy link
Member Author

gnprice commented Feb 10, 2023

Thanks!

but could you highlight what this issue adds that isn't already in the issues you mentioned? There's a lot of information you provide - thanks :) - but I'm having a tough time sifting through it.

Sure. The key part that's distinct about this issue, from a user-visible perspective, is that it specifically affects the scrolling when you do the swipe at an angle, and not when it's close to precisely vertical.

This issue makes swiping rapidly at an angle produce a slower fling-scroll, no matter how fast you swipe, than you get when swiping rapidly straight upward. The resulting fling-scroll also goes less far before stopping.

When you swipe straight up, this issue doesn't come into play. The scroll may still be slower than it should be, and slower than the native behavior, due to the other issues. When you swipe at an angle, the other issues are still present but this one slows the scroll down still more, and widens the gap from the native behavior.

I'd say around 80 - 90% of the time I can reproduce this behavior. The other 10 - 20% I'd consider error on my end.

Cool. Yeah, I find that trying to swipe at a particular angle is fairly imprecise for me too. But an 80-90% reproduction rate is definitely reliable enough for investigating an issue and manually testing a fix.

And I definitely agree we'll want unit tests to accompany any fix. Fortunately those can be a lot more precise than a human finger on a screen, so they should be very reliable.

@goderbauer goderbauer added the P2 Important issues not at the top of the work list label Feb 14, 2023
gnprice added a commit to gnprice/flutter that referenced this issue Mar 9, 2023
Fixes flutter#120345.

The main change here is that when a single-axis DragGestureRecognizer
(say, a VerticalDragGestureRecognizer) sees the end of a drag, it now
projects the drag's velocity onto its own single axis, and then clamps
that.  Previously it would clamp the velocity's magnitude as a 2D
vector, and then consumers would project *that*, which resulted in
clamping too hard when the motion wasn't precisely on-axis.

For PanGestureRecognizer, the behavior is unchanged: we continue
clamping the velocity as a 2D vector, which is the geometrically nice
behavior when we're not trying to focus on a single axis.

When I'd first looked at this code, I was concerned that fixing this
issue might require a breaking change.  But happily I believe it
doesn't, for two reasons:

 * This base class DragGestureRecognizer is effectively sealed (even
   though Dart doesn't quite yet have that feature), because it has
   private abstract methods.

   Dart won't actually stop you extending it, but that seems like a
   bug in Dart (*); when the class's own method implementations go try
   to call those methods, they'll produce runtime errors.  So it's not
   possible to subclass it in any useful way from outside the library,
   which means there are likely no such subclasses anywhere.  That
   means we can freely alter the API between the base class and its
   subclasses.

   (*) In fact, it's in the Dart tracker:
     dart-lang/sdk#25462

 * We're also changing the behavior of the `onEnd` callback:
   previously it would return a DragEndDetails with a two-dimensional
   velocity, even for a single-axis recognizer, and now it returns a
   velocity that's always zero in the cross axis.  In principle that
   could be a breaking change; it's a bit odd for a
   VerticalDragGestureRecognizer to be telling you there was
   horizontal as well as vertical motion, but someone could
   nevertheless be depending on that.

   Fortunately, it turns out that the `onUpdate` callback's behavior
   already agrees with the view that that is an odd thing that
   shouldn't happen: it already returns an axis-locked velocity from
   single-axis recognizers.  So the existing `onEnd` behavior is
   inconsistent with that; and whatever someone might try to do with
   cross-axis velocity from `onEnd`, it seems like it'd be hard for
   them to be doing it successfully already when `onUpdate` isn't
   going along.  That makes me hopeful that nobody is depending on
   that behavior and we can freely clean it up.

Also add tests, not only for the changes but for the existing
behavior: it turns out that the fancy 2-D clamping, which we keep for
PanGestureRecognizer because it's helpful there, wasn't tested at all.
@exaby73 exaby73 added the r: fixed Issue is closed as already fixed in a newer version label Mar 20, 2023
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f: scrolling Viewports, list views, slivers, etc. found in release: 3.7 Found to occur in 3.7 found in release: 3.8 Found to occur in 3.8 framework flutter/packages/flutter repository. See also f: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on P2 Important issues not at the top of the work list r: fixed Issue is closed as already fixed in a newer version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants