Text Selection Overflow (Android) #49391

justinmc · 2020-01-23T23:53:55Z

Description

The text selection menu overflows in some fairly common cases. This PR will fix it by handling it the same way that native does.

This PR is only for Android, and we'll need to do a similar approach for iOS.

Related Issues

#35826
Closes #50935

Tests

In material/text_selection_test.dart, I tested various overflow cases and the position of the menu above and below the anchor. I also noticed an edge case in showing the menu when a large block of text was selected, and I wrote a test for that too.

Breaking change

This is a breaking change, see the design doc.

fluttergithubbot · 2020-01-23T23:53:58Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

… to two pumps

screen.

fluttergithubbot · 2020-03-09T16:50:17Z

Golden file changes remain available for triage from new commit, Click here to view.

Changes reported for pull request #49391 at sha 5340d2128b82fa100165da8b26ff6e0773a51783

…ew key

fluttergithubbot · 2020-03-09T18:15:17Z

Golden file changes remain available for triage from new commit, Click here to view.

Changes reported for pull request #49391 at sha bd7f9b4

justinmc · 2020-03-09T20:29:29Z

Design doc

Breaking change email:

Hello Flutter,

If your app doesn't involve selectable or editable text, you can stop reading now.

I'm modernizing a few things about the text selection menu, and if you use any visual diff tests with the text selection menu open, you may encounter failures. See the full design doc.

Summary of Changes
The text selection menu has a positioning bug when selecting multiline text. The position will be changed to match native.

The visual appearance of Android's menu will be updated slightly to match the latest versions of Android.

In the case that the menu overflows, such as on narrow devices or locales with long menu items, the menu will place overflowing items in an overflow menu in line with the native platform on iOS and Android.

Migration Guide
No code changes are needed. If you have affected visual diff tests, update the tests to recognize the new visuals.

Thanks! Please see the design doc for more information and to leave feedback.

Justin (@justinmc)

fluttergithubbot · 2020-03-09T23:25:15Z

Golden file changes remain available for triage from new commit, Click here to view.

Changes reported for pull request #49391 at sha 2f0327b

goderbauer

LGTM after i18n issue is resolved.

goderbauer · 2020-03-10T18:04:49Z

packages/flutter/lib/src/material/text_selection.dart

+      || ((widget.handleSelectAll == null) != (oldWidget.handleSelectAll == null))) {
+      // Change _TextSelectionToolbarContainer's key when the menu changes in
+      // order to cause it to rebuild. This lets it recalculate its
+      // saved width for the new set of children.


... and also avoids animating any changes in size (or something like that)?

goderbauer · 2020-03-10T18:05:47Z

packages/flutter/lib/src/material/text_selection.dart

+      // saved width for the new set of children.
+      _containerKey = UniqueKey();
+      // If the menu items change, make sure the overflow menu is closed. This
+      // prevents an empty overflow menu and an incorrect saved width.


"the incorrect saved width" part is now handled by the key above. Maybe remove that here?

goderbauer · 2020-03-10T18:07:20Z

packages/flutter/lib/src/material/text_selection.dart

+                      _overflowOpen = !_overflowOpen;
+                    });
+                  },
+                  tooltip: _overflowOpen ? 'Back' : 'More',


These strings need to be internationalized.

goderbauer · 2020-03-10T18:08:32Z

packages/flutter/lib/src/material/text_selection.dart

+
+    // Save the width when the menu is closed. If the menu changes, this width
+    // is invalid, so it's important that this RenderBox be recreated in that
+    // case.


Maybe mention that we achieve this by providing a new key to the corresponding widget.

goderbauer · 2020-03-10T18:10:03Z

packages/flutter/lib/src/material/text_selection.dart

+}
+
+class _ToolbarParentData extends ContainerBoxParentData<RenderBox> {
+  // Whether or not this child is painted.


nit: use /// instead of // (this is a doc-quality comment).

goderbauer · 2020-03-10T18:10:59Z

packages/flutter/lib/src/material/text_selection.dart

+  }
+}
+
+class _TextSelectionToolbarItemsRenderBox extends RenderBox with ContainerRenderObjectMixin<RenderBox, _ToolbarParentData>, RenderBoxContainerDefaultsMixin<RenderBox, _ToolbarParentData> {


Maybe I missed it, but do you actually use anything from RenderBoxContainerDefaultsMixin?

Nope not anymore it looks like, good catch!

goderbauer · 2020-03-10T18:19:40Z

packages/flutter/lib/src/material/text_selection.dart

+        nextSize = Size(nextSize.width + navButton.size.width, nextSize.height);
+      }
+    } else {
+      navButtonParentData.shouldPaint = false;


Didn't you already do this above in line 435? Maybe change line 435 to just be:

if (renderObjectChild == firstChild) { // Deal with the navigation button later. return; } ... and then figure out what to do with the nav button right here.

Will do. I agree this way is misleading now that I look at it.

goderbauer · 2020-03-10T18:20:10Z

packages/flutter/lib/src/material/text_selection.dart

+
+      // The navigation button is placed after iterating all children, and there
+      // is no need to place children that won't be painted.
+      if (renderObjectChild == firstChild


compare this to navButton instead of firstChild to make the code slightly more readable?

goderbauer · 2020-03-10T18:25:39Z

packages/flutter/lib/src/material/text_selection.dart

+            handleCopy: canCopy(delegate) ? () => handleCopy(delegate) : null,
+            handlePaste: canPaste(delegate) ? () => handlePaste(delegate) : null,
+            handleSelectAll: canSelectAll(delegate) ? () => handleSelectAll(delegate) : null,
+            isAbove: fitsAbove,


How is fitsAbove updated when the textfield is in a scrollable and I scroll while the toolbar is shown? Is that even possible? What does native do?

I noticed a couple of problems while looking into this. I'm going to create two new issues and defer them until after this PR, if that's alright with you. I checked and the behavior was the same before this PR.

✅ Scrolling a multiline text field properly updates fitsAbove immediately. The position of the menu changes as well as the orientation of the back button.

❌ Scrolling the entire TextField as an item in a ListView does not update fitsAbove. In a ListView, the text selection menu doesn't update its orientation during scrolling. #52426

❌ On native Android, the menu fades out during scrolling and back in afterwards, but Flutter doesn't do this. Text selection menu should fade during scroll #52425

goderbauer · 2020-03-10T18:26:21Z

Looks like cirrus is unhappy, though.

chunhtai · 2020-03-10T18:37:36Z

packages/flutter/lib/src/material/text_selection.dart

+    return FlatButton(
+      child: Text(label),
+      onPressed: () {
+        setState(() {


Is there a use case that we want to close the overflow when we select a button? If i remember correctly, the callback will remove the overlay entry anyway?

You're right, the overlay entry appears to be closed in all current cases. Also maybe in the future when we've got more/custom selection menu buttons, users will want to handle that themselves. I'll remove this setState.

skia-gold · 2020-03-10T22:07:21Z

Gold has detected one or more untriaged digests on patchset 28.
Please triage them at https://flutter-gold.skia.org/search?issue=49391.

skia-gold · 2020-03-10T22:09:21Z

Gold has detected one or more untriaged digests on patchset 28.
Please triage them at https://flutter-gold.skia.org/search?issue=49391.

fluttergithubbot · 2020-03-10T22:51:15Z

Golden file changes remain available for triage from new commit, Click here to view.

Changes reported for pull request #49391 at sha 1f681e1b2359d2cbb6bfe4cecdbf544d958ceb5c

skia-gold · 2020-03-11T17:03:21Z

Gold has detected one or more untriaged digests on patchset 28.
Please triage them at https://flutter-gold.skia.org/search?issue=49391.

justinmc added 3 commits January 23, 2020 11:19

A menu that displays only the items that fit.

007942c

Overflow menu

88f4fb5

Fix bug when hot reloading

c180f98

justinmc added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Jan 23, 2020

justinmc self-assigned this Jan 23, 2020

fluttergithubbot added the f: material design flutter/packages/flutter/material repository. label Jan 23, 2020

googlebot added the cla: yes label Jan 23, 2020

justinmc mentioned this pull request Jan 23, 2020

Fixed render overflow error and implemented native solution of text s… #39624

Closed

9 tasks

justinmc added 4 commits January 24, 2020 08:58

Overflow menu properly sized and aligned when overflowing and not.

fb40a39

Code deduplicated and cleaned up a bit

d0a73b8

Analyzer fixes

a59349d

Refactor code for reuse

4d245cd

justinmc force-pushed the selection-overflow branch from 27b127b to 4d245cd Compare January 28, 2020 21:00

justinmc added 9 commits January 28, 2020 13:51

Bug fixed where menu with 2 items => select all => overflow

f9a11d7

Fix analyze problem

1bf8424

WIP test

fd6de18

Test the behavior of the overflow menu, and fix tests that failed due…

4f014a8

… to two pumps

Organize test utils into a separate file per analyzer

33915f9

Correct imports in text test utils

e717f37

Test the case where only two buttons can be shown.

a20e468

A test for rendering below the text, skipped for now

eceeb33

Merge branch 'master' into selection-overflow

8159345

justinmc force-pushed the selection-overflow branch from d9ee2af to 8159345 Compare February 3, 2020 17:44

justinmc mentioned this pull request Feb 3, 2020

Copy/paste toolbar needs a "more" menu #3944

Closed

Overflow menu can render under the anchor, but bugs with hanging off

5367704

screen.

justinmc force-pushed the selection-overflow branch from 2b785ec to 5367704 Compare February 4, 2020 22:53

Overflow menu positions correctly in all cases I see

e53e7be

justinmc mentioned this pull request Mar 9, 2020

text selection toolbar over flow ! #35826

Closed

deandreamatias mentioned this pull request Mar 9, 2020

TextField Cut/Copy/Paste popup menu doesn't wrap. #49971

Closed

justinmc added 2 commits March 9, 2020 10:01

Forgot to remove this comment

8a104f5

No need for _menuChanged anymore because AnimatedSize rebuilds with n…

bd7f9b4

…ew key

justinmc force-pushed the selection-overflow branch from 5340d21 to bd7f9b4 Compare March 9, 2020 17:02

Code cleanup

2f0327b

goderbauer approved these changes Mar 10, 2020

View reviewed changes

chunhtai reviewed Mar 10, 2020

View reviewed changes

justinmc added 2 commits March 10, 2020 14:13

Only run new Android menu tests on Android

b11af53

Code review fixes

c72a78f

Explicityly skip browser in android tests

9606e62

justinmc force-pushed the selection-overflow branch from 1f681e1 to 9606e62 Compare March 11, 2020 16:53

justinmc added 2 commits March 11, 2020 11:03

No need to close overflowOpen on button press

fec9127

Localization

0a95c99

justinmc merged commit 4841a7e into flutter:master Mar 11, 2020

justinmc deleted the selection-overflow branch March 11, 2020 20:36

justinmc mentioned this pull request Mar 18, 2020

iOS Text Selection Menu Overflow #52852

Closed

This was referenced Apr 6, 2020

iOS Text Selection Menu Overflow #54139

Closed

iOS Text Selection Menu Overflow #54140

Merged

justinmc mentioned this pull request Apr 28, 2020

Allow selection controls for SelectableText #55178

Closed

13 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Text Selection Overflow (Android) #49391

Text Selection Overflow (Android) #49391

Uh oh!

Conversation

justinmc commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Breaking change

Uh oh!

fluttergithubbot commented Jan 23, 2020

Uh oh!

fluttergithubbot commented Mar 9, 2020

Uh oh!

fluttergithubbot commented Mar 9, 2020

Uh oh!

justinmc commented Mar 9, 2020

Uh oh!

fluttergithubbot commented Mar 9, 2020

Uh oh!

goderbauer 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!

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!

justinmc Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Mar 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skia-gold commented Mar 10, 2020

Uh oh!

skia-gold commented Mar 10, 2020

Uh oh!

fluttergithubbot commented Mar 10, 2020

Uh oh!

skia-gold commented Mar 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

justinmc commented Jan 23, 2020 •

edited

Loading

justinmc Mar 11, 2020 •

edited

Loading