iOS TextSelectionToolbar fidelity #127757

luccasclezar · 2023-05-27T18:39:35Z

CupertinoTextSelectionToolbar is different from the native one, with some UI and UX issues. More details on the linked issue.

#127756

Currently the only problem that I listed on the linked issue that I couldn't fix was the horizontal scrolling, but to workaround this I added a GestureDetector to change pages when swiping the toolbar. It's not exactly the same as native as there is no scroll animation, but it works.

I'm creating this PR a little early to have some feedback as these changes were more complex than the ones in my last PR. Probably best if @justinmc is involved 😅

Version	Video
Flutter Old	old.mov
Flutter New	new.mov
Native	native.mov

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 this PR is [test-exempt].
All existing and new tests are passing.

flutter-dashboard · 2023-05-27T18:39:39Z

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 on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

justinmc

This is an awesome PR, the menu looks so much better from your videos. I think there's one architectural problem to solve (_CupertinoTextSelectionToolbarItemsController) but otherwise the approach looks good.

Is the point of that controller to be able to pass information up the class hierarchy from _RenderCupertinoTextSelectionToolbarItems to _CupertinoTextSelectionToolbarContentState? I think we need to find a better way to do that other than a shared mutable class.

Fortunately it seems like you only need that information in a callback (_handleNext/PreviousPage) so we can probably look it up without having to worry about layout order. Could you pass a key to _CupertinoTextSelectionToolbarItems and then look up the information using GlobalKey.currentWidget? Something like that should be doable...

justinmc · 2023-05-30T18:09:26Z

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

        // accommodate the back button.
      }
+
+      // Update the controller values so that we can check in the horizontal


I think the Flutter style guide recommends not using "we" etc., so maybe say "...so that it's possible to check them in the horizontal..."

Reminder on this one.

luccasclezar · 2023-05-30T20:36:14Z

Thank you! 😁

About the controller, yes, I only use it to know if it's possible to navigate to previous/next page when dragging, basically.

But I don't understand how a GlobalKey would solve this problem. The only way to know if there is a previous/next button (as far as I can tell) is in _RenderCupertinoTextSelectionToolbarItems.performLayout:

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

Lines 946 to 947 in 6762521

    
           controller?.hasNextPage = page != currentPage; 
        
           controller?.hasPreviousPage = page > 0;

So we need to set the variables in this method. From what I understood of your suggestion, a GlobalKey should be used instead of the "controller", like

globalKey.currentWidget.hasNextPage = page != currentPage;
globalKey.currentWidget.hasPreviousPage = page > 0;

But how would I set variables using globalKey.currentWidget if all widget variables are final? If _RenderCupertinoTextSelectionToolbarItems were a Widget I could assign it a GlobalKey and get its properties, but RenderBox doesn't have a key parameter.

Probably I'm missing something here, but I couldn't come up with a solution that doesn't involve a shared mutable class. 😅

tgucio · 2023-05-30T20:50:21Z

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

+// correctly shows/hides the previous/next buttons automatically, but now the
+// horizontal drag gesture callback has to check whether it's possible to
+// navigate or not, making this necessary.
+class _CupertinoTextSelectionToolbarItemsController {


I think controller classes are normally used in APIs for others widgets to change the state of a stateful widget. Here we're operating on a render object so this should probably be done using individual fields e.g.

void updateRenderObject(BuildContext context, _RenderCupertinoTextSelectionToolbarItems renderObject) { renderObject ..page = page ..hasPreviousPage = hasPreviousPage ..hasNextPage = hasNextPage ..dividerColor = dividerColor

Then in the RenderBox class you'd have a setter and a getter just like with dividerColor.

The problem with this approach is that, as far as I understand it, I wouldn't be able to update hasNextPage and hasPreviousPage in _RenderCupertinoTextSelectionToolbarItems, and have it available in its parent (_CupertinoTextSelectionToolbarContentState) when the user does a horizontal drag gesture on the whole toolbar.

This is where I update the values:

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

Lines 944 to 947 in 6762521

// Update the controller values so that we can check in the horizontal

// drag gesture callback when it's possible to navigate.

controller?.hasNextPage = page != currentPage;

controller?.hasPreviousPage = page > 0;

And this is where I use them:

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

Lines 461 to 475 in 6762521

void _handleNextPage() {

if (_toolbarItemsController.hasNextPage) {

_controller.reverse();

_controller.addStatusListener(_statusListener);

_nextPage = _page + 1;

}

}

void _handlePreviousPage() {

if (_toolbarItemsController.hasPreviousPage) {

_controller.reverse();

_controller.addStatusListener(_statusListener);

_nextPage = _page - 1;

}

}

If the hasPreviousPage / hasNextPage fields are only updated by _RenderCupertinoTextSelectionToolbarItems (which makes sense given this will depend on the incoming constraints for the RenderBox), perhaps one approach would be to:

keep them as bool values without getter/setter

from the widget state, in the gesture handler, do something like:

final RenderBox? renderBox = context.findRenderObject() as RenderBox?; if (renderBox is _RenderCupertinoTextSelectionToolbarItems && renderBox.hasPreviousPage) { [...]

Perhaps there will be a bit extra complexity if you insert another widget inbetween like AnimatedSize: if this is the case and context.findRenderObject() returns another box, you could pass a global key to the _CupertinoTextSelectionToolbarItems widget you're looking for and do:

renderBox = _toolbarItemsKey.currentContext?.findRenderObject()

This is commonly done across the framework so you can search for findRenderObject().

+1 that's what I was thinking!

That makes sense and it's way cleaner, thank you! I will see if I can remove the controller until this weekend.

Just fixed this and all tests passed, should be OK now 😁

justinmc · 2023-06-26T20:49:14Z

@tgucio Can you give the final approval here?

tgucio · 2023-06-27T15:14:02Z

LGTM - thanks again for the PR!

Roll Flutter from 96a2c05 to 51bef1b (37 revisions) flutter/flutter@96a2c05...51bef1b 2023-06-28 [email protected] Roll Flutter Engine from e8a1c23d66ba to 241ca5c1d6be (1 revision) (flutter/flutter#129725) 2023-06-28 [email protected] Update analysis, linter, and repo links in analysis options (flutter/flutter#129686) 2023-06-28 [email protected] Roll Flutter Engine from be1073aa352f to e8a1c23d66ba (1 revision) (flutter/flutter#129723) 2023-06-28 [email protected] Dev, examples/api, etc updated for Material 3 by default (flutter/flutter#129683) 2023-06-28 [email protected] Add `DatePickerTheme.inputDecorationTheme` for the DatePicker with input mode. (flutter/flutter#128950) 2023-06-28 [email protected] [framework] ensure flexible space bar fades when scrolling. (flutter/flutter#129527) 2023-06-28 [email protected] Add InputDecorator.error to allow error message customization (flutter/flutter#129275) 2023-06-28 [email protected] Roll Flutter Engine from b388e852be44 to be1073aa352f (1 revision) (flutter/flutter#129712) 2023-06-28 [email protected] Roll Flutter Engine from 17173994a8c2 to b388e852be44 (1 revision) (flutter/flutter#129708) 2023-06-28 [email protected] [flutter roll] Revert "Fix `AnimatedList` & `AnimatedGrid` doesn't apply `MediaQuery` padding" (flutter/flutter#129645) 2023-06-28 [email protected] Roll Flutter Engine from 2f4fc4872699 to 17173994a8c2 (1 revision) (flutter/flutter#129694) 2023-06-28 [email protected] Roll Flutter Engine from a6d9d12c440f to 2f4fc4872699 (1 revision) (flutter/flutter#129691) 2023-06-28 [email protected] Roll Flutter Engine from 25a5850f8b5b to a6d9d12c440f (4 revisions) (flutter/flutter#129687) 2023-06-28 [email protected] Roll Flutter Engine from 7c7c45d53bec to 25a5850f8b5b (1 revision) (flutter/flutter#129682) 2023-06-27 [email protected] Roll Flutter Engine from f320b8c36fee to 7c7c45d53bec (14 revisions) (flutter/flutter#129678) 2023-06-27 [email protected] Update labeler yaml (flutter/flutter#129676) 2023-06-27 [email protected] Revert "Fix the matcher condition where multiple matchers are found" (flutter/flutter#129675) 2023-06-27 [email protected] Revert "Labeler format to remove extra single quote" (flutter/flutter#129674) 2023-06-27 [email protected] Revert "Update labeler.yml to v5.0.0-beta.1" (flutter/flutter#129673) 2023-06-27 [email protected] Labeler format to remove extra single quote (flutter/flutter#129672) 2023-06-27 [email protected] Fix the matcher condition where multiple matchers are found (flutter/flutter#129670) 2023-06-27 [email protected] Automatically migrate ClipboardData.text to non-null (flutter/flutter#129567) 2023-06-27 [email protected] Remove Editable.onCaretChanged callback (flutter/flutter#109114) 2023-06-27 [email protected] Reland "Fix issue where DevTools would not be immediately available when using --start-paused (#126698)" (flutter/flutter#129368) 2023-06-27 [email protected] Update Xcode to 14.3.1 (flutter/flutter#129024) 2023-06-27 [email protected] Adds `dart_fix` support to `integration_test` (flutter/flutter#129579) 2023-06-27 [email protected] Update labeler.yml to v5.0.0-beta.1 (flutter/flutter#129617) 2023-06-27 [email protected] iOS TextSelectionToolbar fidelity (flutter/flutter#127757) 2023-06-27 [email protected] Make a paragraph test involving Chinese characters work with inconsistent host system fonts (flutter/flutter#129628) 2023-06-27 [email protected] Roll Packages from 6b70804 to f89ce02 (7 revisions) (flutter/flutter#129630) 2023-06-27 [email protected] Roll Flutter Engine from 715eff211a42 to f320b8c36fee (6 revisions) (flutter/flutter#129599) 2023-06-27 [email protected] Fix chinese text is not selected by long press (flutter/flutter#129320) 2023-06-26 [email protected] Roll Flutter Engine from 0da06de991a9 to 715eff211a42 (4 revisions) (flutter/flutter#129593) 2023-06-26 [email protected] Fix syntax error in no-response (flutter/flutter#129588) 2023-06-26 [email protected] Roll Flutter Engine from f2d70cc809cd to 0da06de991a9 (3 revisions) (flutter/flutter#129582) 2023-06-26 [email protected] Updated chip_test.dart tests for M3 (flutter/flutter#129570) 2023-06-26 [email protected] Roll Flutter Engine from 4032a9bc964e to f2d70cc809cd (4 revisions) (flutter/flutter#129574) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose ...

luccasclezar added 4 commits May 27, 2023 00:45

Update CupertinoSelectionToolbar fidelity

69293e2

Improve CupertinoTextSelectionToolbar fidelity

aefc317

Add swipe gesture to CupertinoTextSelectionToolbar

d3c10f9

Merge branch 'flutter:master' into master

112f0a2

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels May 27, 2023

github-actions bot removed framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels May 27, 2023

luccasclezar added 2 commits May 29, 2023 17:06

Fix tests for Cupertino text toolbar update

e5421b1

Merge branch 'flutter:master' into master

a91d8b4

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels May 29, 2023

Use CupertinoButton on text toolbar button

3484a12

github-actions bot removed framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels May 29, 2023

luccasclezar added 5 commits May 29, 2023 19:04

Fix text_field_test.dart after toolbar changes

6483e1e

Fix text_field_test.dart after toolbar changes

248868b

Merge branch 'flutter:master' into master

f4a744c

Fix recursive import

57962fb

Merge branch 'flutter:master' into master

6762521

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels May 30, 2023

justinmc reviewed May 30, 2023

View reviewed changes

tgucio reviewed May 30, 2023

View reviewed changes

luccasclezar added 2 commits June 4, 2023 20:20

Remove controller from Cupertino toolbar

cd3aea7

Merge branch 'flutter:master' into master

91aeb75

github-actions bot removed the f: cupertino flutter/packages/flutter/cupertino repository label Jun 4, 2023

tgucio approved these changes Jun 27, 2023

View reviewed changes

justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 27, 2023

auto-submit bot merged commit a90c33f into flutter:master Jun 27, 2023

engine-flutter-autoroll mentioned this pull request Jun 27, 2023

Roll Flutter from 96a2c0535810 to a90c33fd61e1 (10 revisions) flutter/packages#4324

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 27, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

bc4660b

engine-flutter-autoroll mentioned this pull request Jun 27, 2023

Roll Flutter from 96a2c0535810 to b359e9072ff6 (11 revisions) flutter/packages#4325

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 27, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

cc2f4b4

engine-flutter-autoroll mentioned this pull request Jun 27, 2023

Roll Flutter from 96a2c0535810 to 321abcbe4f70 (12 revisions) flutter/packages#4326

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 27, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

16bb425

engine-flutter-autoroll mentioned this pull request Jun 27, 2023

Roll Flutter from 96a2c0535810 to 9531cd40008a (13 revisions) flutter/packages#4327

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 27, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

4fd748d

engine-flutter-autoroll mentioned this pull request Jun 27, 2023

Roll Flutter from 96a2c0535810 to 5ea2be69ca07 (14 revisions) flutter/packages#4328

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 27, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

ba0fdaa

danagbemava-nc mentioned this pull request Jun 28, 2023

[iOS] Improve CupertinoTextSelectionToolbar visual fidelity #127756

Closed

2 tasks

engine-flutter-autoroll mentioned this pull request Jun 28, 2023

Roll Flutter from 96a2c0535810 to 8d837062a447 (31 revisions) flutter/packages#4334

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 28, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

cd51f6c

engine-flutter-autoroll mentioned this pull request Jun 28, 2023

Roll Flutter from 96a2c0535810 to 32fe4d7f0a27 (33 revisions) flutter/packages#4335

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 28, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

15a53e1

engine-flutter-autoroll mentioned this pull request Jun 28, 2023

Roll Flutter from 96a2c0535810 to 73bb959a6524 (35 revisions) flutter/packages#4336

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 28, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

ca01719

engine-flutter-autoroll mentioned this pull request Jun 28, 2023

Roll Flutter from 96a2c0535810 to 51bef1b63271 (37 revisions) flutter/packages#4338

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 28, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

912137c

HansMuller mentioned this pull request Jun 30, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

4d6a0fb

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

ed03012

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

4ef4056

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

a90c33f iOS TextSelectionToolbar fidelity (flutter/flutter#127757)

1df34f8

	// Update the controller values so that we can check in the horizontal
	// drag gesture callback when it's possible to navigate.
	controller?.hasNextPage = page != currentPage;
	controller?.hasPreviousPage = page > 0;

	void _handleNextPage() {
	if (_toolbarItemsController.hasNextPage) {
	_controller.reverse();
	_controller.addStatusListener(_statusListener);
	_nextPage = _page + 1;
	}
	}

	void _handlePreviousPage() {
	if (_toolbarItemsController.hasPreviousPage) {
	_controller.reverse();
	_controller.addStatusListener(_statusListener);
	_nextPage = _page - 1;
	}
	}

iOS TextSelectionToolbar fidelity #127757

iOS TextSelectionToolbar fidelity #127757

Uh oh!

Conversation

luccasclezar commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented May 27, 2023

Uh oh!

justinmc left a comment • 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!

justinmc May 30, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

luccasclezar commented May 30, 2023

Uh oh!

tgucio May 30, 2023

Choose a reason for hiding this comment

Uh oh!

luccasclezar May 31, 2023

Choose a reason for hiding this comment

Uh oh!

tgucio May 31, 2023 • 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!

justinmc May 31, 2023

Choose a reason for hiding this comment

Uh oh!

luccasclezar May 31, 2023

Choose a reason for hiding this comment

Uh oh!

luccasclezar Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc commented Jun 26, 2023

Uh oh!

tgucio commented Jun 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

luccasclezar commented May 27, 2023 •

edited

Loading

justinmc left a comment •

edited

Loading

tgucio May 31, 2023 •

edited

Loading