iOS Text Selection Menu Overflow #54140

justinmc · 2020-04-06T21:05:17Z

Description

If the selection menu buttons are too wide for their available space, they will cause an overflow error. This PR implements the native iOS solution for this problem, which paginates overflowing items.

Related Issues

Closes #35826
Android solution: #49391
Reopened from #52852 to fix weird versioning test failures.

Tests

When all buttons fit on one page.
When the screen is narrow enough that only two buttons fit on each page.
When the screen is narrow enough that only one button fits on each page.
When the locale strings are so long that only one button fits on each page and must be ellided.

Breaking Change

None.

commit 729501d Author: Justin McCandless <jmccandless@google.com> Date: Mon Apr 6 09:16:32 2020 -0700 Trying to fix CL failure with update-packages --force-upgrade commit 0cbc472 Author: Justin McCandless <jmccandless@google.com> Date: Fri Apr 3 11:09:38 2020 -0700 Comments commit 43cbd77 Author: Justin McCandless <jmccandless@google.com> Date: Fri Apr 3 11:05:18 2020 -0700 Custom long locale test commit 3b5a7e4 Author: Justin McCandless <jmccandless@google.com> Date: Fri Apr 3 09:38:40 2020 -0700 Clean up code, fix test with incorrect width. commit 491ed03 Author: Justin McCandless <jmccandless@google.com> Date: Thu Apr 2 16:28:15 2020 -0700 On selectall, page resets. Width maxes out at first page width. commit c690d50 Merge: 6c9bd26 e2621bf Author: Justin McCandless <jmccandless@google.com> Date: Thu Apr 2 14:43:41 2020 -0700 Merge branch 'master' into selection-overflow-ios commit 6c9bd26 Author: Justin McCandless <jmccandless@google.com> Date: Thu Apr 2 14:41:29 2020 -0700 Slot naming commit 90aa6b3 Author: Justin McCandless <jmccandless@google.com> Date: Thu Apr 2 14:39:38 2020 -0700 debugDescribeChildren commit 0126737 Author: Justin McCandless <jmccandless@google.com> Date: Thu Apr 2 14:32:44 2020 -0700 Reimplement dividers as empty space commit 56653a5 Author: Justin McCandless <jmccandless@google.com> Date: Thu Apr 2 11:12:34 2020 -0700 Fix debugImplementOnstageChildren so tests work commit 4dc4d29 Author: Justin McCandless <jmccandless@google.com> Date: Wed Apr 1 16:22:38 2020 -0700 Fix analyze errors commit 3cf51ec Author: Justin McCandless <jmccandless@google.com> Date: Wed Apr 1 15:54:05 2020 -0700 Hit test slot children as well, allowing pages to work commit 305e3b9 Author: Justin McCandless <jmccandless@google.com> Date: Wed Apr 1 15:31:23 2020 -0700 Fix crash when dismissing menu due to duplicate detach commit ee3294d Author: Justin McCandless <jmccandless@google.com> Date: Wed Mar 25 17:00:08 2020 -0700 Painting slotted and list children together, but no divider yet, no changing page. commit e0640f5 Author: Justin McCandless <jmccandless@google.com> Date: Wed Mar 25 11:48:25 2020 -0700 Rendering properly in the simple case of no overflow commit 56122a4 Author: Justin McCandless <jmccandless@google.com> Date: Tue Mar 24 16:24:25 2020 -0700 Slots for divider and buttons. Compiles but needs rendering support. commit be69380 Author: Justin McCandless <jmccandless@google.com> Date: Fri Mar 20 09:25:06 2020 -0700 Divider infrastructure commit 0f2bd59 Author: Justin McCandless <jmccandless@google.com> Date: Fri Mar 20 08:47:38 2020 -0700 Share parent data between material and cupertino commit de84e9b Author: Justin McCandless <jmccandless@google.com> Date: Thu Mar 19 08:20:04 2020 -0700 Keep const while passing linter commit cdf9ab6 Author: Justin McCandless <jmccandless@google.com> Date: Wed Mar 18 14:25:12 2020 -0700 Code cleanup commit f3ff782 Author: Justin McCandless <jmccandless@google.com> Date: Wed Mar 18 14:11:33 2020 -0700 Don't create menu if no buttons commit 21b262a Author: Justin McCandless <jmccandless@google.com> Date: Wed Mar 18 14:03:46 2020 -0700 Full test suite working (with dividers disabled for now commit c1da1db Author: Justin McCandless <jmccandless@google.com> Date: Wed Mar 18 11:02:34 2020 -0700 Tests (some WIP) and shouldPaint commit 6acf592 Merge: a12d1e7 a811bce Author: Justin McCandless <jmccandless@google.com> Date: Wed Mar 18 08:48:20 2020 -0700 Merge branch 'master' into selection-overflow-ios commit a12d1e7 Author: Justin McCandless <jmccandless@google.com> Date: Tue Mar 10 12:58:37 2020 -0700 Long child at any position works. commit 898e946 Author: Justin McCandless <jmccandless@google.com> Date: Mon Mar 9 15:35:04 2020 -0700 First long child fits and is ellided commit 267feb3 Author: Justin McCandless <jmccandless@google.com> Date: Mon Mar 9 14:55:45 2020 -0700 Next arrow always shows on subsequent pages, but may be disabled commit a5388c5 Author: Justin McCandless <jmccandless@google.com> Date: Mon Mar 9 09:48:34 2020 -0700 Back and forward buttons work commit 385174f Author: Justin McCandless <jmccandless@google.com> Date: Thu Mar 5 15:36:13 2020 -0800 Rendering by page, but needs hit detection and cleanup commit d512941 Author: Justin McCandless <jmccandless@google.com> Date: Fri Feb 28 11:05:41 2020 -0800 A working renderbox setup, but fading and pages is broken right now commit 113638f Author: Justin McCandless <jmccandless@google.com> Date: Fri Feb 28 10:40:06 2020 -0800 Animate between pages commit 55b7ea6 Author: Justin McCandless <jmccandless@google.com> Date: Fri Feb 28 09:49:10 2020 -0800 Proof of concept menu with multiple pages

chunhtai

I only review the widget part of it. will review the renderobject later

chunhtai · 2020-04-08T19:14:00Z

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

+    @required this.children,
+    @required this.isArrowPointingDown,
+  }) : assert(children != null),
+       assert(children.length > 0), // ignore: prefer_is_empty


Is this to get around the const?

Please add a comment to the ignore explaining why it is there per our style guide.

Yes, .isNotEmpty isn't const. I'll add a comment about it.

chunhtai · 2020-04-08T19:16:47Z

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

+  }
+
+  void _listener() {
+    if (_controller.value != 0.0) {


If this only trigger when animation finishes, you can use addStatusListener

That should be more performant, thanks for the tip 👍

chunhtai · 2020-04-08T19:18:26Z

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

+  void _handlePreviousPage() {
+    _controller.reverse();
+    _controller.addListener(_listener);
+    setState(() {


do we need setstate here? it seems the the only _page is used in building the widget

chunhtai · 2020-04-08T19:18:35Z

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

+  void _handleNextPage() {
+    _controller.reverse();
+    _controller.addListener(_listener);
+    setState(() {


chunhtai · 2020-04-08T19:20:03Z

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

+  void didUpdateWidget(_CupertinoTextSelectionToolbarContent oldWidget) {
+    // If the children are changing, the current page should be reset.
+    if (widget.children != oldWidget.children) {
+      _page = 0;


I wonder if we need to worry about if the animation is on going

That's a great catch, it would be potentially possible for the animation to navigate to a non-existent page. I'll fix this by resetting the animation here.

chunhtai · 2020-04-08T19:21:31Z

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

+      // This was eyeballed on a physical iOS device running iOS 13.
+      duration: const Duration(milliseconds: 150),
+    );
+    super.initState();


nit: this should be at the start of the method.

goderbauer · 2020-04-08T23:00:16Z

packages/flutter/test/material/text_field_test.dart

@@ -16,9 +16,9 @@ import 'package:flutter/foundation.dart';
 import 'package:flutter/gestures.dart' show DragStartBehavior, PointerDeviceKind;

 import '../rendering/mock_canvas.dart';
+import '../text.dart' show findRenderEditable, globalize, textOffsetToPosition;


putting this file in the toplevel test directory feels odd. Can we stick it in tests/widgets since they seem to include util functions for dealing with EditableText widgets?

Yeah good call.

goderbauer · 2020-04-09T18:02:01Z

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

+    @required this.children,
+    @required this.isArrowPointingDown,
+  }) : assert(children != null),
+       assert(children.length > 0), // ignore: prefer_is_empty


Please add a comment to the ignore explaining why it is there per our style guide.

goderbauer · 2020-04-09T18:06:03Z

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

+            borderRadius: null,
+            color: _kToolbarBackgroundColor,
+            minSize: _kToolbarHeight,
+            onPressed: () {},


I would have expected a disabled button to have null as onPressed handler. Why is it an empty closure?

Ah, I did this because the default disabled background color didn't look right. I can fix this with disabledColor though, so I'll remove this onPressed.

goderbauer · 2020-04-09T18:07:20Z

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

+            pressedOpacity: 0.7,
+            child: const Text('▶', style: _kToolbarButtonFontStyle),
+          ),
+          nextButtonDisabled: CupertinoButton(


for my own understanding: When does the toolbar show a disabled button? I thought if there is no page to go to it just doesn't show a button?

From my experimentation, the only time it won't show a next button is if there is only one page. Otherwise there is always a next button, and on the last page it's disabled.

Interesting. I never noticed that. Thanks for the explanation.

goderbauer · 2020-04-09T18:13:30Z

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

+  @override
+  void initState() {
+    _controller = AnimationController(
+      value: 1.0,


initially, the toolbar doesn't fade in? It just appears?

Initially the entire toolbar fades in. This controller handles the fading of the buttons within the toolbar. During page changes, the toolbar background stays visible, but the buttons fade in/out inside of it. There is another existing widget that handles fading the entire toolbar when it shows/hides.

I'll add a comment to the declaration of _controller about what it does.

I also noticed there's a slight existing bug with transitioning between states that's somewhat related to this. I opened a new issue: #54470

goderbauer · 2020-04-09T19:05:12Z

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

+      i++;
+      final RenderBox child = renderObjectChild as RenderBox;
+      final ToolbarItemsParentData childParentData = child.parentData as ToolbarItemsParentData;
+      childParentData.shouldPaint = false;


Is this necessary when it gets overwritten by line 931?

Now it is! Since I made the optimization above where I don't layout all children, line 931 doesn't get run on all children.

goderbauer · 2020-04-09T20:43:26Z

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

+    _nextButton.layout(constraints.loosen(), parentUsesSize: true);
+    _nextButtonDisabled.layout(constraints.loosen(), parentUsesSize: true);
+
+    double buttonPosition = 0.0;


When first reading through this code I was a little confused by the variable naming of buttonPosition and buttonsWidth. buttonsWidth only includes the width of the forward/back button, where-as buttonPosition is for all buttons.

Not super-sure how to make this better. Maybe call this one nextButtonPostition? And maybe the other paginationButtonWidth?

I struggled with coming up with good names when writing this. I'm going to go with currentButtonPosition since nextButtonPosition could be interpreted as the position of the right arrow. paginationButtonsWidth sounds good.

goderbauer · 2020-04-09T20:46:35Z

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

+    _nextButtonDisabled.layout(constraints.loosen(), parentUsesSize: true);
+
+    double buttonPosition = 0.0;
+    double parentWidth = constraints.maxWidth; // The width of the whole widget.


maybe: toolbarWidth?

Why does it start out as the max width possible instead of zero?

I think I was using it as a constraint earlier and then I changed my code, good call.

goderbauer · 2020-04-09T20:52:07Z

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

+  @override
+  void attach(PipelineOwner owner) {
+    super.attach(owner);
+    visitChildren((RenderObject renderObjectChild) {


Will this not attach the list children twice? From the supercall and here again?

Yes, good catch.

goderbauer · 2020-04-09T20:53:31Z

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

+    super.detach();
+
+    // Detach slot children.
+    visitChildren((RenderObject renderObjectChild) {


Instead of iterating over all children, couldn't you just iterate over the slot children in childToSlot?

justinmc · 2020-04-11T00:18:24Z

@chunhtai @goderbauer Thank you guys! It's ready for re-review when you have time.

goderbauer

LGTM

goderbauer · 2020-04-13T21:52:16Z

packages/flutter/test/cupertino/text_selection_test.dart

+  // Returns true iff the button is visually enabled.
+  bool appearsEnabled(WidgetTester tester, String text) {
+    final CupertinoButton button = tester.widget<CupertinoButton>(
+


nit: remove this blank line?

goderbauer · 2020-04-13T22:12:18Z

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

+  }
+
+  @override
+  void initState() {


nit: order-wise it would be nice to place initState before dispose and didUpdateWidget.

goderbauer · 2020-04-13T22:13:41Z

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

+  void didUpdateWidget(_CupertinoTextSelectionToolbarContent oldWidget) {
+    // If the children are changing, the current page should be reset.
+    if (widget.children != oldWidget.children) {
+      _page = 0;


Maybe also reset _nextPage to null? (I don't think this is a visual bug, but it may be strange during debugging if this is set to something that's no longer valid.)

goderbauer · 2020-04-13T22:14:39Z

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

+            pressedOpacity: 0.7,
+            child: const Text('▶', style: _kToolbarButtonFontStyle),
+          ),
+          nextButtonDisabled: CupertinoButton(


Interesting. I never noticed that. Thanks for the explanation.

goderbauer · 2020-04-13T22:21:34Z

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

+    _nextButtonDisabled.layout(constraints.loosen(), parentUsesSize: true);
+
+    final double subsequentPageButtonsWidth =
+      _backButton.size.width + _nextButton.size.width;


nit: either leave this on the previous line or indent by 4

goderbauer · 2020-04-13T22:22:59Z

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

+        buttonsWidth = _backButton.size.width + _nextButton.size.width;
+      }
+
+      // The width of the menu is set by the first page.


Interesting observation. Thanks for the gif.

goderbauer · 2020-04-13T22:24:33Z

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

+    assert(page <= currentPage);
+
+    // Position page nav buttons.
+    final ToolbarItemsParentData nextButtonParentData =


nit: these three vars could all be declared inside the if (currentPage > 0) {, no?

Good catch 👍

chunhtai

LGTM

chunhtai · 2020-04-13T23:26:33Z

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

-        child: Text(text, style: _kToolbarButtonFontStyle),
+        child: Text(
+          text,
+          overflow: TextOverflow.ellipsis,


Just a FYI, this currently only work with word boundary. If for some reason the space is not enough, copy -> ... Paste -> ... select all -> select ...

I think somehow it's working ok, see this screenshot if I add a bunch of A's to Paste. Is that what you mean?

oh nvm it seems you are right.

justinmc requested review from goderbauer and chunhtai April 6, 2020 21:05

justinmc self-assigned this Apr 6, 2020

googlebot added the cla: yes label Apr 6, 2020

chunhtai reviewed Apr 8, 2020

View reviewed changes

goderbauer reviewed Apr 9, 2020

View reviewed changes

justinmc added 5 commits April 10, 2020 10:29

Comment and status listener

569679c

Bug fixed where can go to nonexistent page

5670c05

Move text.dart to widgets test folder

6b7e449

_controller comment and simplified disabled styling

3de828e

Code review

74be346

goderbauer approved these changes Apr 13, 2020

View reviewed changes

chunhtai approved these changes Apr 13, 2020

View reviewed changes

Code review fixes

14934c0

justinmc merged commit f646e26 into flutter:master Apr 14, 2020

justinmc deleted the selection-overflow-ios-2 branch April 14, 2020 16:49

justinmc mentioned this pull request Apr 14, 2020

text selection toolbar over flow ! #35826

Closed

justinmc mentioned this pull request Apr 28, 2020

Allow selection controls for SelectableText #55178

Closed

13 tasks

PsyOhm23 mentioned this pull request Jun 2, 2020

Material text interactive selection uses buttonTheme #37550

Open

iapicca mentioned this pull request Jun 5, 2020

[proposal] Allow changing the textStyle of TextField's menu #58512

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

iOS Text Selection Menu Overflow #54140

iOS Text Selection Menu Overflow #54140

Conversation

justinmc commented Apr 6, 2020

Description

Related Issues

Tests

Breaking Change

chunhtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc commented Apr 11, 2020

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc Apr 10, 2020 •

edited

Loading