-
Notifications
You must be signed in to change notification settings - Fork 28k
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
Conversation
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.
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
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. |
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 |
Windows tests failed with the flake #103230:
Re-running. |
There was a problem hiding this 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!
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 existingonEnd
behavior is inconsistent with that; and whatever someone might try to do with cross-axis velocity fromonEnd
, it seems like it'd be hard for them to be doing it successfully already whenonUpdate
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
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.