[TextSelectionControls] move the tap gesture callback into _TextSelectionHandlePainter, remove `_TransparentTapGestureRecognizer`. by LongCatIsLooong · Pull Request #83639 · flutter/flutter

LongCatIsLooong · 2021-05-31T00:22:59Z

For iOS/Android behaviors see #39376 (comment)

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

chunhtai · 2021-06-04T17:11:22Z

    await tester.tap(find.byKey(icon));
    await tester.pump();
    expect(controller.text, '');
-    expect(controller.selection, const TextSelection.collapsed(offset: 0, affinity: TextAffinity.upstream));


Why this change?

I think before the change the tap event actually altered the selection. Now it's only hitting the button and not the text field so that should be expected.

chunhtai · 2021-06-04T17:11:34Z

+      MaterialApp(
+        home: Scaffold(
+          body: TextField(
+            //focusNode: focusNode,


chunhtai · 2021-06-04T17:19:07Z

-  ///
-  /// Both regular taps and long presses invoke this callback, but a drag
-  /// gesture won't.
+  /// A callback that's optionally invoked when a selection handle is tapped.


Suggested change

/// A callback that's optionally invoked when a selection handle is tapped.

/// A callback that's optionally invoked based on platforms when a selection handle is tapped.

I think ultimately this depends on what kind of selection control you're using, so it's not really platform-dependant. Even in TextField's implementation you can override textSelectionControl so it's still up to the developer.

Anyways IMO we should deprecate this callback and let the TextSelectionControl implementation decides how the handles should be built and what interactions it should support. Currently it does not have enough information (as it doesn't have access to the EditableTextState that builds the selection overlay).

chunhtai · 2021-06-04T17:20:19Z

+  /// large for tapping (as it's not meant to be tapped) so it does not call
+  /// [onSelectionHandleTapped] even when tapped.
  /// {@endtemplate}
+  // See https://github.com/flutter/flutter/issues/39376#issuecomment-848406415


I think we avoid pointing to a github link in API doc

The last 2 lines are prefixed by "//".

chunhtai · 2021-06-04T17:22:02Z

  /// The top left corner of this widget is positioned at the bottom of the
  /// selection position.
-  Widget buildHandle(BuildContext context, TextSelectionHandleType type, double textLineHeight);
+  Widget buildHandle(BuildContext context, TextSelectionHandleType type, double textLineHeight, VoidCallback? onTap);


document what onTap is

chunhtai · 2021-06-04T17:25:40Z

                bottom: padding.bottom,
              ),
-              child: widget.selectionControls!.buildHandle(
+              child: widget.selectionControls.buildHandle(


is it possible we wraps the widget from the buildhandle with the gesturedetector here? It feels weird that we ask the widget to build the gestureDetector.

It's the selection control of the user's choice that builds the handle right? The selection control decides the style of the handles and the types of interactions it suppoorts.

It's a bit subtle that onTap should only work on the "knob" , while drag gestures should work on both the knob and the rectangle attached to the knob.

That is an unfortunate how onSelectionHandleTapped behave differently based on the implementation of TextSelectionControls.

I am ok with this approach if we can deprecate the onSelectionHandleTapped at some point....

chunhtai

LGTM

chunhtai · 2021-06-04T19:18:56Z

                bottom: padding.bottom,
              ),
-              child: widget.selectionControls!.buildHandle(
+              child: widget.selectionControls.buildHandle(


That is an unfortunate how onSelectionHandleTapped behave differently based on the implementation of TextSelectionControls.

I am ok with this approach if we can deprecate the onSelectionHandleTapped at some point....

creativecreatorormaybenot · 2021-06-17T10:13:40Z

-  Widget buildHandle(BuildContext context, TextSelectionHandleType type, double textLineHeight);
+  ///
+  /// The supplied [onTap] should be invoked when the handle is tapped, if such
+  /// interaction is allowed. As a counterexample, the default selection handle
+  /// on iOS [cupertinoTextSelectionControls] does not call [onTap] at all,
+  /// since its handles are not meant to be tapped.
+  Widget buildHandle(BuildContext context, TextSelectionHandleType type, double textLineHeight, VoidCallback? onTap);


@LongCatIsLooong This is a breaking change 😓 How are we supposed to deal with packages that should work with multiple versions of Flutter that call buildHandle.

https://pub.dev/packages/flutter_math_fork is such a package and now it cannot work.

The change should have been [VoidCallback? onTap], otherwise it is breaking. Should I make a PR and we cherry pick a hot fix for the dev channel?

So beta and stable have 3 positional parameters and dev has 4 positional parameters...

Yeah the proposed change sounds good to me. Sorry for the breakage!

Per the breaking change policy this is not a breaking change so I'd recommend contributing tests to flutter/tests to prevent future breakage.
I'm not sure we have a process for cherry-picking a commit to the dev channel. See https://github.com/flutter/flutter/wiki/Flutter-Cherrypick-Process. Maybe we should add that dev build to https://github.com/flutter/flutter/wiki/Bad-Builds.

creativecreatorormaybenot · 2021-06-17T10:35:41Z

@LongCatIsLooong This is how I tried to fix it: simpleclub/flutter_math@7c13f1e#diff-0cee011dc67be19dc0c38a0f8a915554273b9f7f1b4c38402db2112d16a7d596R175

justinmc · 2021-06-23T23:07:17Z

Thanks for fixing this @LongCatIsLooong! Good straightforward idea. This bug has been coming up over and over for 2 years.

LongCatIsLooong added 4 commits May 29, 2021 22:18

wip

de94773

fix buildHandle signature

d04011e

add tests

172b100

update docs

d87b81b

flutter-dashboard Bot added p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels May 31, 2021

google-cla Bot added the cla: yes label May 31, 2021

LongCatIsLooong changed the title ~~Deprecate handle tap callback~~ [TextSelectionControls] move the tap gesture callback into _TextSelectionHandlePainter, remove _TransparentTapGestureRecognizer. May 31, 2021

remove unused code comments

01375b2

LongCatIsLooong requested a review from chunhtai June 4, 2021 04:25

chunhtai reviewed Jun 4, 2021

View reviewed changes

review

81c2cbd

chunhtai approved these changes Jun 4, 2021

View reviewed changes

LongCatIsLooong added the waiting for tree to go green label Jun 8, 2021

fluttergithubbot merged commit 21273fd into flutter:master Jun 8, 2021

LongCatIsLooong deleted the deprecate-handle-tap-callback branch June 8, 2021 02:21

creativecreatorormaybenot reviewed Jun 17, 2021

View reviewed changes

creativecreatorormaybenot mentioned this pull request Jun 17, 2021

Make buildHandle onTap optional #84806

Merged

5 of 8 tasks

	/// A callback that's optionally invoked when a selection handle is tapped.
	/// A callback that's optionally invoked based on platforms when a selection handle is tapped.

Conversation

LongCatIsLooong commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

creativecreatorormaybenot commented Jun 17, 2021

Uh oh!

justinmc commented Jun 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LongCatIsLooong commented May 31, 2021 •

edited

Loading