TapAndDragGestureRecognizer should declare victory immediately when drag is detected

[Renzo-Olivares]

This change does the following:

• Similar to other DragGestureRecognizers in the framework, the BaseTapAndDragGestureRecognizer should declare victory immediately when it detects a drag.
• BaseTapAndDragGestureRecognizer becomes a sealed class.
• BaseTapAndDragGestureRecognizer declares victory immediately when the drag threshold is met (for the desired axis) or when we have won the arena for the primary pointer and the drag threshold is met on any axes.
• TapAndHorizontalDragGestureRecognizer accepts drags when movement on the x-axis passes the defined threshold.
• TapAndPanGestureRecognizer works how TapAndDragGestureRecognizer originally worked which was movement on the x and y axes.
• TapAndDragGestureRecognizer deprecated.
• Text selection on mobile platforms like iOS/Android/Fuchsia use TapAndHorizontalDragGestureRecognizer so they do not conflict with VerticalDragGestureRecognizers that may be in a Scrollable.
• Text selection on desktop platforms like Linux/macOS/Windows use TapAndPanGestureRecognizer.

Fixes #122519
Fixes #122141

Before fix

macOS	iOS	Android
Screen.Recording.2023-03-28.at.1.39.17.AM.mov	Screen.Recording.2023-03-28.at.1.40.55.AM.mov	Screen.Recording.2023-03-28.at.1.42.41.AM.mov

After fix

macOS	iOS	Android
Screen.Recording.2023-03-28.at.1.23.07.AM.mov	Screen.Recording.2023-03-28.at.1.27.11.AM.mov	Screen.Recording.2023-03-28.at.1.29.24.AM.mov

Thanks @xu-baolin for his work and investigation on #122374 which some changes are incorporated into this PR.

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• I signed the [CLA].
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is [test-exempt].
• All existing and new tests are passing.

[Renzo-Olivares]

This is ready for some initial feedback, I will still be adding more tests.

[justinmc]

Just a question about TapAndDragGestureRecognizer vs. TapAndPanGestureRecognizer to make sure I understand the big picture. Otherwise this looks good.

[justinmc]

This could be a breaking change.

Did you think about keeping this as a non-abstract class and not having TapAndPanGestureRecognizer, or is that not possible? I'm not saying you should do that instead, just wondering about the tradeoffs.

[Renzo-Olivares]

Yeah it could be a breaking change. I ran it through g3 tests and it passed all of them, and doing a github search does not show any usages of TapAndDragGestureRecognizer outside of the framework. Also TapAndDragGestureRecognizer has not yet landed in stable so this might be the best opportunity to make this change. But I agree that we shouldn't make this change unless completely necessary.

The functionality of TapAndHorizontalDragGestureRecognizer might fit all of our needs on desktop and mobile from trying it out more.

I'll try to explain further let's take TapAndHorizontalDragGestureRecognizer. Say the user does a vertical drag gesture in a GestureArena where TapAndHorizontalDragGestureRecognizer is not competing with other recognizers, then it will first check the horizontal drag threshold, since it was a vertical drag that condition will not be met, and it will check if the pointer has already been accepted with wonArenaForPrimaryPointer and if it met the threshold for a pan. In this case it will accept since the vertical drag can be considered a pan.

If it is competing with other recognizers such as VerticalDragGestureRecognizer in a Scrollable. Then it will only check if the PointerEvent moved past the horizontal drag threshold, which will never be true if a user did a vertical drag. This fits our use case of text selection in a scrollable.

tldr; horizontal drag threshold is for when the pointer has not been accepted yet, and horizontal threshold || pan threshold is for when the pointer has been accepted.

Speaking outside the realms of text selection, I wonder if someone might want to instead of having the horizontal drag threshold have a vertical drag threshold. Making this class abstract gives us the opportunity to create TapAndVerticalDragGestureRecognizer in that case without having to change the core functionality of the recognizer.

Another option if we want to minimize breakage might be adding a parameter to TapAndDragGestureRecognizer called something like TapAndDragDelegate. And this delegate provides methods like hasSufficientGlobalDistanceToAccept, getDeltaForDetails, and getPrimaryValueFromOffset. And we could set a default delegate as TapAndPanDelegate since that is how the TapAndDragGestureRecognizer worked originally.

There might also be other options like using a mixin.

class TapAndDragGestureRecognizer with _TapAndPanDelegate {
  void test() {
    bool shouldAccept = hasSufficientGlobalDistanceToAccept();
    debugPrint('$shouldAccept');
  }
}

class TapAndHorizontalDragGestureRecognizer extends TapAndDragGestureRecognizer with _TapAndHorizontalDragDelegate {
  
}

mixin _TapAndDragDelegate {
  bool hasSufficientGlobalDistanceToAccept();
  
  Offset getDeltaForDetails(Offset delta);

  double? getPrimaryValueFromOffset(Offset value);
}

mixin _TapAndPanDelegate implements _TapAndDragDelegate {
  @override
  bool hasSufficientGlobalDistanceToAccept() {
    return false;
  }
  
  @override
  Offset getDeltaForDetails(Offset delta) => delta;

  @override
  double? getPrimaryValueFromOffset(Offset value) => null;

}

mixin _TapAndHorizontalDragDelegate implements _TapAndDragDelegate {
  @override
  bool hasSufficientGlobalDistanceToAccept() {
    return true;
  }
  
  @override
  Offset getDeltaForDetails(Offset delta) => Offset(delta.dx, 0.0);

  @override
  double getPrimaryValueFromOffset(Offset value) => value.dx;
}

Let me know what you think.

[Renzo-Olivares]

Looking at the mixin implementation above I noticed by extending TapAndDragGestureRecognizer which has a default _TapAndPanDelegate, TapAndHorizontalDragGestureRecognizer technically is mixed with _TapAndPanDelegate and _TapAndHorizontalDragDelegate but they don't seem to conflict with each other.

[chunhtai]

I may have missed something, why not just have a TapAndDragGestureRecongnizerBase to be used by TapAndDragGestureRecongnizer and TapAndVerticalDragGestureRecongnizer
TapAndVHorizontalDragGestureRecongnizer

[Renzo-Olivares]

Oh that would work nicely! I did not think of that. Thank you for the suggestion.

[justinmc]

Is there anything that should also be tested for CupertinoTextField? Assuming this can't be tested on EditableText.

[Renzo-Olivares]

Yeah, I will write these same tests for CupertinoTextField just in case.

[justinmc]

Ah I missed that you said you were still adding tests above 👍

[xu-baolin]

Hi, @Renzo-Olivares I will take a look at this tomorrow :)

[xu-baolin]

should assert this in this case?:
assert(currentUp == null)

[Renzo-Olivares]

I wonder if this is still necessary since the recognizer does not wait before declaring victory when the drag threshold is met. Before, there could have been a drag detected and no victory so it was possible that the PointerUpEvent came before accepting the drag. Now, since we declare victory immediately when the drag threshold is met, I don't think a PointerUpEvent could come before. What do you think?

[xu-baolin]

If the PointerUpEvent should not come before, we add this reasonable to prevent #122519 regressing. Assertions can be help in diagnosis when problems occur.

[xu-baolin]

if (_dragState == _DragState.accepted && _start != null) should be

if (_start != null) {
 assert(_dragState == _DragState.accepted);
...
}

right?

[xu-baolin]

      } else if (_dragState == _DragState.possible) {
        if (_start == null) {
          // Only check for a drag if the start of a drag was not already identified.
          _checkDrag(event);
        }
        // This can occur when the recognizer is accepted before a [PointerMoveEvent] has been
        // received that moves the pointer a sufficient global distance to be considered a drag.
        if (_start != null) {
          _acceptDrag(_start!);
        }
      }

this should change to:

      } else if (_dragState == _DragState.possible) {
        if (_start == null) {
          // Only check for a drag if the start of a drag was not already identified.
          _checkDrag(event);
        }
        assert(_start == null)
      }

If the _checkDrag successed, _acceptDrag will called immediately

[xu-baolin]

Ohh, I understand what the case is, you are right ：）

[xu-baolin]

I am wondering why _pastSlopTolerance && _wonArenaForPrimaryPointer should accept drag?
But why _pastSlopTolerance && !_wonArenaForPrimaryPointer need reject the gesture instead of waiting the gesture win result to accetp drag?

[xu-baolin]

In other words, what should we do if _pastSlopTolerance == true and has not won the arena (will won later)?

[Renzo-Olivares]

My logic here is because _pastSlopTolerance == true the gesture cannot be considered a tap. And because we are in didStopTrackingLastPointer for the given pointer and we have still not passed the drag threshold then the gesture cannot be considered a drag so we cancel and reject.

DragGestureRecognizer has this logic as well.

flutter/packages/flutter/lib/src/gestures/monodrag.dart

Lines 458 to 460 in a28aae9

	case _DragState.possible:
	resolve(GestureDisposition.rejected);
	_checkCancel();

.

[xu-baolin]

Got it.

[xu-baolin]

resolve(GestureDisposition.accepted);

can change to

if (!_wonArenaForPrimaryPointer ) {
  resolve(GestureDisposition.accepted);
}

[xu-baolin]

If we already won the arena, we should not declear victory again.

[Renzo-Olivares]

Will make this change.

[chunhtai]

How does this translate to widget tree of RawGestureDetector? i.e. if I nested TapAndDragGestureRecognizer which one would win

[Renzo-Olivares]

If you nested them and the pointer has moved past the drag threshold (considered a drag), then the first TapAndDragGestureRecognizer instance to receive the PointerEvent will win the arena because it declares victory immediately. I think the first one to receive the event is the child since hit testing propagates from inner -> outer from my understanding.

If it has not moved past the drag threshold (considered a tap), then the first recognizer to enter the arena will win (i.e. they both tie and the gesture arena will sweep so first member of arena wins).

This is inline with TapGestureRecognizer and DragGestureRecognizer.

[chunhtai]

Sounds good, can you add that into the doc? I think it would be clearer than the current doc

[Renzo-Olivares]

Sure thing!

[chunhtai]

This should be public, otherwise this abstract class can't be used outside of this file, you might as well make the abstract class private

[chunhtai]

Also these seems like a vary limited API, I would probably suggest toward making _checkDrag more customizable if this class meant to be general purposes.

If the TapAndDragGestureRecognizer meant to be private, then I think this is fine.

[Renzo-Olivares]

I'm a little on the fence about this since DragGestureRecognizer has these same methods private and I would prefer to keep them private unless there is an immediate use case for it.

My goal for this PR is mostly to fix the scrolling issue in TextField.
I can always expose these methods in a later PR if there is a need for it.

I think keeping BaseTapAndDragGestureRecognizer public is fine, since it doesn't make it any more or less extensible than TapAndDragGestureRecognizer initially was. What do you think?

[chunhtai]

Anyone would try to extends it outside this file would crash. The current implementation will create a foot gun

I think there are four ways to do this

1. Make these methods public
2. Make this class private
3. Turns constructor into private constructor (so no one can override)
4. give these method default implementations

[Renzo-Olivares]

Thank you for further clarifying the problem that this causes and your suggestions! Maybe another option is also to make BaseTapAndDragGestureRecognizer a sealed class.

class TapAndDragTestExtension extends BaseTapAndDragGestureRecognizer {

}

Trying this outside of the file results in the following error: The class 'BaseTapAndDragGestureRecognizer' can't be extended, implemented, or mixed in outside of its library because it's a sealed class.dart[invalid_use_of_type_outside_library](https://dart.dev/diagnostics/invalid_use_of_type_outside_library).

This should avoid the foot gun and also allow us to keep the documentation on BaseTapAndDragGestureRecognizer.

[chunhtai]

I may have missed something, why not just have a TapAndDragGestureRecongnizerBase to be used by TapAndDragGestureRecongnizer and TapAndVerticalDragGestureRecongnizer
TapAndVHorizontalDragGestureRecongnizer

[justinmc]

I confirmed that this fixes #102084, and I added it to the "fixes" list in the description of this PR above.

[Renzo-Olivares]

Added tests.

[chunhtai]

LGTM