Unverified Commit 83f19be2 authored by Renzo Olivares's avatar Renzo Olivares Committed by GitHub

Fix bottom sheet rebuilding when tapping (#127526)

This fixes an issue where the bottom sheet would rebuild when `enableDrag` is set to true on every tap. This is because `DragGestureRecognizer` would win the arena by default and dispatch the `drag` callbacks (in `acceptGesture`) even though it had not met the drag threshold. This changes keep the default behavior of `DragGestureRecognizer` the same, but adds a parameter `onlyAcceptDragOnThreshold` that a user can use to stop drag callbacks from being fired when the drag threshold has not been met.

Fixes #126833
parent 347e304e
...@@ -77,6 +77,7 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer { ...@@ -77,6 +77,7 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
super.debugOwner, super.debugOwner,
this.dragStartBehavior = DragStartBehavior.start, this.dragStartBehavior = DragStartBehavior.start,
this.velocityTrackerBuilder = _defaultBuilder, this.velocityTrackerBuilder = _defaultBuilder,
this.onlyAcceptDragOnThreshold = false,
super.supportedDevices, super.supportedDevices,
AllowedButtonsFilter? allowedButtonsFilter, AllowedButtonsFilter? allowedButtonsFilter,
}) : super(allowedButtonsFilter: allowedButtonsFilter ?? _defaultButtonAcceptBehavior); }) : super(allowedButtonsFilter: allowedButtonsFilter ?? _defaultButtonAcceptBehavior);
...@@ -210,6 +211,25 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer { ...@@ -210,6 +211,25 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
/// If null then [kMaxFlingVelocity] is used. /// If null then [kMaxFlingVelocity] is used.
double? maxFlingVelocity; double? maxFlingVelocity;
/// Whether the drag threshold should be met before dispatching any drag callbacks.
///
/// The drag threshold is met when the global distance traveled by a pointer has
/// exceeded the defined threshold on the relevant axis, i.e. y-axis for the
/// [VerticalDragGestureRecognizer], x-axis for the [HorizontalDragGestureRecognizer],
/// and the entire plane for [PanGestureRecognizer]. The threshold for both
/// [VerticalDragGestureRecognizer] and [HorizontalDragGestureRecognizer] are
/// calculated by [computeHitSlop], while [computePanSlop] is used for
/// [PanGestureRecognizer].
///
/// If true, the drag callbacks will only be dispatched when this recognizer has
/// won the arena and the drag threshold has been met.
///
/// If false, the drag callbacks will be dispatched immediately when this recognizer
/// has won the arena.
///
/// This value defaults to false.
bool onlyAcceptDragOnThreshold;
/// Determines the type of velocity estimation method to use for a potential /// Determines the type of velocity estimation method to use for a potential
/// drag gesture, when a new pointer is added. /// drag gesture, when a new pointer is added.
/// ///
...@@ -284,6 +304,7 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer { ...@@ -284,6 +304,7 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
Offset _getDeltaForDetails(Offset delta); Offset _getDeltaForDetails(Offset delta);
double? _getPrimaryValueFromOffset(Offset value); double? _getPrimaryValueFromOffset(Offset value);
bool _hasSufficientGlobalDistanceToAccept(PointerDeviceKind pointerDeviceKind, double? deviceTouchSlop); bool _hasSufficientGlobalDistanceToAccept(PointerDeviceKind pointerDeviceKind, double? deviceTouchSlop);
bool _hasDragThresholdBeenMet = false;
final Map<int, VelocityTracker> _velocityTrackers = <int, VelocityTracker>{}; final Map<int, VelocityTracker> _velocityTrackers = <int, VelocityTracker>{};
...@@ -386,10 +407,15 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer { ...@@ -386,10 +407,15 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
untransformedEndPosition: localPosition untransformedEndPosition: localPosition
).distance * (_getPrimaryValueFromOffset(movedLocally) ?? 1).sign; ).distance * (_getPrimaryValueFromOffset(movedLocally) ?? 1).sign;
if (_hasSufficientGlobalDistanceToAccept(event.kind, gestureSettings?.touchSlop)) { if (_hasSufficientGlobalDistanceToAccept(event.kind, gestureSettings?.touchSlop)) {
_hasDragThresholdBeenMet = true;
if (_acceptedActivePointers.contains(event.pointer)) {
_checkDrag(event.pointer);
} else {
resolve(GestureDisposition.accepted); resolve(GestureDisposition.accepted);
} }
} }
} }
}
if (event is PointerUpEvent || event is PointerCancelEvent || event is PointerPanZoomEndEvent) { if (event is PointerUpEvent || event is PointerCancelEvent || event is PointerPanZoomEndEvent) {
_giveUpPointer(event.pointer); _giveUpPointer(event.pointer);
} }
...@@ -401,45 +427,8 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer { ...@@ -401,45 +427,8 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
void acceptGesture(int pointer) { void acceptGesture(int pointer) {
assert(!_acceptedActivePointers.contains(pointer)); assert(!_acceptedActivePointers.contains(pointer));
_acceptedActivePointers.add(pointer); _acceptedActivePointers.add(pointer);
if (_state != _DragState.accepted) { if (!onlyAcceptDragOnThreshold || _hasDragThresholdBeenMet) {
_state = _DragState.accepted; _checkDrag(pointer);
final OffsetPair delta = _pendingDragOffset;
final Duration? timestamp = _lastPendingEventTimestamp;
final Matrix4? transform = _lastTransform;
final Offset localUpdateDelta;
switch (dragStartBehavior) {
case DragStartBehavior.start:
_initialPosition = _initialPosition + delta;
localUpdateDelta = Offset.zero;
case DragStartBehavior.down:
localUpdateDelta = _getDeltaForDetails(delta.local);
}
_pendingDragOffset = OffsetPair.zero;
_lastPendingEventTimestamp = null;
_lastTransform = null;
_checkStart(timestamp, pointer);
if (localUpdateDelta != Offset.zero && onUpdate != null) {
final Matrix4? localToGlobal = transform != null ? Matrix4.tryInvert(transform) : null;
final Offset correctedLocalPosition = _initialPosition.local + localUpdateDelta;
final Offset globalUpdateDelta = PointerEvent.transformDeltaViaPositions(
untransformedEndPosition: correctedLocalPosition,
untransformedDelta: localUpdateDelta,
transform: localToGlobal,
);
final OffsetPair updateDelta = OffsetPair(local: localUpdateDelta, global: globalUpdateDelta);
final OffsetPair correctedPosition = _initialPosition + updateDelta; // Only adds delta for down behaviour
_checkUpdate(
sourceTimeStamp: timestamp,
delta: localUpdateDelta,
primaryDelta: _getPrimaryValueFromOffset(localUpdateDelta),
globalPosition: correctedPosition.global,
localPosition: correctedPosition.local,
);
}
// This acceptGesture might have been called only for one pointer, instead
// of all pointers. Resolve all pointers to `accepted`. This won't cause
// infinite recursion because an accepted pointer won't be accepted again.
resolve(GestureDisposition.accepted);
} }
} }
...@@ -462,6 +451,7 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer { ...@@ -462,6 +451,7 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
case _DragState.accepted: case _DragState.accepted:
_checkEnd(pointer); _checkEnd(pointer);
} }
_hasDragThresholdBeenMet = false;
_velocityTrackers.clear(); _velocityTrackers.clear();
_initialButtons = null; _initialButtons = null;
_state = _DragState.ready; _state = _DragState.ready;
...@@ -486,6 +476,50 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer { ...@@ -486,6 +476,50 @@ abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
} }
} }
void _checkDrag(int pointer) {
if (_state == _DragState.accepted) {
return;
}
_state = _DragState.accepted;
final OffsetPair delta = _pendingDragOffset;
final Duration? timestamp = _lastPendingEventTimestamp;
final Matrix4? transform = _lastTransform;
final Offset localUpdateDelta;
switch (dragStartBehavior) {
case DragStartBehavior.start:
_initialPosition = _initialPosition + delta;
localUpdateDelta = Offset.zero;
case DragStartBehavior.down:
localUpdateDelta = _getDeltaForDetails(delta.local);
}
_pendingDragOffset = OffsetPair.zero;
_lastPendingEventTimestamp = null;
_lastTransform = null;
_checkStart(timestamp, pointer);
if (localUpdateDelta != Offset.zero && onUpdate != null) {
final Matrix4? localToGlobal = transform != null ? Matrix4.tryInvert(transform) : null;
final Offset correctedLocalPosition = _initialPosition.local + localUpdateDelta;
final Offset globalUpdateDelta = PointerEvent.transformDeltaViaPositions(
untransformedEndPosition: correctedLocalPosition,
untransformedDelta: localUpdateDelta,
transform: localToGlobal,
);
final OffsetPair updateDelta = OffsetPair(local: localUpdateDelta, global: globalUpdateDelta);
final OffsetPair correctedPosition = _initialPosition + updateDelta; // Only adds delta for down behaviour
_checkUpdate(
sourceTimeStamp: timestamp,
delta: localUpdateDelta,
primaryDelta: _getPrimaryValueFromOffset(localUpdateDelta),
globalPosition: correctedPosition.global,
localPosition: correctedPosition.local,
);
}
// This acceptGesture might have been called only for one pointer, instead
// of all pointers. Resolve all pointers to `accepted`. This won't cause
// infinite recursion because an accepted pointer won't be accepted again.
resolve(GestureDisposition.accepted);
}
void _checkStart(Duration? timestamp, int pointer) { void _checkStart(Duration? timestamp, int pointer) {
if (onStart != null) { if (onStart != null) {
final DragStartDetails details = DragStartDetails( final DragStartDetails details = DragStartDetails(
......
...@@ -357,15 +357,14 @@ class _BottomSheetState extends State<BottomSheet> { ...@@ -357,15 +357,14 @@ class _BottomSheetState extends State<BottomSheet> {
dragHandleColor: widget.dragHandleColor, dragHandleColor: widget.dragHandleColor,
dragHandleSize: widget.dragHandleSize, dragHandleSize: widget.dragHandleSize,
); );
// Only add [GestureDetector] to the drag handle when the rest of the // Only add [_BottomSheetGestureDetector] to the drag handle when the rest of the
// bottom sheet is not draggable. If the whole bottom sheet is draggable, // bottom sheet is not draggable. If the whole bottom sheet is draggable,
// no need to add it. // no need to add it.
if (!widget.enableDrag) { if (!widget.enableDrag) {
dragHandle = GestureDetector( dragHandle = _BottomSheetGestureDetector(
onVerticalDragStart: _handleDragStart, onVerticalDragStart: _handleDragStart,
onVerticalDragUpdate: _handleDragUpdate, onVerticalDragUpdate: _handleDragUpdate,
onVerticalDragEnd: _handleDragEnd, onVerticalDragEnd: _handleDragEnd,
excludeFromSemantics: true,
child: dragHandle, child: dragHandle,
); );
} }
...@@ -407,11 +406,10 @@ class _BottomSheetState extends State<BottomSheet> { ...@@ -407,11 +406,10 @@ class _BottomSheetState extends State<BottomSheet> {
); );
} }
return !widget.enableDrag ? bottomSheet : GestureDetector( return !widget.enableDrag ? bottomSheet : _BottomSheetGestureDetector(
onVerticalDragStart: _handleDragStart, onVerticalDragStart: _handleDragStart,
onVerticalDragUpdate: _handleDragUpdate, onVerticalDragUpdate: _handleDragUpdate,
onVerticalDragEnd: _handleDragEnd, onVerticalDragEnd: _handleDragEnd,
excludeFromSemantics: true,
child: bottomSheet, child: bottomSheet,
); );
} }
...@@ -1300,7 +1298,39 @@ PersistentBottomSheetController<T> showBottomSheet<T>({ ...@@ -1300,7 +1298,39 @@ PersistentBottomSheetController<T> showBottomSheet<T>({
); );
} }
class _BottomSheetGestureDetector extends StatelessWidget {
const _BottomSheetGestureDetector({
required this.child,
required this.onVerticalDragStart,
required this.onVerticalDragUpdate,
required this.onVerticalDragEnd,
});
final Widget child;
final GestureDragStartCallback onVerticalDragStart;
final GestureDragUpdateCallback onVerticalDragUpdate;
final GestureDragEndCallback onVerticalDragEnd;
@override
Widget build(BuildContext context) {
return RawGestureDetector(
excludeFromSemantics: true,
gestures: <Type, GestureRecognizerFactory<GestureRecognizer>>{
VerticalDragGestureRecognizer : GestureRecognizerFactoryWithHandlers<VerticalDragGestureRecognizer>(
() => VerticalDragGestureRecognizer(debugOwner: this),
(VerticalDragGestureRecognizer instance) {
instance
..onStart = onVerticalDragStart
..onUpdate = onVerticalDragUpdate
..onEnd = onVerticalDragEnd
..onlyAcceptDragOnThreshold = true;
},
),
},
child: child,
);
}
}
// BEGIN GENERATED TOKEN PROPERTIES - BottomSheet // BEGIN GENERATED TOKEN PROPERTIES - BottomSheet
......
...@@ -74,6 +74,75 @@ void main() { ...@@ -74,6 +74,75 @@ void main() {
GestureBinding.instance.handleEvent(up91, HitTestEntry(MockHitTestTarget())); GestureBinding.instance.handleEvent(up91, HitTestEntry(MockHitTestTarget()));
}); });
testGesture('DragGestureRecognizer should not dispatch drag callbacks when it wins the arena if onlyAcceptDragOnThreshold is true and the threshold has not been met', (GestureTester tester) {
final VerticalDragGestureRecognizer verticalDrag = VerticalDragGestureRecognizer();
final List<String> dragCallbacks = <String>[];
verticalDrag
..onlyAcceptDragOnThreshold = true
..onStart = (DragStartDetails details) {
dragCallbacks.add('onStart');
}
..onUpdate = (DragUpdateDetails details) {
dragCallbacks.add('onUpdate');
}
..onEnd = (DragEndDetails details) {
dragCallbacks.add('onEnd');
};
const PointerDownEvent down1 = PointerDownEvent(
pointer: 6,
position: Offset(10.0, 10.0),
);
const PointerUpEvent up1 = PointerUpEvent(
pointer: 6,
position: Offset(10.0, 10.0),
);
verticalDrag.addPointer(down1);
tester.closeArena(down1.pointer);
tester.route(down1);
tester.route(up1);
expect(dragCallbacks.isEmpty, true);
verticalDrag.dispose();
dragCallbacks.clear();
});
testGesture('DragGestureRecognizer should dispatch drag callbacks when it wins the arena if onlyAcceptDragOnThreshold is false and the threshold has not been met', (GestureTester tester) {
final VerticalDragGestureRecognizer verticalDrag = VerticalDragGestureRecognizer();
final List<String> dragCallbacks = <String>[];
verticalDrag
..onlyAcceptDragOnThreshold = false
..onStart = (DragStartDetails details) {
dragCallbacks.add('onStart');
}
..onUpdate = (DragUpdateDetails details) {
dragCallbacks.add('onUpdate');
}
..onEnd = (DragEndDetails details) {
dragCallbacks.add('onEnd');
};
const PointerDownEvent down1 = PointerDownEvent(
pointer: 6,
position: Offset(10.0, 10.0),
);
const PointerUpEvent up1 = PointerUpEvent(
pointer: 6,
position: Offset(10.0, 10.0),
);
verticalDrag.addPointer(down1);
tester.closeArena(down1.pointer);
tester.route(down1);
tester.route(up1);
expect(dragCallbacks.isEmpty, false);
expect(dragCallbacks, <String>['onStart', 'onEnd']);
verticalDrag.dispose();
dragCallbacks.clear();
});
group('Recognizers on different button filters:', () { group('Recognizers on different button filters:', () {
final List<String> recognized = <String>[]; final List<String> recognized = <String>[];
late HorizontalDragGestureRecognizer primaryRecognizer; late HorizontalDragGestureRecognizer primaryRecognizer;
......
...@@ -199,6 +199,43 @@ void main() { ...@@ -199,6 +199,43 @@ void main() {
expect(find.text('BottomSheet'), findsNothing); expect(find.text('BottomSheet'), findsNothing);
}); });
testWidgets('Tapping on a BottomSheet should not trigger a rebuild when enableDrag is true', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/126833.
final GlobalKey<ScaffoldState> scaffoldKey = GlobalKey<ScaffoldState>();
int buildCount = 0;
await tester.pumpWidget(MaterialApp(
home: Scaffold(
key: scaffoldKey,
body: const Center(child: Text('body')),
),
));
await tester.pump();
expect(buildCount, 0);
expect(find.text('BottomSheet'), findsNothing);
scaffoldKey.currentState!.showBottomSheet<void>((BuildContext context) {
buildCount++;
return const SizedBox(
height: 200.0,
child: Text('BottomSheet'),
);
},
enableDrag: true,
);
await tester.pumpAndSettle();
expect(buildCount, 1);
expect(find.text('BottomSheet'), findsOneWidget);
// Tap on bottom sheet should not trigger a rebuild.
await tester.tap(find.text('BottomSheet'));
await tester.pumpAndSettle();
expect(buildCount, 1);
expect(find.text('BottomSheet'), findsOneWidget);
});
testWidgets('Modal BottomSheet builder should only be called once', (WidgetTester tester) async { testWidgets('Modal BottomSheet builder should only be called once', (WidgetTester tester) async {
late BuildContext savedContext; late BuildContext savedContext;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment