Skip to content

Commit

Permalink
Fix interference in fling-scrolling from cross-axis motion
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@gnprice
gnprice committed Mar 9, 2023
1 parent c7681f0 commit 286701b
Show file tree
Hide file tree
Showing 3 changed files with 156 additions and 25 deletions.
8 changes: 6 additions & 2 deletions packages/flutter/lib/src/gestures/drag_details.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,14 +216,18 @@ typedef GestureDragUpdateCallback = void Function(DragUpdateDetails details);
class DragEndDetails {
/// Creates details for a [GestureDragEndCallback].
///
/// If [primaryVelocity] is non-null, its value must match one of the
/// coordinates of `velocity.pixelsPerSecond` and the other coordinate
/// must be zero.
///
/// The [velocity] argument must not be null.
DragEndDetails({
this.velocity = Velocity.zero,
this.primaryVelocity,
}) : assert(
primaryVelocity == null
|| primaryVelocity == velocity.pixelsPerSecond.dx
|| primaryVelocity == velocity.pixelsPerSecond.dy,
|| (primaryVelocity == velocity.pixelsPerSecond.dx && velocity.pixelsPerSecond.dy == 0)
|| (primaryVelocity == velocity.pixelsPerSecond.dy && velocity.pixelsPerSecond.dx == 0),
);

/// The velocity the pointer was moving when it stopped contacting the screen.
Expand Down
93 changes: 70 additions & 23 deletions packages/flutter/lib/src/gestures/monodrag.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,14 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
/// The distance traveled by the pointer since the last update is provided in
/// the callback's `details` argument, which is a [DragUpdateDetails] object.
///
/// If this gesture recognizer recognizes movement on a single axis (a
/// [VerticalDragGestureRecognizer] or [HorizontalDragGestureRecognizer]),
/// then `details` will reflect movement only on that axis and its
/// [DragUpdateDetails.primaryDelta] will be non-null.
/// If this gesture recognizer recognizes movement in all directions
/// (a [PanGestureRecognizer]), then `details` will reflect movement on
/// both axes and its [DragUpdateDetails.primaryDelta] will be null.
///
/// See also:
///
/// * [allowedButtonsFilter], which decides which button will be allowed.
Expand All @@ -162,6 +170,14 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
/// The velocity is provided in the callback's `details` argument, which is a
/// [DragEndDetails] object.
///
/// If this gesture recognizer recognizes movement on a single axis (a
/// [VerticalDragGestureRecognizer] or [HorizontalDragGestureRecognizer]),
/// then `details` will reflect movement only on that axis and its
/// [DragEndDetails.primaryVelocity] will be non-null.
/// If this gesture recognizer recognizes movement in all directions
/// (a [PanGestureRecognizer]), then `details` will reflect movement on
/// both axes and its [DragEndDetails.primaryVelocity] will be null.
///
/// See also:
///
/// * [allowedButtonsFilter], which decides which button will be allowed.
Expand Down Expand Up @@ -258,6 +274,13 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
/// inertia, for example.
bool isFlingGesture(VelocityEstimate estimate, PointerDeviceKind kind);

/// Determines if a gesture is a fling or not, and if so its effective velocity.
///
/// A fling calls its gesture end callback with a velocity, allowing the
/// provider of the callback to respond by carrying the gesture forward with
/// inertia, for example.
DragEndDetails? _considerFling(VelocityEstimate estimate, PointerDeviceKind kind);

Offset _getDeltaForDetails(Offset delta);
double? _getPrimaryValueFromOffset(Offset value);
bool _hasSufficientGlobalDistanceToAccept(PointerDeviceKind pointerDeviceKind, double? deviceTouchSlop);
Expand Down Expand Up @@ -504,33 +527,21 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
}

final VelocityTracker tracker = _velocityTrackers[pointer]!;
final VelocityEstimate? estimate = tracker.getVelocityEstimate();

final DragEndDetails details;
DragEndDetails? details;
final String Function() debugReport;

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),
);
debugReport = () {
return '$estimate; fling at $velocity.';
};
if (estimate == null) {
debugReport = () => 'Could not estimate velocity.';
} else {
details = DragEndDetails(
primaryVelocity: 0.0,
);
debugReport = () {
if (estimate == null) {
return 'Could not estimate velocity.';
}
return '$estimate; judged to not be a fling.';
};
details = _considerFling(estimate, tracker.kind);
debugReport = (details != null)
? () => '$estimate; fling at ${details!.velocity}.'
: () => '$estimate; judged to not be a fling.';
}
invokeCallback<void>('onEnd', () => onEnd!(details), debugReport: debugReport);
details ??= DragEndDetails(primaryVelocity: 0.0);

invokeCallback<void>('onEnd', () => onEnd!(details!), debugReport: debugReport);
}

void _checkCancel() {
Expand Down Expand Up @@ -578,6 +589,19 @@ class VerticalDragGestureRecognizer extends DragGestureRecognizer {
return estimate.pixelsPerSecond.dy.abs() > minVelocity && estimate.offset.dy.abs() > minDistance;
}

@override
DragEndDetails? _considerFling(VelocityEstimate estimate, PointerDeviceKind kind) {
if (!isFlingGesture(estimate, kind)) {
return null;
}
final double maxVelocity = maxFlingVelocity ?? kMaxFlingVelocity;
final double dy = clampDouble(estimate.pixelsPerSecond.dy, -maxVelocity, maxVelocity);
return DragEndDetails(
velocity: Velocity(pixelsPerSecond: Offset(0, dy)),
primaryVelocity: dy,
);
}

@override
bool _hasSufficientGlobalDistanceToAccept(PointerDeviceKind pointerDeviceKind, double? deviceTouchSlop) {
return _globalDistanceMoved.abs() > computeHitSlop(pointerDeviceKind, gestureSettings);
Expand Down Expand Up @@ -620,6 +644,19 @@ class HorizontalDragGestureRecognizer extends DragGestureRecognizer {
return estimate.pixelsPerSecond.dx.abs() > minVelocity && estimate.offset.dx.abs() > minDistance;
}

@override
DragEndDetails? _considerFling(VelocityEstimate estimate, PointerDeviceKind kind) {
if (!isFlingGesture(estimate, kind)) {
return null;
}
final double maxVelocity = maxFlingVelocity ?? kMaxFlingVelocity;
final double dx = clampDouble(estimate.pixelsPerSecond.dx, -maxVelocity, maxVelocity);
return DragEndDetails(
velocity: Velocity(pixelsPerSecond: Offset(dx, 0)),
primaryVelocity: dx,
);
}

@override
bool _hasSufficientGlobalDistanceToAccept(PointerDeviceKind pointerDeviceKind, double? deviceTouchSlop) {
return _globalDistanceMoved.abs() > computeHitSlop(pointerDeviceKind, gestureSettings);
Expand Down Expand Up @@ -660,6 +697,16 @@ class PanGestureRecognizer extends DragGestureRecognizer {
&& estimate.offset.distanceSquared > minDistance * minDistance;
}

@override
DragEndDetails? _considerFling(VelocityEstimate estimate, PointerDeviceKind kind) {
if (!isFlingGesture(estimate, kind)) {
return null;
}
final Velocity velocity = Velocity(pixelsPerSecond: estimate.pixelsPerSecond)
.clampMagnitude(minFlingVelocity ?? kMinFlingVelocity, maxFlingVelocity ?? kMaxFlingVelocity);
return DragEndDetails(velocity: velocity);
}

@override
bool _hasSufficientGlobalDistanceToAccept(PointerDeviceKind pointerDeviceKind, double? deviceTouchSlop) {
return _globalDistanceMoved.abs() > computePanSlop(pointerDeviceKind, gestureSettings);
Expand Down
80 changes: 80 additions & 0 deletions packages/flutter/test/gestures/drag_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,86 @@ void main() {
expect(primaryVelocity, velocity.pixelsPerSecond.dx);
});

/// Drag the pointer at the given velocity, and return the details
/// the recognizer passes to onEnd.
///
/// This method will mutate `recognizer.onEnd`.
DragEndDetails performDragToEnd(GestureTester tester, DragGestureRecognizer recognizer, Offset pointerVelocity) {
late DragEndDetails actual;
recognizer.onEnd = (DragEndDetails details) {
actual = details;
};
final TestPointer pointer = TestPointer();
final PointerDownEvent down = pointer.down(Offset.zero);
recognizer.addPointer(down);
tester.closeArena(pointer.pointer);
tester.route(down);
tester.route(pointer.move(pointerVelocity * 0.025, timeStamp: const Duration(milliseconds: 25)));
tester.route(pointer.move(pointerVelocity * 0.050, timeStamp: const Duration(milliseconds: 50)));
tester.route(pointer.up(timeStamp: const Duration(milliseconds: 50)));
return actual;
}

testGesture('Clamp max pan velocity in 2D, isotropically', (GestureTester tester) {
final PanGestureRecognizer recognizer = PanGestureRecognizer();
addTearDown(recognizer.dispose);

void checkDrag(Offset pointerVelocity, Offset expectedVelocity) {
final DragEndDetails actual = performDragToEnd(tester, recognizer, pointerVelocity);
expect(actual.velocity.pixelsPerSecond, offsetMoreOrLessEquals(expectedVelocity, epsilon: 0.1));
expect(actual.primaryVelocity, isNull);
}

checkDrag(const Offset( 400.0, 400.0), const Offset( 400.0, 400.0));
checkDrag(const Offset( 2000.0, -2000.0), const Offset( 2000.0, -2000.0));
checkDrag(const Offset(-8000.0, -8000.0), const Offset(-5656.9, -5656.9));
checkDrag(const Offset(-8000.0, 6000.0), const Offset(-6400.0, 4800.0));
checkDrag(const Offset(-9000.0, 0.0), const Offset(-8000.0, 0.0));
checkDrag(const Offset(-9000.0, -1000.0), const Offset(-7951.1, - 883.5));
checkDrag(const Offset(-1000.0, 9000.0), const Offset(- 883.5, 7951.1));
checkDrag(const Offset( 0.0, 9000.0), const Offset( 0.0, 8000.0));
});

testGesture('Clamp max vertical-drag velocity vertically', (GestureTester tester) {
final VerticalDragGestureRecognizer recognizer = VerticalDragGestureRecognizer();
addTearDown(recognizer.dispose);

void checkDrag(Offset pointerVelocity, double expectedVelocity) {
final DragEndDetails actual = performDragToEnd(tester, recognizer, pointerVelocity);
expect(actual.primaryVelocity, moreOrLessEquals(expectedVelocity, epsilon: 0.1));
expect(actual.velocity.pixelsPerSecond.dx, 0.0);
expect(actual.velocity.pixelsPerSecond.dy, actual.primaryVelocity);
}

checkDrag(const Offset( 500.0, 400.0), 400.0);
checkDrag(const Offset( 3000.0, -2000.0), -2000.0);
checkDrag(const Offset(-9000.0, -9000.0), -8000.0);
checkDrag(const Offset(-9000.0, 0.0), 0.0);
checkDrag(const Offset(-9000.0, 1000.0), 1000.0);
checkDrag(const Offset(-1000.0, -9000.0), -8000.0);
checkDrag(const Offset( 0.0, -9000.0), -8000.0);
});

testGesture('Clamp max horizontal-drag velocity horizontally', (GestureTester tester) {
final HorizontalDragGestureRecognizer recognizer = HorizontalDragGestureRecognizer();
addTearDown(recognizer.dispose);

void checkDrag(Offset pointerVelocity, double expectedVelocity) {
final DragEndDetails actual = performDragToEnd(tester, recognizer, pointerVelocity);
expect(actual.primaryVelocity, moreOrLessEquals(expectedVelocity, epsilon: 0.1));
expect(actual.velocity.pixelsPerSecond.dx, actual.primaryVelocity);
expect(actual.velocity.pixelsPerSecond.dy, 0.0);
}

checkDrag(const Offset( 500.0, 400.0), 500.0);
checkDrag(const Offset( 3000.0, -2000.0), 3000.0);
checkDrag(const Offset(-9000.0, -9000.0), -8000.0);
checkDrag(const Offset(-9000.0, 0.0), -8000.0);
checkDrag(const Offset(-9000.0, 1000.0), -8000.0);
checkDrag(const Offset(-1000.0, -9000.0), -1000.0);
checkDrag(const Offset( 0.0, -9000.0), 0.0);
});

testGesture('Synthesized pointer events are ignored for velocity tracking', (GestureTester tester) {
final HorizontalDragGestureRecognizer drag = HorizontalDragGestureRecognizer() ..dragStartBehavior = DragStartBehavior.down;
addTearDown(drag.dispose);
Expand Down

0 comments on commit 286701b

Please sign in to comment.