Add TextField triple tap/click gestures - #119046

Perhaps remove the doc comment reference to [int] as it's a generic type? Can follow the style of TextBoundary (or even CharacterBoundary which only has class level doc comments...).

I guess these magic numbers could be avoided with constants (also needed in isLineTerminator).

Same as above on constants.

Looking at TextPainter, the list of what's considered a newline is different: no 0xD on its own, plus 0x2028/0x2029.

  static bool _isNewline(int codePoint) {
    switch (codePoint) {
      case 0x000A:
      case 0x0085:
      case 0x000B:
      case 0x000C:
      case 0x2028:
      case 0x2029:
        return true;
      default:
        return false;
    }
  }

Hmm I got the list from here https://www.unicode.org/standard/reports/tr13/tr13-5.html. I added 0x2028 and 0x2029 I had left out.

Why do iOS and Mac fall through to the same behavior as Windows if the comments above seem to describe their behavior differently?

To me it looked like iOS/macOS/Windows increased their tap count infinitely but they handled taps past 3 differently.

iOS and macOS don't handle anything past a third tap. The selection is retained at the paragraph that was selected on the triple tap.

Windows alternates between double click and triple click when going past the third tap. Alternating between selecting the word or paragraph at the position.

Let me know if I can clarify this further.

I think it's ok as-is. I looked through the code to see how consecutiveTapCount is used and it makes sense that these all return null but later have different behavior.

I think it makes more sense now after introducing an effectiveConsecutiveTapCount.

Ah I guess some tests that previously tapped very soon after a double tap might behave differently now. We should watch out for this breaking Google tests or anything else. Though it looks like Google tests are currently passing 👍

Yeah, how it worked previously is that the double tap timer on TextSelectionGestureDetector would reset the count to 0 after every double tap for every platform. So some test behavior changed a bit, but google tests look good so far.

Is your plan to do this in another PR?

I'm hoping that I can fix it in this one. But if the fix becomes overly complex then I'll probably make it into another PR.

Just wanted to draw attention to this again in case you forgot about it.

It's a bummer that we have to duplicate all of these tests for Cupertino and Material, but I guess they won't work on EditableText because of the code changes in text_selection.dart.

Someday we need to make it easier to share these kinds of tests!

Agreed, it would definitely help with velocity if we only had to write these tests once.

It's nice that this test still works (with minor changes) and makes a lot of sense with the addition of triple tap.

nit: maybe it's easier to write the if clauses, if the local variable is for "effective tap count".

or maybe move the logic to the static method you created later in this file that turns the raw tap count to the effective tap count.

Sorry i'm not sure I follow, can you elaborate? The static method I created later in the file is meant to tell the gesture recognizer what its ceiling is for the consecutive tap count before it resets it to 0.

is it possible to repurpose _getDefaultMaxConsecutiveTap so it takes the raw consecutive tap count and returns the "effective" tap count? For instance for windows the conversion would look like input < 2 ? input : 2 + input % 2 and for other platforms it's max(input, 3).

In this scenario if we are repurposing _getDefaultMaxConsecutiveTap are we then setting the ceiling for consecutive tap count on the recognizer to null for all platforms?

In that case I think we can do this.

  static int _getEffectiveConsecutiveTapCount(int rawCount) {
    switch(defaultTargetPlatform) {
      case TargetPlatform.android:
      case TargetPlatform.fuchsia:
      case TargetPlatform.linux:
        return rawCount <= 3 ? rawCount : 0 + (rawCount % 3 == 0 ? 3 : rawCount % 3);
      case TargetPlatform.iOS:
      case TargetPlatform.macOS:
        return math.min(rawCount, 3);
      case TargetPlatform.windows:
        return rawCount < 2 ? rawCount : 2 + rawCount % 2;
    }
  }

Definitely makes the if cases much better! Thanks for the code and suggestion!

If selectParagraph API simply calls selectParagraphsInRange API then I think we should have just one method exposed doing both, unless one of them does something specific using a private field like selectWord / selectWordsInRange in editable.dart.

Also, shouldn't this be in editable.dart together with other select* methods?

I think I want to avoid adding more methods to RenderEditable that mutate the selection, and instead have it provide selection calculations that EditableText uses to mutate its selection.

For design clarity they should all be in a single place I think. It makes sense to move the selection modifying methods to EditableText but it looks like they were in RenderEditable for years so it'd be a breaking change. Wondering if @LongCatIsLooong or @justinmc may have some thoughts?

I really like the idea of separating the calculation of text related stuff from its mutation. I hope we can get to where we have a clear place for each. For example, if some Flutter dev wants to create a button that selects the nearest 3 paragraphs or something, it would be nice to be able to give them access to our paragraph selection logic without making them actually make a change to an EditableText.

For now though it looks like Renzo has made selectParagraphsInRange and selectParagraph private and moved them to text_selection.dart. That's definitely fine for now since we can always change it later.

Nit: maybe a bit easier to understand like this:

    final TextSelection newSelection = isFromParagraphBeforeToParagraph
        ? TextSelection(baseOffset: fromRange.start, extentOffset: toRange.end),
        : TextSelection(baseOffset: fromRange.end, extentOffset: toRange.start);

Also, wondering if we may ever get weird cases here e.g. both ranges starting at the same offset but with different lengths, overlapping ranges, empty ranges etc.

Would it make sense to put the logic in _getEffectiveConsecutiveTapCount on the Details classes directly somehow? Looking at these calls, I'm thinking that it would be easy for someone to introduce a bug in the future by forgetting about _getEffectiveConsecutiveTapCount and using consecutiveTapCount directly.

However, if it doesn't make sense for the details to be concerned about this kind of stuff, then I agree with how it is now.

I can add some comments about this but I think the details classes are platform agnostic at the moment and this particular use of _getEffectiveConsecutiveTapCount is only for text selection gestures.

Ok that sounds good 👍

Just wanted to draw attention to this again in case you forgot about it.

indentation nit: tab == 2 spaces I think?

Is this kind of ternary supposed to be indented 2 or 4 spaces? I think I've done both on different occasions because I wasn't sure...

I'm actually not too sure, but I just followed the convention used in this file which seems to be 4 spaces.

What do you mean by this line?

Similar explanation here: #119046 (comment)

Ah sorry.

nit: !delegate.selectionEnabled to avoid nested ifs (so readers don't have to find the closing bracket).

Do touch devices work on these platforms?

I'm not aware of any touch devices on macOS, but there definitely is on Windows.

Can you remove the 0 + ....? Or maybe 1 + (rawCount - 1) % 3?

I think I can remove this 0, I'll check on it.

Is that because the selection control has a tap recognizer? Can you triple tap recognizer declare victory immediately?

It could but the recognizer never receives the third tap because the selection handles overlay absorb the pointer immediately when they pop up.

absorb the pointer immediately

Do you mean the text field's hitTest method won't be called, or it will be called, but the selection handle will declare victory immediately in the arena?

I mean it won't be called, so TextSelectionGestureDetector does not get to participate in the arena at all.

Were you able to fix this? hitTest should be called for the selection gesture recognizers right?

Nit: Maybe _selectionOverlay should call rebuildHandles itself when handlesAllowPointers changes? I'm thinking about how to best architect this to avoid problems like forgetting to call rebuild in the future.

That makes sense to me since this method won't really work without a rebuild.

"does" => "do" I think.