[SuperEditor] Prevent long-press to resolve when swiping to pop (Resolves #1947) #2064
Conversation
| @@ -198,7 +198,10 @@ class TapSequenceGestureRecognizer extends GestureRecognizer { | |||
| _trackers.remove(tracker.pointer); | |||
| tracker.entry.resolve(GestureDisposition.rejected); | |||
| _freezeTracker(tracker); | |||
| if (_firstTap != null || _secondTap != null) { | |||
|
|
|||
| // Check if we need to cancel even when both _firstTap and _secondTap are null, | |||
There was a problem hiding this comment.
I don't really understand this comment. Can you try rephrasing it?
| @@ -372,7 +377,7 @@ class TapSequenceGestureRecognizer extends GestureRecognizer { | |||
| } | |||
|
|
|||
| void _checkCancel() { | |||
| if (_firstTap == null && onTapCancel != null) { | |||
| if ((_firstTap == null || _firstTapDownDetails != null) && onTapCancel != null) { | |||
There was a problem hiding this comment.
Can you add an explanation comment to this line, too?
There was a problem hiding this comment.
Shouldn't _firstTap == null || _firstTapDownDetails != null be an &&?
Also, given that the tap down happens first, temporarily, it's probably more helpful to the reader to reverse those two. First check the tap down, then check the first tap.
There was a problem hiding this comment.
It should be an ||. If the user tapped down, but not released the tap, _firstTap is null.
There was a problem hiding this comment.
That still sounds like an && condition: "the user has tapped down AND the user has not released the tap" - isn't that the condition?
There was a problem hiding this comment.
Currently, if the user:
- Taps down
- Release the tap
- Taps again
- Drags horizontally
We will still fire the cancel event. If we change it to && it will no longer be the case. Is this expected?
| @@ -198,7 +198,16 @@ class TapSequenceGestureRecognizer extends GestureRecognizer { | |||
| _trackers.remove(tracker.pointer); | |||
| tracker.entry.resolve(GestureDisposition.rejected); | |||
| _freezeTracker(tracker); | |||
| if (_firstTap != null || _secondTap != null) { | |||
|
|
|||
| // Both _firstTap and _secondTap are only set when a pointer up event is received, | |||
There was a problem hiding this comment.
I think this is good info, but I'm wondering if we're combining unrelated situations in a single if-statement?
Most of this code was copied and adjusted from Flutter, so don't consider the existing code to be particularly readable. If we can improve the readability, we should.
I notice the first if-statement has 3 OR conditions, which sounds like 3 different things. Then within that if-statement we have another if/else, and the else contains another if. It's a lot of different conditions.
Can you try to refactor this little piece into unique conditions, where each condition explains what it represnts?
There was a problem hiding this comment.
@matthew-carroll Updated. Can you take a look to see if it's better now?
BTW, we still have some unusual code here. In _reject, there are situations that we call _reset. Then, on _reset, we can nullify _firstTap and _secondTap and recursively call _reject again a couple times.
There was a problem hiding this comment.
I assume that's stuff copied from Flutter. I had no idea how this thing worked when I adapted it to our needs. Any time we figure out a little bit of it, we should add comments. Also, where possible, if we can simplify an area that we're working on, we should.
| @@ -372,7 +377,7 @@ class TapSequenceGestureRecognizer extends GestureRecognizer { | |||
| } | |||
|
|
|||
| void _checkCancel() { | |||
| if (_firstTap == null && onTapCancel != null) { | |||
| if ((_firstTap == null || _firstTapDownDetails != null) && onTapCancel != null) { | |||
There was a problem hiding this comment.
Shouldn't _firstTap == null || _firstTapDownDetails != null be an &&?
Also, given that the tap down happens first, temporarily, it's probably more helpful to the reader to reverse those two. First check the tap down, then check the first tap.
[SuperEditor] Prevent long-press to resolve when swiping to pop. Resolves #1947
On mobile, dragging from left to right starts a swipe-to-pop gesture. However, when doing so we are changing the selection on both mobile platforms, showing the magnifier on iOS and showing the mobile toolbar on Android:
android_swipe_before.webm
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-05-30.at.16.28.24.mp4
The issue is that, even though the horizontal drag recognizer is winning in the gesture arena, the
onTapDownis still called, causing the long-press timer to be started.To resolve that, we need to cancel the timer when we lose on the arena.
I found another issue, where the
onCancelisn't being called after the recognizer was defeated in the arena, even if theonTapDownwas already fired.This PR changes the
TapSequenceGestureRecognizerto callonTapCancelafter we lose on the arena and change the mobile interactors to cancel the long-press timers on that callback.I wasn't able to write a failing test for Android, I guess this back gesture is handled different for Android, since I'm not seeing any other recognizers on the arena after starting the swipe.
With this PR's implementation:
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-05-30.at.16.30.05.mp4
android_swipe_after.webm