Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SuperTextField] - Right-click and other gesture overrides (Resolves #278) #692

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

matthew-carroll
Copy link
Contributor

@matthew-carroll matthew-carroll commented Jul 9, 2022

[SuperTextField] - Right-click and other gesture overrides (Resolves #278)

Currently, SuperTextField offer a onRightClick callback. This callback has some issues, as discussed in #278. Also, there are other gestures that apps needs to support, e.g., CTRL + Left Click to open a context menu.

I investigated three approaches to this problem.

The approach that I chose for this PR is to allow apps to includes their own gesture widget in the middle of the SuperTextField subtree. This gives developers full control over both the desired recognizer, as well as the widgets that are used to add that recognizer to the tree.


I rejected the following approaches:

Take in callback overrides for every gesture that SuperTextField handles. This approach worked. In fact, it worked a bit better than the one that I chose (more on that below). However, this approach had extensibility problems. Any gesture that we added to SuperTextField in the future would need to another optional override callback. Any gesture that we remove from SuperTextField would lead to a breaking change when we remove its corresponding override callback. This approach also added a lot of verbosity with a large set of override callbacks. It seemed like a maintenance nightmare.

Take in a GestureRecognizer that we add to the SuperTextField RawGestureDetector. This approach is almost equivalent to the solution that's in this PR, except that it wouldn't give app's control over widgets. On the one hand, this approach would prevent apps from inadvertently breaking the SuperTextField widget tree. On the other hand, this approach would make it more difficult to track information related to gestures. For example, the solution in this PR allows for an app to pass a StatefulWidget to handle gestures, which includes the ability to track information. But a GestureRecognizerFactory doesn't include any long-lived pieces, so apps would need to stick gesture information somewhere else.


The problem with this PR: There's still an outstanding problem in this PR that we need to solve. Currently, overridden gestures are handled by the override detector AND the SuperTextField detector. For some reason, the overriding GestureDetector is not consuming the gestures before they get to the standard RawGestureDetector. This is probably due to issues with our custom multi-tap gesture detector. This is a problem because when the user Control + Left Taps to open a context menu, it causes the selection in the text field to change. We need to fix this before merging.

@matthew-carroll matthew-carroll requested review from miguelcmedeiros and angelosilvestre and removed request for miguelcmedeiros July 9, 2022 22:27
@matthew-carroll
Copy link
Contributor Author

@venkatd @knopp - Please take a look at the approach in this PR to handle gestures to open context menus, etc.

@knopp - There's one outstanding problem with this PR, which is described up above. Do you happen to know enough about the gesture system to figure out where that problem is coming from, and how we might fix it?

@miguelcmedeiros miguelcmedeiros requested review from knopp and removed request for miguelcmedeiros July 10, 2022 23:09
Copy link
Collaborator

@angelosilvestre angelosilvestre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@knopp
Copy link
Contributor

knopp commented Jul 13, 2022

I don't see how this is currently fixable. TapSequenceGestureRecognizer reports onTap immediately when receiving pointer event, while for BaseTapGestureRecognizer 100ms deadline has to pass. I don't know if there's an easy way around this that wouldn't require changes to TapSequenceGestureRecognizer and possibly introducing additional lag. Given these restrictions I think the first approach might be better. Or at least a combination that would let us prevent executing default handler (i.e. when ctrl is pressed).

@matthew-carroll
Copy link
Contributor Author

@knopp are you familiar with the gesture arena API? For example, do you know how hold on a pointer is intended to work? I have to believe there's a reasonable way to make this work.

For example, in our custom recognizer, if we see that no one else is interested in the gesture, then trigger callbacks immediately. If, on the other hand, it looks like someone else is interested in the gesture, then introduce some kind of delay, or some kind of negotiation to figure out which one should win.

The problem is, I have no idea how the recognizers and handlers are supposed to work. The docs are too high level and ambiguous, and the implementations span about 4 level so subclassing, so it's tough to know which behaviors are fundamental rules, and which behaviors were added to make other things easier.

@knopp
Copy link
Contributor

knopp commented Jul 14, 2022

I think the way gestures are resolved in Flutter might be too simplistic for this usecase unfortunately. Consider the basic example:

 GestureDetector(
  behavior: HitTestBehavior.opaque,
  onTapDown: (details) {
    print("Tap Down 1");
  },
  child: GestureDetector(
    behavior: HitTestBehavior.opaque,
    onTapDown: (details) {
      print("Tap Down 2");
    },
    child: const Text(
      'You have pushed the button this many times:',
    ),
  ),
),

If you tap the label which text will be printed? The answer unfortunately depends on how fast you'll tap on the label. If you tap quickly (less than 100ms) the gesture arena will resolve to the first recognizer on PointerUpEvent, which will trigger the innermost onTapDown callback.

If the tap down is over 100ms, both recognizer will exceed the 100ms deadline and both will fire the onTapDown callback. So even in a trivial example like this you can't ensure that the inner tap down always prevail.

On top of that, there doesn't seem to be any way to determine if there are other gesture arena members already in the arena for current pointer. The members access is not exposed anywhere. So it seems like to coordinate something like this the user would need to be using custom recognizer that allows for more coordination.

I always felt like the gesture arena in Flutter is overly simplistic compare to iOS for example, where each recognizer can have a delegate that can be used to coordinate multiple recognizer with much higher flexibility.

@matthew-carroll
Copy link
Contributor Author

I filed flutter/flutter#107671 with Flutter to see if we can get any clarity/help on this. I'd really like to avoid the first option that I tried because it was a mess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SuperTextField onRightClick should be invoked on tap down
3 participants