Fix text selection toolbar appearing under obstructions - #29809

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

@justinmc could you take a first pass at this?

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jiisd Check out the failures under the "analyze" check when you get a chance. There are some small linter failures to fix.

@jiisd Check out the failures under the "analyze" check when you get a chance. There are some small linter failures to fix.

🙂 already fixed.

Thank you!

One more thing that I forgot to mention, sorry. This will definitely need tests.

There are similar tests for the selection menu in https://github.com/flutter/flutter/blob/master/packages/flutter/test/material/text_field_test.dart#L887. You can reuse some of that code to get the selection menu to appear in your test. Then get its location with something like tester.getTopLeft(find.text('SELECT ALL')) and expect it to be above the TextField when it's not near the top of the screen and below the TextField when it is near the top of the screen.

CC @xster @LongCatIsLooong we should check the native iOS behavior, and then pull down this branch and compare.

Thank you!

One more thing that I forgot to mention, sorry. This will definitely need tests.

There are similar tests for the selection menu in https://github.com/flutter/flutter/blob/master/packages/flutter/test/material/text_field_test.dart#L887. You can reuse some of that code to get the selection menu to appear in your test. Then get its location with something like tester.getTopLeft(find.text('SELECT ALL')) and expect it to be above the TextField when it's not near the top of the screen and below the TextField when it is near the top of the screen.

Thanks for your reminder!

The corresponding regression test logic has been added(this commit).

➜ flutter git:(fix/issues_29808) ✗ flutter test /Users/yaheng/.flutter_lib/packages/flutter/test/material/text_field_test.dart
00:10 +130: All tests passed!

@jiisd The test looks good, thanks. I confirmed that it fails without this fix.

How do you feel about increasing the distance from the input to the menu in Android so that the selection handles are visible? It looks like native Android positions the menu so that the handles are visible.

Android (native):

iOS looks fine I think, except problems unrelated to this PR like the position of the arrow.

iOS (native):

iOS (Flutter, this branch):

@justinmc Yep, on android, I think your idea of increasing distance is very appropriate and can make the user experience better.
On iOS, I have tried to make the arrow to adjust the left and right position dynamically before, but I haven't found an elegant solution. If I progress, I will submit the relevant PR again.

Thanks. This is the issue about the arrow position FYI: #22576

The iOS arrow can be done in a separate PR (if you'd like to keep sending us PR :D)

It's tracked in #11274

@xster Thanks!

[Breaking Change] The public interface of the abstract class TextSelectionControls

Description

When repairing the problem of #29808, it found that the interface is no longer sufficient. In order to get a more accurate anchor point for the toolbar, some parameters are added to the relevant interface. You can get more details form #29809.

The path of the change class lib/src/widgets/text_selection.dart line:90.

Previous interface definition:

  /// Builds a toolbar near a text selection.
  ///
  /// Typically displays buttons for copying and pasting text.
  Widget buildToolbar(BuildContext context, Rect globalEditableRegion, Offset position, TextSelectionDelegate delegate);

Current interface definition:

  /// Builds a toolbar near a text selection.
  ///
  /// Typically displays buttons for copying and pasting text.
  ///
  /// [globalEditableRegion] is the TextField size of the global coordinate system
  /// in logical pixels.
  ///
  /// The [position] is a general calculation midpoint parameter of the toolbar.
  /// If you want more detailed position information, can use [endpoints]
  /// to calculate it.
  Widget buildToolbar(
    BuildContext context,
    Rect globalEditableRegion,
    Offset position,
    List<TextSelectionPoint> endpoints,
    TextSelectionDelegate delegate,
  );

How to update

If you have already used this interface, you should update it to the latest version and add the endpoints parameter.
We keep the position parameter because we think it's probably already used in subclasses.

@xster Any idea on the test failure? It seems unrelated. Something about gradle.

The failure went green after a rerun. I think this is ready to merge.

Yup, LGTM

I copy edited the message a bit. @jiisd if you're ok with the edit, I'll send it out and give the community a day to give feedback. Thanks again for the fix.

[Breaking Change] TextSelectionControls.buildToolbar signature adds another 'endpoint' argument

If you don't implement your own TextSelectionControls subclass, you can stop reading.

Description

While fixing the problem of #29808, it was found that the TextSelectionControls.buildToolbar interface is no longer sufficient to adequately replicate the native behavior. In order to get a more accurate anchor point for the text selection toolbar, the selection's start and end positions are added to the function signature. You can get more details form #29809.

In lib/src/widgets/text_selection.dart:

Previous interface definition

  /// Builds a toolbar near a text selection.
  ///
  /// Typically displays buttons for copying and pasting text.
  Widget buildToolbar(
    BuildContext context, 
    Rect globalEditableRegion, 
    Offset position, 
    TextSelectionDelegate delegate,
  );

becomes

  /// Builds a toolbar near a text selection.
  ///
  /// Typically displays buttons for copying and pasting text.
  ///
  /// [globalEditableRegion] is the TextField size of the global coordinate system
  /// in logical pixels.
  ///
  /// The [position] is a general calculation midpoint parameter of the toolbar.
  /// If you want more detailed position information, can use [endpoints]
  /// to calculate it.
  Widget buildToolbar(
    BuildContext context,
    Rect globalEditableRegion,
    Offset position,
    List<TextSelectionPoint> endpoints,
    TextSelectionDelegate delegate,
  );

Specifically List<TextSelectionPoint> endpoints was added.

How to update

If you currently implement this interface, you should update its signature to the latest version and add the endpoints parameter.
For backward compatibility, the position parameter (which is the midpoint between the 2 endpoint positions) was preserved.

@xster Awesome thanks!

Actually on second thought, I sent you an invite to join our flutter-announce google group (to your 163.com). You can send the breaking change announcement yourself once you accept the invitation.

@xster The email from 163.com was rejected. Can you invite another account(gmail.com)?
Thanks!

Thanks, sent

Still failing, is the target email address of [email protected] incorrectly?

Sorry, one of the permission settings wasn't set correctly. Your gmail address should have posting rights now.

It works!
Thanks.

Since we’re breaking the API, is it better to make parameters into a Details object (as in here) so that we can add more information in the future without breaking the code again and without having too many parameters?

I think it's mostly ok. The gesture events is a highly public API used by callback implementers everywhere.

Since we might change the arguments in any signature in the future, the argument can be generally applied to all functions having a single data class as argument so it's non-breaking-modifiable :D

So we'd have to draw the line somewhere and for subclass overrides, I think it's ok.

Thanks so much for fixing this @jiisd!

we use Flutter 1.7.8+hotfix.4, still have this issue.

@FrankYan09 could you create a new issue here https://github.com/flutter/flutter/issues/new/choose, preferably with screenshots and code to reproduce the issue so we would be able to identify the problem quickly?