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

Fix interference in fling-scrolling from cross-axis motion #122338

Merged
merged 2 commits into from
Mar 17, 2023

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Mar 9, 2023

Fixes #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: Class can claim to implement an interface even if the other has privates 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.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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.
@flutter-dashboard flutter-dashboard bot added f: gestures flutter/packages/flutter/gestures repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 9, 2023
@gnprice
Copy link
Member Author

gnprice commented Mar 9, 2023

After this change, if you fling rapidly on Android, the distance the fling travels is a pixel-perfect match for the distance it would travel with a native Android list.

Previously #120420 made that true if you managed to swipe your finger precisely along the scroll axis with negligible cross-axis motion. With that PR now merged, this change makes it true even if you move your finger at an angle, which is the most natural motion for many ways of using a phone.

Here are before/after screen recordings, taken with the platform_tests/scroll_overlay app on a Pixel 5 running Android 13:

Before After
before.mp4
after.mp4

The repro steps are the same as in the report of #120345, except steps 2, 3, 4, and 5 there can be skipped now that #120338 and #119875 are fixed. The numbered items should match exactly between the Android- and Flutter-driven scrolling lists, and in the "after" recording they now do.

@gnprice
Copy link
Member Author

gnprice commented Mar 9, 2023

I say the match is pixel-perfect "if you fling rapidly" because if you make a less rapid fling, you can still get small differences.

I suspect this means that we're estimating a different velocity for the drag than Android is. Flinging rapidly means the velocity is large enough that it gets clamped, masking any such difference, so that this PR fixing the clamping behavior plus #120420 previously fixing the ensuing fling curve is enough to make the behavior match exactly.

Those remaining differences for slower flings are small, and I haven't debugged to work out the pattern of them. I'm also not yet sure they're enough to matter from a user perspective: they're easily noticed when using the scroll_overlay app, which is all about highlighting such differences, but I think they might be awfully subtle when simply using a scrollable in an app and the existing behavior (after this PR) may feel just fine.

@gnprice
Copy link
Member Author

gnprice commented Mar 9, 2023

Windows tests failed with the flake #103230:

[   +1 ms] Evaluating project ':app' using build file 'C:\b\s\w\ir\x\w\flutter\examples\platform_view\android\app\build.gradle'.
[   +1 ms] Caching disabled for Kotlin DSL script compilation (Project/ScriptPlugin/stage1) because:
[        ]   Build cache is disabled
[        ] Skipping Kotlin DSL script compilation (Project/ScriptPlugin/stage1) as it is up-to-date.
[   +1 ms] FAILURE: Build failed with an exception.
[   +3 ms] * Where:
[        ] Script 'C:\b\s\w\ir\x\w\flutter\packages\flutter_tools\gradle\flutter.gradle.kts' line: 34
[        ] * What went wrong:
[        ] Failed to apply plugin class 'Flutter_gradle$FlutterPluginKts'.
[        ] > Comparison method violates its general contract!
[        ] * Try:
[        ] > Run with --debug option to get more log output.
[        ] > Run with --scan to get full insights.
[        ] * Exception is:
[        ] org.gradle.api.internal.plugins.PluginApplicationException: Failed to apply plugin class 'Flutter_gradle$FlutterPluginKts'.

Re-running.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent, thank you! LGTM!

@Piinks Piinks added a: fidelity Matching the OEM platforms better f: scrolling Viewports, list views, slivers, etc. a: quality A truly polished experience autosubmit Merge PR when tree becomes green via auto submit App labels Mar 17, 2023
@auto-submit auto-submit bot merged commit 328f088 into flutter:master Mar 17, 2023
@gnprice gnprice deleted the pr-scroll-clamping branch March 19, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: fidelity Matching the OEM platforms better a: quality A truly polished experience autosubmit Merge PR when tree becomes green via auto submit App f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fling-scrolling is slow unless you swipe precisely vertically
2 participants