[iOS] Add `UITextInteraction` only when `inputDelegate` is nil #32881

LongCatIsLooong · 2022-04-23T01:33:32Z

Fixes TextField text reappears after calling TextEditingController.clear with Japanese text input flutter#99652
Fixes [iOS]: When moving the cursor using keyboard, the Korean input does not start anew and is merged with the last input characters. flutter#98590

This pull request removes the UITextInteraction initialization in [FlutterTextInputView initWithOwner:] previously added to FlutterTextInputView in #26486.

When a certain UITextInteraction is removed from the text input view, it (incorrectly?) sets inputDelegate to nil instead of [UIKeyboardImpl activeInstance]. This workaround adds a UITextInteraction to the view when we need inputDelegate but it's currently nil (see setTextInputState), so the UITextInteraction's setup process sets inputDelegate to a UITextInteractionInputDelegate which eventually notifies the current active UIKeyboardImpl in its UITextInputDelegate implementation. And we remove the UITextInteraction when we finish using inputDelegate.

A few observations:

Floating cursor seems to be working correctly even without the UITextInteraction now.
It seems inputDelegate is set to nil when the last/root text interaction is removed from the view. So another (easier) workaround for the issue is to add our own UITextInteraction to the input view to make sure there's always one UITextInteraction installed, but that adds unwanted visual artifacts during scribble.

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jmagman

Was it an accident? #24224 (comment)

That comment says it was "delayed" not removed so maybe it was an accident?

Can you confirm this change doesn't regress

for some reason, the OS seems to be adding a grey shaded rectangle to the screen when using scribble to delete. I suspect it is related to autocorrect highlighting, but I'm not sure. So far I haven't been able to get rid of it. It does go away when tapping the screen again.

LongCatIsLooong · 2022-04-25T17:47:05Z

@fbcouch it looks like the Japanese romaji keyboard needs the UITextInteraction to work (my guess is it has to do with the swipe gestures used on those keyboards). Is there an alternative workaround for the scribble delete issue?

fbcouch · 2022-04-25T18:18:42Z

Hm. I didn't have another workaround, but – I notice you're just changing where the interaction gets set up. Is it possible to either detect the language the user is using and conditionally set up the UITextInteraction there, or add something to the framework that tells the engine that it needs that to be already available?

LongCatIsLooong · 2022-04-26T02:20:06Z

The romaji keyboard doesn't seem to call any of UITextInteraction's public properties/methods when I use swipe gesture to enter text.
But UITextInteraction does set UITextInput.inputDelegate to a UITextInteractionInputDelegate I guess that's why it is needed, whenever the text is changed the delegate needs to be notified.

flutter-dashboard · 2022-04-26T17:35:16Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

This reverts commit f499993.

fbcouch · 2022-04-26T22:13:14Z

I wonder if we might be able to make use of https://developer.apple.com/documentation/uikit/uiscribbleinteraction/3566750-pencilinputexpected?language=objc somehow to conditionally set up the UITextInteraction...or, from reading your comment on the linked issue, is there a way to just not clear the TextInput.inputDelegate?

LongCatIsLooong · 2022-04-26T22:57:32Z

Did a bit testing and UITextInteraction seems to be longer needed for floating cursor to work with marked text. We should be able to remove it if it wasn't for flutter/flutter#99652.

flutter/flutter#99652 seems to be a UIKit bug. @fbcouch I have a (not so pleasant) workaround to reset the inputDelegate to the current activeUIKeyboardImpl instance. I'll update the pull request in a bit.

LongCatIsLooong · 2022-04-27T00:28:33Z

the workaround unfortunately doesn't work well when there's composing text, as it requires the keyboard to restart, and as a result the currently marked (composing) text will be lost.
assigning inputDelegate to [UIKeyboardImpl activeInstance] should work but that's a private class
trying to retain the UITextInteractionInputDelegate and remove the UITextInteration from the view will result in EXC_BAD_ACCESS.

The easiest way out is to keep the UITextInteraction in init until the bug is fixed in UIKit. @fbcouch how badly would scribble be affected if UITextInteraction is set up in init?

LongCatIsLooong · 2022-04-26T23:34:55Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+  if (!self.inputDelegate && self.isFirstResponder) {
+    NSAssert(!_restartingInputDelegate, @"setTextInputState is not re-entrant.");
+    _restartingInputDelegate = YES;
+    [self resignFirstResponder];


For unknown reasons -[UIResponder reloadInputViews] does not work here (while it does set the inputDelegate when called in setTextInputClient:withConfiguration). -[UIKeyboardImpl setDelegate:force:] is called but it doesn't actually set inputDelegate to the receiver UIKeyboardImpl.

LongCatIsLooong · 2022-04-27T01:23:25Z

I guess I could add a dummy text interaction when the input delegate is being set to 0. I'll try that tomorrow

fbcouch · 2022-04-27T14:11:07Z

The easiest way out is to keep the UITextInteraction in init until the bug is fixed in UIKit. @fbcouch how badly would scribble be affected if UITextInteraction is set up in init?

I think the only issue is that there's a grey rectangle left over in some cases (maybe when deleting with scribble?) that goes away on the next interaction. I wonder if an alternative workaround would be to remove the interaction when a scribble interaction starts, then add it back when it ends?

LongCatIsLooong · 2022-04-27T17:12:02Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.mm

 #pragma mark - Floating Cursor - Tests

- (void)testInputViewsHaveUIInteractions {
+- (void)testInputViewsDoNotHaveUITextInteractions {


I can't think of a good way to test this.

Well I guess I can verify setInputDelegate is called in setTextInputState. Let me do that.

…on-back

jmagman · 2022-04-27T23:25:56Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+  if (!_textInteraction) {
+    _textInteraction =
+        [[UITextInteraction textInteractionForMode:UITextInteractionModeEditable] retain];
+    _textInteraction.textInput = self;


Does _textInteraction.textInput need to be nil'd out somewhere to break the retain cycle?

https://developer.apple.com/documentation/uikit/uitextinteraction/3255084-textinput?language=objc this is a weak property and the framework is compiled with ARC so I assume it doesn't increase the retain count?

👍 Thanks for checking.

Sorry that was supposed to be a comment not approval.

jmagman

LGTM

jmagman · 2022-04-28T00:22:09Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+  if (!_textInteraction) {
+    _textInteraction =
+        [[UITextInteraction textInteractionForMode:UITextInteractionModeEditable] retain];
+    _textInteraction.textInput = self;


👍 Thanks for checking.

flutter/engine#32881)

LongCatIsLooong requested review from gaaclarke and jmagman April 23, 2022 01:33

flutter-dashboard bot added the platform-ios label Apr 23, 2022

jmagman reviewed Apr 25, 2022

View reviewed changes

Put UITextInteraction back

b8319b5

LongCatIsLooong force-pushed the put-uitextinteraction-back branch from 5386b20 to b8319b5 Compare April 25, 2022 17:43

Remove UITextInteraction

f499993

LongCatIsLooong marked this pull request as draft April 26, 2022 17:29

Put UITextInteraction back

9b6f13f

This reverts commit f499993.

LongCatIsLooong force-pushed the put-uitextinteraction-back branch from b4ffe70 to 64222c9 Compare April 26, 2022 23:07

remove UITextInteraction. Add workaround for inputDelegate.

517743a

LongCatIsLooong force-pushed the put-uitextinteraction-back branch from 64222c9 to 517743a Compare April 26, 2022 23:12

LongCatIsLooong commented Apr 27, 2022

View reviewed changes

Remove UITextInteraction initialization from [FlutterTextInputView init]

6d63001

LongCatIsLooong changed the title ~~[iOS] Add UITextInteraction back~~ [iOS] Add UITextInteraction only when inputDelegate is nil Apr 27, 2022

LongCatIsLooong marked this pull request as ready for review April 27, 2022 17:10

LongCatIsLooong commented Apr 27, 2022

View reviewed changes

LongCatIsLooong requested a review from jmagman April 27, 2022 17:12

LongCatIsLooong force-pushed the put-uitextinteraction-back branch from a64b0c7 to c15a69c Compare April 27, 2022 17:58

add test

3a9a7ca

LongCatIsLooong force-pushed the put-uitextinteraction-back branch from c15a69c to 3a9a7ca Compare April 27, 2022 18:09

Merge remote-tracking branch 'upstream/main' into put-uitextinteracti…

10432ea

…on-back

jmagman previously approved these changes Apr 27, 2022

View reviewed changes

jmagman approved these changes Apr 28, 2022

View reviewed changes

LongCatIsLooong added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 28, 2022

fluttergithubbot merged commit 9d784a8 into flutter:main Apr 28, 2022

LongCatIsLooong deleted the put-uitextinteraction-back branch April 28, 2022 16:46

engine-flutter-autoroll mentioned this pull request Apr 29, 2022

Roll Engine from dc2a7da549d2 to 649fe1080d56 (14 revisions) flutter/flutter#102795

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 29, 2022

9d784a8 [iOS] Add UITextInteraction only when inputDelegate is nil (

f98c4c2

flutter/engine#32881)

danagbemava-nc mentioned this pull request Jul 4, 2022

"Composing" of TextEditingValue is revived after TextEditingController.clear() flutter/flutter#107040

Closed

[iOS] Add UITextInteraction only when inputDelegate is nil #32881

[iOS] Add UITextInteraction only when inputDelegate is nil #32881

Uh oh!

Conversation

LongCatIsLooong commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Apr 25, 2022

Uh oh!

fbcouch commented Apr 25, 2022

Uh oh!

LongCatIsLooong commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Apr 26, 2022

Uh oh!

fbcouch commented Apr 26, 2022

Uh oh!

LongCatIsLooong commented Apr 26, 2022

Uh oh!

LongCatIsLooong commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LongCatIsLooong Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Apr 27, 2022

Uh oh!

fbcouch commented Apr 27, 2022

Uh oh!

LongCatIsLooong Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

jmagman Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

jmagman Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[iOS] Add `UITextInteraction` only when `inputDelegate` is nil #32881

[iOS] Add `UITextInteraction` only when `inputDelegate` is nil #32881

LongCatIsLooong commented Apr 23, 2022 •

edited

Loading

LongCatIsLooong commented Apr 26, 2022 •

edited

Loading

LongCatIsLooong commented Apr 27, 2022 •

edited

Loading