Commit 61240fe8 authored by Adam Barth's avatar Adam Barth Committed by GitHub

Tap above LongPressDraggable should work (#6348)

Previously, we were nulling out the ArenaEntry in MultiDragPointerState
too early, which was prevent MultiDragPointerState from rejecting the
gesture in `dispose` if we hadn't accepted by the time the pointer went
up. Now we properly reject the gesture, which causes the tap gesture to
win during `sweep` in the arena.

Also, add a bunch of docs and annotations based on information I learned
while studying this issue. Finally, rename a private member of tap
recognizer to a name that would have confused me less in my
investigation.

Fixes #1186
parent 6399a3af
...@@ -92,6 +92,18 @@ class GestureArenaManager { ...@@ -92,6 +92,18 @@ class GestureArenaManager {
} }
/// Forces resolution of the arena, giving the win to the first member. /// Forces resolution of the arena, giving the win to the first member.
///
/// Sweep is typically after all the other processing for a [PointerUpEvent]
/// have taken place. It ensures that multiple passive gestures do not cause a
/// stalemate that prevents the user from interacting with the app.
///
/// Recognizers that wish to delay resolving an arena past [PointerUpEvent]
/// should call [hold] to delay sweep until [release] is called.
///
/// See also:
///
/// * [hold]
/// * [release]
void sweep(int pointer) { void sweep(int pointer) {
_GestureArena state = _arenas[pointer]; _GestureArena state = _arenas[pointer];
if (state == null) if (state == null)
...@@ -112,6 +124,17 @@ class GestureArenaManager { ...@@ -112,6 +124,17 @@ class GestureArenaManager {
} }
/// Prevents the arena from being swept. /// Prevents the arena from being swept.
///
/// Typically, a winner is chosen in an arena after all the other
/// [PointerUpEvent] processing by [sweep]. If a recognizer wishes to delay
/// resolving an arena past [PointerUpEvent], the recognizer can [hold] the
/// arena open using this function. To release such a hold and let the arena
/// resolve, call [release].
///
/// See also:
///
/// * [sweep]
/// * [release]
void hold(int pointer) { void hold(int pointer) {
_GestureArena state = _arenas[pointer]; _GestureArena state = _arenas[pointer];
if (state == null) if (state == null)
...@@ -123,6 +146,11 @@ class GestureArenaManager { ...@@ -123,6 +146,11 @@ class GestureArenaManager {
/// ///
/// If a sweep was attempted on a held arena, the sweep will be done /// If a sweep was attempted on a held arena, the sweep will be done
/// on release. /// on release.
///
/// See also:
///
/// * [sweep]
/// * [hold]
void release(int pointer) { void release(int pointer) {
_GestureArena state = _arenas[pointer]; _GestureArena state = _arenas[pointer];
if (state == null) if (state == null)
......
...@@ -73,6 +73,7 @@ abstract class MultiDragPointerState { ...@@ -73,6 +73,7 @@ abstract class MultiDragPointerState {
/// Resolve this pointer's entry in the [GestureArenaManager] with the given disposition. /// Resolve this pointer's entry in the [GestureArenaManager] with the given disposition.
@protected @protected
@mustCallSuper
void resolve(GestureDisposition disposition) { void resolve(GestureDisposition disposition) {
_arenaEntry.resolve(disposition); _arenaEntry.resolve(disposition);
} }
...@@ -131,7 +132,6 @@ abstract class MultiDragPointerState { ...@@ -131,7 +132,6 @@ abstract class MultiDragPointerState {
void _up() { void _up() {
assert(_arenaEntry != null); assert(_arenaEntry != null);
_arenaEntry = null;
if (_client != null) { if (_client != null) {
assert(pendingDelta == null); assert(pendingDelta == null);
final DragEndDetails details = new DragEndDetails(velocity: _velocityTracker.getVelocity() ?? Velocity.zero); final DragEndDetails details = new DragEndDetails(velocity: _velocityTracker.getVelocity() ?? Velocity.zero);
...@@ -147,7 +147,6 @@ abstract class MultiDragPointerState { ...@@ -147,7 +147,6 @@ abstract class MultiDragPointerState {
void _cancel() { void _cancel() {
assert(_arenaEntry != null); assert(_arenaEntry != null);
_arenaEntry = null;
if (_client != null) { if (_client != null) {
assert(pendingDelta == null); assert(pendingDelta == null);
final Drag client = _client; final Drag client = _client;
...@@ -165,6 +164,7 @@ abstract class MultiDragPointerState { ...@@ -165,6 +164,7 @@ abstract class MultiDragPointerState {
@mustCallSuper @mustCallSuper
void dispose() { void dispose() {
_arenaEntry?.resolve(GestureDisposition.rejected); _arenaEntry?.resolve(GestureDisposition.rejected);
_arenaEntry = null;
assert(() { _pendingDelta = null; return true; }); assert(() { _pendingDelta = null; return true; });
} }
} }
......
...@@ -62,7 +62,8 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer { ...@@ -62,7 +62,8 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer {
final Map<int, GestureArenaEntry> _entries = <int, GestureArenaEntry>{}; final Map<int, GestureArenaEntry> _entries = <int, GestureArenaEntry>{};
final Set<int> _trackedPointers = new HashSet<int>(); final Set<int> _trackedPointers = new HashSet<int>();
/// Called when a pointer event is routed to this recongizer. /// Called when a pointer event is routed to this recognizer.
@protected
void handleEvent(PointerEvent event); void handleEvent(PointerEvent event);
@override @override
...@@ -75,9 +76,12 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer { ...@@ -75,9 +76,12 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer {
/// ///
/// The given pointer ID is the ID of the last pointer this recognizer was /// The given pointer ID is the ID of the last pointer this recognizer was
/// tracking. /// tracking.
@protected
void didStopTrackingLastPointer(int pointer); void didStopTrackingLastPointer(int pointer);
/// Resolves this recognizer's participation in each gesture arena with the given disposition. /// Resolves this recognizer's participation in each gesture arena with the given disposition.
@protected
@mustCallSuper
void resolve(GestureDisposition disposition) { void resolve(GestureDisposition disposition) {
List<GestureArenaEntry> localEntries = new List<GestureArenaEntry>.from(_entries.values); List<GestureArenaEntry> localEntries = new List<GestureArenaEntry>.from(_entries.values);
_entries.clear(); _entries.clear();
...@@ -100,6 +104,7 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer { ...@@ -100,6 +104,7 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer {
/// The pointer events are delivered to [handleEvent]. /// The pointer events are delivered to [handleEvent].
/// ///
/// Use [stopTrackingPointer] to remove the route added by this function. /// Use [stopTrackingPointer] to remove the route added by this function.
@protected
void startTrackingPointer(int pointer) { void startTrackingPointer(int pointer) {
GestureBinding.instance.pointerRouter.addRoute(pointer, handleEvent); GestureBinding.instance.pointerRouter.addRoute(pointer, handleEvent);
_trackedPointers.add(pointer); _trackedPointers.add(pointer);
...@@ -113,6 +118,7 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer { ...@@ -113,6 +118,7 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer {
/// call [didStopTrackingLastPointer] synchronously. /// call [didStopTrackingLastPointer] synchronously.
/// ///
/// Use [startTrackingPointer] to add the routes in the first place. /// Use [startTrackingPointer] to add the routes in the first place.
@protected
void stopTrackingPointer(int pointer) { void stopTrackingPointer(int pointer) {
if (_trackedPointers.contains(pointer)) { if (_trackedPointers.contains(pointer)) {
GestureBinding.instance.pointerRouter.removeRoute(pointer, handleEvent); GestureBinding.instance.pointerRouter.removeRoute(pointer, handleEvent);
...@@ -124,6 +130,7 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer { ...@@ -124,6 +130,7 @@ abstract class OneSequenceGestureRecognizer extends GestureRecognizer {
/// Stops tracking the pointer associated with the given event if the event is /// Stops tracking the pointer associated with the given event if the event is
/// a [PointerUpEvent] or a [PointerCancelEvent] event. /// a [PointerUpEvent] or a [PointerCancelEvent] event.
@protected
void stopTrackingIfPointerNoLongerDown(PointerEvent event) { void stopTrackingIfPointerNoLongerDown(PointerEvent event) {
if (event is PointerUpEvent || event is PointerCancelEvent) if (event is PointerUpEvent || event is PointerCancelEvent)
stopTrackingPointer(event.pointer); stopTrackingPointer(event.pointer);
...@@ -202,11 +209,13 @@ abstract class PrimaryPointerGestureRecognizer extends OneSequenceGestureRecogni ...@@ -202,11 +209,13 @@ abstract class PrimaryPointerGestureRecognizer extends OneSequenceGestureRecogni
} }
/// Override to provide behavior for the primary pointer when the gesture is still possible. /// Override to provide behavior for the primary pointer when the gesture is still possible.
@protected
void handlePrimaryPointer(PointerEvent event); void handlePrimaryPointer(PointerEvent event);
/// Override to be notified when [deadline] is exceeded. /// Override to be notified when [deadline] is exceeded.
/// ///
/// You must override this method if you supply a [deadline]. /// You must override this method if you supply a [deadline].
@protected
void didExceedDeadline() { void didExceedDeadline() {
assert(deadline == null); assert(deadline == null);
} }
......
...@@ -59,7 +59,7 @@ typedef void GestureTapCancelCallback(); ...@@ -59,7 +59,7 @@ typedef void GestureTapCancelCallback();
/// [TapGestureRecognizer] considers all the pointers involved in the pointer /// [TapGestureRecognizer] considers all the pointers involved in the pointer
/// event sequence as contributing to one gesture. For this reason, extra /// event sequence as contributing to one gesture. For this reason, extra
/// pointer interactions during a tap sequence are not recognized as additional /// pointer interactions during a tap sequence are not recognized as additional
/// taps. Fo example, down-1, down-2, up-1, up-2 produces only one tap on up-1. /// taps. For example, down-1, down-2, up-1, up-2 produces only one tap on up-1.
/// ///
/// See also: /// See also:
/// ///
...@@ -84,7 +84,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer { ...@@ -84,7 +84,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer {
GestureTapCancelCallback onTapCancel; GestureTapCancelCallback onTapCancel;
bool _sentTapDown = false; bool _sentTapDown = false;
bool _wonArena = false; bool _wonArenaForPrimaryPointer = false;
Point _finalPosition; Point _finalPosition;
@override @override
...@@ -97,7 +97,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer { ...@@ -97,7 +97,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer {
@override @override
void resolve(GestureDisposition disposition) { void resolve(GestureDisposition disposition) {
if (_wonArena && disposition == GestureDisposition.rejected) { if (_wonArenaForPrimaryPointer && disposition == GestureDisposition.rejected) {
if (onTapCancel != null) if (onTapCancel != null)
onTapCancel(); onTapCancel();
_reset(); _reset();
...@@ -115,7 +115,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer { ...@@ -115,7 +115,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer {
super.acceptGesture(pointer); super.acceptGesture(pointer);
if (pointer == primaryPointer) { if (pointer == primaryPointer) {
_checkDown(); _checkDown();
_wonArena = true; _wonArenaForPrimaryPointer = true;
_checkUp(); _checkUp();
} }
} }
...@@ -140,7 +140,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer { ...@@ -140,7 +140,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer {
} }
void _checkUp() { void _checkUp() {
if (_wonArena && _finalPosition != null) { if (_wonArenaForPrimaryPointer && _finalPosition != null) {
resolve(GestureDisposition.accepted); resolve(GestureDisposition.accepted);
if (onTapUp != null) if (onTapUp != null)
onTapUp(new TapUpDetails(globalPosition: _finalPosition)); onTapUp(new TapUpDetails(globalPosition: _finalPosition));
...@@ -152,7 +152,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer { ...@@ -152,7 +152,7 @@ class TapGestureRecognizer extends PrimaryPointerGestureRecognizer {
void _reset() { void _reset() {
_sentTapDown = false; _sentTapDown = false;
_wonArena = false; _wonArenaForPrimaryPointer = false;
_finalPosition = null; _finalPosition = null;
} }
......
...@@ -1200,6 +1200,29 @@ void main() { ...@@ -1200,6 +1200,29 @@ void main() {
expect(find.text('Target'), findsOneWidget); expect(find.text('Target'), findsOneWidget);
}); });
testWidgets('Tap above long-press draggable works', (WidgetTester tester) async {
List<String> events = <String>[];
await tester.pumpWidget(new MaterialApp(
home: new Material(
child: new Center(
child: new GestureDetector(
onTap: () {
events.add('tap');
},
child: new LongPressDraggable<int>(
feedback: new Text('Feedback'),
child: new Text('X'),
),
),
),
),
));
expect(events, isEmpty);
await tester.tap(find.text('X'));
expect(events, equals(<String>['tap']));
});
} }
class DragTargetData { } class DragTargetData { }
......
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