[iOS TextInputPlugin] adjust the markedTextRange when replacing text. #46603

LongCatIsLooong · 2023-10-06T00:58:29Z

Fixes flutter/flutter#130880 , by making sure when the text is cleared the markedTextRange is cleared with it.

[UITextField replaceRange:withText:] always sets the selection range to the end of the replacement text, and removes the current markedTextRange. This PR makes the input plugin do the same in the replaceRange:withText: implementation.

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 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.

stuartmorgan-g · 2023-10-11T17:25:45Z

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

+  // documentation but UITextField always sets markedTextRange to nil,
+  // and collapses the selection to the end of the new replacement text.
+  range.location += text.length;
+  range.length = 0;


Why is this stomping the parameter instead of making a new range (and then replacing that new range with the clamped version)? Writing to parameters is generally bad for readability.

stuartmorgan-g · 2023-10-11T17:29:24Z

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


-  NSUInteger selectionLocation = markedSelectedRange.location + markedTextRange.location;
-  selectedRange = NSMakeRange(selectionLocation, markedSelectedRange.length);
+  markedSelectedRange.location += newMarkedRange.location;


Please use a new variable rather than overwriting the param.

stuartmorgan-g · 2023-10-11T17:30:59Z

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

 - (BOOL)isVisibleToAutofill;
 - (id<FlutterTextInputDelegate>)textInputDelegate;
 - (void)configureWithDictionary:(NSDictionary*)configuration;
+- (void)replaceRangeLocal:(NSRange)range withText:(NSString*)text;


Why can't the behavior be tested via the text input protocol, rather than internal implementation details?

hellohuanlin · 2023-10-12T19:09:32Z

I may need some help understanding this change. Could you update the PR description with more details?

LongCatIsLooong · 2023-10-12T22:41:49Z

PR description updated.

hellohuanlin

Overall makes sense. Just 1 question

hellohuanlin · 2023-10-13T17:42:15Z

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

+                                           ? currentMarkedTextRange.range
+                                           : _selectedTextRange.range;
+  // No need to call replaceRangeLocal as this method always adjusts the
+  // selected/marked text ranges anyways.


I am a bit lost here. Can you explain more why we had this before, and we removed it now?

replaceRangeLocal also changes the selected text range and the marked text range, based on the original selected text range and the marked text range and the text replacement. It's not needed in this method because with this method the caller always have to specify a new selected range and marked range anyways.

Is the the same reason we removed the markedText state?

markedText was never used.

stuartmorgan-g

Code LGTM

Just to be safe, have you checked any issues/behaviors referenced in the PRs that created the logic you are replacing, to make sure nothing is being regressed (that wasn't sufficiently unit tested)?

…xtRange

LongCatIsLooong · 2023-10-18T20:19:05Z

i've tested this patch on ios17 simulator and it worked fine, and it does fix the framework assert.

…cing text. (flutter/engine#46603)

…136844) flutter/engine@6caee32...b67edb0 2023-10-18 [email protected] [iOS TextInputPlugin] adjust the markedTextRange when replacing text. (flutter/engine#46603) 2023-10-18 [email protected] Reland: Remove the frontend server wrapper (flutter/engine#47010) 2023-10-18 [email protected] Roll Skia from 9880c4006735 to ef0e93524e7d (1 revision) (flutter/engine#47069) 2023-10-18 [email protected] Roll Skia from 523f04f1a898 to 9880c4006735 (1 revision) (flutter/engine#47067) 2023-10-18 [email protected] Roll Skia from ccd07c6f5042 to 523f04f1a898 (1 revision) (flutter/engine#47065) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll 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 Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…#46603) Fixes flutter/flutter#130880 , by making sure when the text is cleared the `markedTextRange` is cleared with it. `[UITextField replaceRange:withText:]` always sets the selection range to the end of the replacement text, and removes the current `markedTextRange`. This PR makes the input plugin do the same in the `replaceRange:withText:` implementation. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the platform-ios label Oct 6, 2023

LongCatIsLooong force-pushed the ios-adjust-markedTextRange branch 2 times, most recently from a0f4ec6 to 2bf714a Compare October 6, 2023 01:20

Adjust the markedTextRange when replacing text.

8471501

LongCatIsLooong force-pushed the ios-adjust-markedTextRange branch 5 times, most recently from 9fb835d to 8ddf821 Compare October 8, 2023 04:59

Fix tests

12a3d68

LongCatIsLooong force-pushed the ios-adjust-markedTextRange branch from 8ddf821 to 12a3d68 Compare October 9, 2023 03:29

add a test

62ae972

LongCatIsLooong force-pushed the ios-adjust-markedTextRange branch from 86b3df7 to 62ae972 Compare October 9, 2023 17:58

LongCatIsLooong marked this pull request as ready for review October 9, 2023 18:00

LongCatIsLooong requested review from cyanglaz, hellohuanlin and stuartmorgan-g and removed request for stuartmorgan-g October 9, 2023 18:01

stuartmorgan-g suggested changes Oct 11, 2023

View reviewed changes

LongCatIsLooong force-pushed the ios-adjust-markedTextRange branch 2 times, most recently from 0f23d4d to b1d2846 Compare October 12, 2023 20:59

LongCatIsLooong requested a review from stuartmorgan-g October 12, 2023 22:41

hellohuanlin reviewed Oct 13, 2023

View reviewed changes

stuartmorgan-g approved these changes Oct 13, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into ios-adjust-markedTe…

97f0a33

…xtRange

LongCatIsLooong force-pushed the ios-adjust-markedTextRange branch from b1d2846 to 97f0a33 Compare October 17, 2023 23:28

review

dbebed5

hellohuanlin approved these changes Oct 18, 2023

View reviewed changes

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2023

auto-submit bot merged commit b67edb0 into flutter:main Oct 18, 2023

engine-flutter-autoroll mentioned this pull request Oct 18, 2023

Roll Flutter Engine from 6caee3236d37 to b67edb05d3f7 (5 revisions) flutter/flutter#136844

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 18, 2023

b67edb05d [iOS TextInputPlugin] adjust the markedTextRange when repla…

1ec052c

…cing text. (flutter/engine#46603)

LongCatIsLooong deleted the ios-adjust-markedTextRange branch October 18, 2023 21:46

[iOS TextInputPlugin] adjust the markedTextRange when replacing text. #46603

[iOS TextInputPlugin] adjust the markedTextRange when replacing text. #46603

Uh oh!

Conversation

LongCatIsLooong commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Oct 12, 2023

Uh oh!

LongCatIsLooong commented Oct 12, 2023

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LongCatIsLooong commented Oct 6, 2023 •

edited

Loading