Display text input toolbar on single tap for iOS

[doppio]

This PR changes the behavior of single-tapping an in-focus editable text field on iOS, such that the selection toolbar is toggled if the user's text selection was not changed. The intent is for Flutter (iOS) text fields to mimic the behavior of native input toolbars on iOS. Note that this PR does not attempt to fix other platform inconsistencies with the text selection toolbar.

The primary change removes the hideToolbar call from the Material/Cupertino-specific implementations of TextField. Instead, the toolbar hide/show logic is performed in TextSelectionGestureDectorBuilder.onSingleTapUp based on the current platform.

This is an attempt to address a personal urgent need for correct input toolbar behavior on iOS, but it sounds like this change would be affected/overwritten by @justinmc's refactor mentioned here.

Fixes #48434

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.

[jmagman]

Test failures:
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8827194662548759057/+/u/run_test.dart_for_framework_tests_shard_and_subshard_libraries/test_stdout

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: <7>
  Actual: <4>

When the exception was thrown, this was the stack:
#4      main.<anonymous closure> (file:///opt/s/w/ir/x/w/flutter/packages/flutter/test/material/text_field_test.dart:2075:5)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

This was caught by the test expectation on the following line:
  file:///opt/s/w/ir/x/w/flutter/packages/flutter/test/material/text_field_test.dart line 2075
The test description was:
  dragging caret within a word doesn't affect composing region (variant: TargetPlatform.iOS)
════════════════════════════════════════════════════════════════════════════════════════════════════

01:55 +3295 ~14 -1: /opt/s/w/ir/x/w/flutter/packages/flutter/test/material/text_field_test.dart: dragging caret within a word doesn't affect composing region (variant: TargetPlatform.iOS) [E]        
  Test failed. See exception logs above.
  The test description was: dragging caret within a word doesn't affect composing region (variant: TargetPlatform.iOS)

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8827194662566049585/+/u/run_test.dart_for_web_canvaskit_tests_shard_and_subshard_3/test_stdout

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: at least one matching node in the widget tree
  Actual: _WidgetTypeFinder:<zero widgets with type "CupertinoButton" (ignoring offstage widgets)>
   Which: means none were found but some were expected

When the exception was thrown, this was the stack:
../dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 251:49  throw_
../packages/test_api/src/expect/async_matcher.dart.js 145:22                     fail
../packages/test_api/src/expect/async_matcher.dart.js 142:12                     _expect
../packages/test_api/src/expect/async_matcher.dart.js 75:12                      expect$
../packages/flutter_test/src/matchers.dart.js 4563:12                            expect$
text_field_test.dart.js 13040:21                                                 <fn>
../dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 45:50            <fn>
../packages/stack_trace/src/stack_zone_specification.dart.js 160:98              <fn>
../packages/stack_trace/src/stack_zone_specification.dart.js 212:16              [_run]
../packages/stack_trace/src/stack_zone_specification.dart.js 160:80              <fn>
../dart-sdk/lib/async/zone.dart 1434:47                                          _rootRunUnary
../dart-sdk/lib/async/zone.dart 1335:19                                          runUnary
../dart-sdk/lib/async/future_impl.dart 160:18                                    handleValue
../dart-sdk/lib/async/future_impl.dart 767:44                                    handleValueCallback
../dart-sdk/lib/async/future_impl.dart 796:13                                    _propagateToListeners
../dart-sdk/lib/async/future_impl.dart 602:5                                     [_completeWithValue]
../dart-sdk/lib/async/future_impl.dart 640:7                                     <fn>
../packages/fake_async/fake_async.dart.js 123:40                                 flushMicrotasks
../packages/flutter_test/src/deprecated.dart.js 1561:19                          <fn>
../dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 84:54            runBody
../dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 123:5            _async
../packages/flutter_test/src/deprecated.dart.js 1558:62                          <fn>
../dart-sdk/lib/async/future.dart 276:37                                         <fn>
../packages/stack_trace/src/stack_zone_specification.dart.js 212:16              [_run]
../packages/stack_trace/src/stack_zone_specification.dart.js 155:71              <fn>
../dart-sdk/lib/async/zone.dart 1418:47                                          _rootRun
../dart-sdk/lib/async/zone.dart 1328:19                                          run
../dart-sdk/lib/async/zone.dart 1236:7                                           runGuarded
../dart-sdk/lib/async/zone.dart 1276:23                                          <fn>
../packages/stack_trace/src/stack_zone_specification.dart.js 212:16              [_run]
../packages/stack_trace/src/stack_zone_specification.dart.js 155:71              <fn>
../dart-sdk/lib/async/zone.dart 1426:13                                          _rootRun
../dart-sdk/lib/async/zone.dart 1328:19                                          run
../dart-sdk/lib/async/zone.dart 1236:7                                           runGuarded
../dart-sdk/lib/async/zone.dart 1276:23                                          callback
../dart-sdk/lib/async/schedule_microtask.dart 40:11                              _microtaskLoop
../dart-sdk/lib/async/schedule_microtask.dart 49:5                               _startMicrotaskLoop
../dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 166:15           <fn>

The test description was:
  slow double tap on an unfocused field shows the toolbar (variant: TargetPlatform.iOS)
════════════════════════════════════════════════════════════════════════════════════════════════════

02:59 +333 ~10 -1: test/cupertino/text_field_test.dart: slow double tap on an unfocused field shows the toolbar (variant: TargetPlatform.iOS) [E]                                                      
  Test failed. See exception logs above.
  The test description was: slow double tap on an unfocused field shows the toolbar (variant: TargetPlatform.iOS)

[justinmc]

Thanks for doing this! Sorry this code is so convoluted before the refactor. I think you must have noticed from the previous failures, small changes to this code can have a lot of unintended consequences on other gestures and platforms. I think you've almost got it working though.

There was still one failure, but maybe it was a flake. I re-ran it.

[doppio]

Hrm, there's still a failure, but I don't think it's related to the changes here. Looks like there's some kind of permissions issue when running the check. 🤷‍♂️

[justinmc]

Looked like an infrastructure problem to me (not related to this PR). I just re-ran it. If it doesn't turn green, can you try merging master into this branch?

[justinmc]

@doppio Can you look at my comment about the return value in the test? Otherwise I think this is ready to merge. Sorry I forgot to check back in on this PR for awhile when it went green.

[doppio]

@justinmc You're right, that doesn't seem to serve any purpose anymore. I don't recall why that got added 😅. Removed!

[doppio]

@justinmc I think these check failures are more infrastructure flakiness -- they show as canceled. Should they be run again?

[flutter-dashboard]

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

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

[justinmc]

@doppio The failures did look unrelated. I've rerun them and hopefully they turn green.

I think there is a bug with this PR though. If you tap in a way that will move the cursor, then on native iOS the toolbar doesn't show, but it does when using this PR:

Screen.Recording.2022-02-24.at.1.20.12.PM.mov

[doppio]

Ahh, you're right @justinmc. After digging into it, I remember why I originally did it this way: #95644 (comment). Revising it re-introduced incorrect behavior. renderEditable.selection is not immediately updated when selectWordEdge is called, so the check for selection equality is returning a false positive. That's why I was returning the new selection.

I've made a revision to immediately update selection when the value gets set, after the TextSelectionDelegate has had a chance to do any input formatting that it needs, which feels like a better solution than changing that return value.

[SimonVillage]

Can you rebase with master to update your branch? That should resolve the failing checks.

[doppio]

I rebased with master, but the checks are still failing. I'm not sure what's going on with these check failures now as they were passing before.

[jmagman]

Failure is:

01:59 +2620 ~3: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart: keyboard is requested after setEditingState after switching to a new text field                           
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: [
            'TextInput.clearClient',
            'TextInput.setClient',
            'TextInput.setEditableSizeAndTransform',
            'TextInput.setMarkedTextRect',
            'TextInput.setStyle',
            'TextInput.setEditingState',
            'TextInput.show',
            'TextInput.requestAutofill',
            'TextInput.setCaretRect'
          ]
  Actual: MappedListIterable<MethodCall, String>:[
            'TextInput.clearClient',
            'TextInput.setClient',
            'TextInput.setEditableSizeAndTransform',
            'TextInput.setMarkedTextRect',
            'TextInput.setCaretRect',
            'TextInput.setStyle',
            'TextInput.setEditingState',
            'TextInput.show',
            'TextInput.requestAutofill'
          ]
   Which: at location [4] is 'TextInput.setCaretRect' instead of 'TextInput.setStyle'

When the exception was thrown, this was the stack:
#4      main.<anonymous closure> (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart:8290:7)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

This was caught by the test expectation on the following line:
  file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart line 8290
The test description was:
  keyboard is requested after setEditingState after switching to a new text field
════════════════════════════════════════════════════════════════════════════════════════════════════

01:59 +2620 ~3 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart: keyboard is requested after setEditingState after switching to a new text field [E]                    
  Test failed. See exception logs above.
  The test description was: keyboard is requested after setEditingState after switching to a new text field

[doppio]

@justinmc Any chance this could get another look? Apologies that it took me so long to get around to fixing up this PR.

[iosephmagno]

We hope so since this is key feature on ios textfield. Users use only single tap to show toolbar, app without that feature is just considered buggy

[justinmc]

@doppio +1 to what @Renzo-Olivares said about editableText.textEditingValue. Also can you update this PR with master? There is a merge conflict.

Otherwise I think this looks good!

[doppio]

I've made the suggested changes but it looks like some tests are now failing due to other changes to this system since I originally made the PR, and I'm struggling to fix them. To be honest, I think I'm in over my head at this point and I don't really have the time to keep revisiting this every few months. I'm closing this but anyone is welcome to take my changes if they contribute to a working solution.

[justinmc]

@doppio No worries and thanks for sticking with this for so long! This is really tricky to fix, hence why we're trying to refactor this in the long run. I'll see if I can work out a solution in the same vein in the meantime.

[iosephmagno]

@justinmc we at Presence are adopting a native wrapper. Our code is closed but you can access a public version here https://github.com/collinjackson/flutter-native-text-input

I might be wrong, but I think flutter should change its mind about text input and wrapper native textfield for both android and ios.
Simply put, it is very complex (perhaps impossible) to reproduce a true replica of the native widget and users are so used to it that wouldnt accept anything different for a premium app. Also, any change from android/ios would required you to catch up.

[SimonVillage]

It also seems like that a lot of tests are failing due to that change. As example tests which check that the toolbar doesn't show on a simple tap. It would require a bit more work then just the current changes in this PR.

[justinmc]

@iosephmagno I fielded a similar question recently here: #106703 (comment)

I'm keeping one eye on packages like flutter-native-text-input in case our current approach ends up clearly falling behind though.

[iosephmagno]

@justinmc actually the list of iOS flutter input issues is long. We started to think about wrapping native input because of #28894. Emojis size is very small in both text and textfield and that is a deal breaker for a premium chat app in flutter.

But then we also have come across with the other issues that you guys are dealing with such as #48434, #12920, #106703 and a few others.

Henry’s version of flutter native input textfield is not the best one. The guy stopped to be active on the plugin and we improved it a lot. We and some friends, added support for custom font, flutter-like onTap feature and fixed several bugs. If you guys want to give it a try, write to me and I will provide a link to the most updated repo.

The rationale is:

1. There is only 1 textfield per screen and that means that a native input field cannot cause a performance issue at screen opening. If truly needed, flutter team can make ad hoc improvements.
2. The native field is solid, fast and “native”. Flutter textfield issues will be wiped out all at once.
3. User will notice it. Platform fidelity for input components is everything. Indeed I could tell that an app is built in flutter by looking only at texts