[iOS] TextInputPlugin should batch TextEditingDeltas and dispatch on the next runLoop #43267

Renzo-Olivares · 2023-06-27T17:01:21Z

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.

Renzo-Olivares · 2023-07-06T19:03:02Z

Some observations:

When typing a sequence that results in an em-dash:
- followed by another - will be transformed into —. deleteBackwards is called to remove the initial - and then insertText is called to insert the em dash. Before this change the deleteBackwards and insertText would be sent as individual delta batches, i.e. updateEditingValueWithDeltas would be called twice. Sending individual batches would cause the editing state to become out of sync in some cases where the editing state is also changed on the framework side in the middle of this em dash transformation. After this change deleteBackwards and insertText are sent in one batch, i.e. a single call to updateEditingValueWithDeltas. This gives the user a chance to process an entire batch of deltas before making any edits to the editing state.

I also tried to observe what a native UITextField does in this situation by listening to the textDidChange event. I only observed one textDidChange call when doing an em-dash transformation.

[self.textField addTarget:self action:@selector(textFieldDidChange:) forControlEvents:UIControlEventEditingChanged];

justinmc

LGTM but @LongCatIsLooong should definitely take a look.

The run loop thing looks like an elegant solution to this problem. It seems like that's how things like the em dash are supposed to be handled.

justinmc · 2023-07-06T19:59:55Z

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

 - (BOOL)shouldChangeTextInRange:(UITextRange*)range replacementText:(NSString*)text {
  // `temporarilyDeletedComposedCharacter` should only be used during a single text change session.
-  // So it needs to be cleared at the start of each text editting session.
+  // So it needs to be cleared at the start of each text editing session.


justinmc · 2023-07-06T20:12:21Z

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

+  done = false;
+  XCTAssertFalse(done);
+  CFRunLoopPerformBlock(CFRunLoopGetMain(), (__bridge CFStringRef)runLoopMode, ^{
+    done = true;
+  });
+
+  while (!done) {
+    // Each invocation will handle one source.
+    CFRunLoopRunInMode((__bridge CFStringRef)runLoopMode, 0, true);
+  }
+  XCTAssertTrue(done);


Would it be any cleaner to abstract this code to a function or something? It's kind of a bummer that we have to clear the runloop all over the place like this, but it makes sense. The next time a test is added here, it should be obvious that this is necessary since it's all over.

justinmc · 2023-07-06T20:12:58Z

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

    _markedRect = kInvalidFirstRect;
    _cachedFirstRect = kInvalidFirstRect;
    _scribbleInteractionStatus = FlutterScribbleInteractionStatusNone;
+    _pendingDeltas = [[NSMutableArray alloc] init];


Do you have to release this somewhere? I'm just guessing since I'm no objective c expert.

No release here is needed as this file is ARC.

stuartmorgan-g · 2023-07-10T19:14:30Z

Please wait for @cyanglaz to review as well for ObjC expertise.

cyanglaz · 2023-07-10T19:15:50Z

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

    _markedRect = kInvalidFirstRect;
    _cachedFirstRect = kInvalidFirstRect;
    _scribbleInteractionStatus = FlutterScribbleInteractionStatusNone;
+    _pendingDeltas = [[NSMutableArray alloc] init];


No release here is needed as this file is ARC.

cyanglaz · 2023-07-10T19:24:44Z

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

 @property(nonatomic, copy) NSDictionary* markedTextStyle;
 @property(nonatomic, weak) id<UITextInputDelegate> inputDelegate;
+@property(nonatomic, strong) NSMutableArray* pendingDeltas;
+@property(nonatomic, readwrite, copy) NSString* customRunLoopMode;


nit: readwrite is the default so it is not necessary here.

It looks like you don't need to expose the run loop mode? IIRC in the macOS implementation it was only exposed because there was a leak in the tests so we couldn't use async expectations.

Now that I think about it, do you need to use the runloop API? dispatch_async does the same thing right?

Yes I think dispatch_async should accomplish the same thing and we wouldn't need to set the mode either. I can try that.

cyanglaz · 2023-07-10T21:39:58Z

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

+
+        [self.textInputDelegate flutterTextInputView:self
+                                 updateEditingClient:_textInputClient
+                                           withDelta:deltas];


I think it is fine but we should confirm that the framework can handle any delta batches. cc @LongCatIsLooong

Are you referring to the frameworks ability to receive/apply delta batches?

LongCatIsLooong · 2023-07-11T01:09:10Z

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

-  NSDictionary* deltas = @{
-    @"deltas" : @[ deltaToFramework ],
-  };
+  [_pendingDeltas addObject:deltaToFramework];


Now that there's batching, what happens if oldText is different in the same batch? For example when there's a framework change happened in between?

Do you mean in this situation:

Batch: Insertion, Deletion, Insertion
After first insertion delta is applied, the framework changes the editing state in response to the insertion?

If the engine applies an update from the framework, after the block is scheduled and before the block is invoked. In theory that can happen right?

I see what you mean. In theory that can happen something like the following example.
Type A followed by B:

Engine types A.
Engine schedules delta block to run.
Framework types C for x reason.
Engine has C.
Engine types B.
Engine has CB and sends to framework, Expected ACB?

Deltas sent:
InsertionA, oldText: ''
InsertionB, oldText:'C'.

Here the user will not really be able to recover. I don't think this is too different from the IME-Framework out of sync issues we usually face. What do you think about creating some method that signals the framework that a batch has been scheduled, and that it probably is not safe to make any framework side changes during this time.

Another thing to note is that Android already does batching of TextEditingDeltas on its TextInputPlugin, so maybe this is an issue we already face on that end.

In your example the batch is going to be partially applied by the framework right? The framework will apply changes in the batch until the point where oldText mismatches? Shouldn't it be all or nothing, when there's on inconsistent "oldText", when framework rejects the entire batch?

I think the framework will ignore any difference in the oldText. https://github.com/flutter/flutter/blob/f468f3366c26a5092eb964a230ce7892fda8f2f8/packages/flutter/lib/src/services/text_editing_delta.dart#L324-L334 . A TextEditingDelta applies the delta to the oldText to follow a last write wins policy. A user can still prevent the application of a delta by checking the oldText with their current value themselves before calling apply. A user can make it all or nothing but we don't do this by default.

Sorry missed the reply.

A user can make it all or nothing but we don't do this by default

So the current implementation in the framework doesn't guarantee that a delta batch is atomically applied? That sounds like the same issue could still happen if a framework edit is sent to the engine while the engine is replacing a double-dash?

Oh the framework edit is going to be completely ignored in that case?

So the current implementation in the framework doesn't guarantee that a delta batch is atomically applied? That sounds like the same issue could still happen if a framework edit is sent to the engine while the engine is replacing a double-dash?

Yes in theory a framework edit can still come in during the batch accumulation and interrupt the double-dash replacement. I don't think we can avoid this case but I don't think it is less likely. Before this change the double-dash replacement sequence would come in as 3 separate delta batches, so the user could potentially react to the 1st delta batch and make some edit and disrupt the sequence. After this change the sequence comes in as one delta batch, so the user can avoid making an edit until they fully apply the delta batch.

Oh the framework edit is going to be completely ignored in that case?

No, it is still applied by the engine. In the example I stated above the edit that is ignored is the 1st engine edit that types 'A'.

LongCatIsLooong · 2023-07-11T01:10:54Z

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

 @property(nonatomic, copy) NSDictionary* markedTextStyle;
 @property(nonatomic, weak) id<UITextInputDelegate> inputDelegate;
+@property(nonatomic, strong) NSMutableArray* pendingDeltas;
+@property(nonatomic, readwrite, copy) NSString* customRunLoopMode;


It looks like you don't need to expose the run loop mode? IIRC in the macOS implementation it was only exposed because there was a leak in the tests so we couldn't use async expectations.

LongCatIsLooong · 2023-07-11T01:15:47Z

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

+                                  : kCFRunLoopCommonModes;
+
+    CFRunLoopPerformBlock(CFRunLoopGetMain(), runLoopMode, ^{
+      if (_pendingDeltas.count > 0) {


Is it possible for the text input plugin to already be released when the block is invoked?

I'm not sure, i'm not too familiar with objc memory management. I would think it is possible since the block is run asynchronously. Is this something we should handle?

use __weak maybe?

…elta block has been invoked

LongCatIsLooong · 2023-07-20T19:39:22Z

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

+    __weak FlutterTextInputView* weakSelf = self;
+    dispatch_async(dispatch_get_main_queue(), ^{
+      __strong FlutterTextInputView* strongSelf = weakSelf;
+      NSAssert(strongSelf,


I think this is something that can happen legitimately?

LongCatIsLooong · 2023-07-20T19:45:40Z

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

                             }];
 }

+- (void)dispatchAsyncOnMain {


flushScheduledAsyncBlocks, or some such?

LongCatIsLooong · 2023-07-20T19:46:36Z

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

+  [inputView insertText:@"—"];
+
+  [self dispatchAsyncOnMain];
+  // __block bool done = false;


remove commented code here?

… lost

chinmaygarde · 2023-07-27T20:01:27Z

May I add the autosubmit tag?

…atch on the next runLoop (flutter/engine#43267)

flutter/engine@284771d...196474b 2023-07-27 skia-flutter-autoroll@skia.org Roll Skia from 20108acef7d2 to d3a7efb77af3 (2 revisions) (flutter/engine#44082) 2023-07-27 rmolivares@renzo-olivares.dev [iOS] TextInputPlugin should batch TextEditingDeltas and dispatch on the next runLoop (flutter/engine#43267) 2023-07-27 skia-flutter-autoroll@skia.org Roll Skia from b522808eb0af to 20108acef7d2 (3 revisions) (flutter/engine#44079) 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 jonahwilliams@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…1443) flutter/engine@284771d...196474b 2023-07-27 skia-flutter-autoroll@skia.org Roll Skia from 20108acef7d2 to d3a7efb77af3 (2 revisions) (flutter/engine#44082) 2023-07-27 rmolivares@renzo-olivares.dev [iOS] TextInputPlugin should batch TextEditingDeltas and dispatch on the next runLoop (flutter/engine#43267) 2023-07-27 skia-flutter-autoroll@skia.org Roll Skia from b522808eb0af to 20108acef7d2 (3 revisions) (flutter/engine#44079) 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 jonahwilliams@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…the next runLoop (flutter#43267) Fixes flutter/flutter#129061

github-actions bot added the platform-ios label Jun 27, 2023

Renzo-Olivares marked this pull request as ready for review June 30, 2023 21:16

Renzo-Olivares force-pushed the ios-deltas-runloop branch from 1a60079 to a02dcc7 Compare July 6, 2023 18:45

Renzo-Olivares requested review from LongCatIsLooong and justinmc July 6, 2023 19:03

justinmc approved these changes Jul 6, 2023

View reviewed changes

stuartmorgan-g requested a review from cyanglaz July 10, 2023 19:14

cyanglaz reviewed Jul 10, 2023

View reviewed changes

LongCatIsLooong reviewed Jul 11, 2023

View reviewed changes

Renzo-Olivares added 18 commits July 19, 2023 14:31

batch deltas and wait for runLoop to dispatch them

da9199b

add test

db1bb62

formatting

c152b91

fix batching test

6fb0f34

formatting

ab92acd

formatting

ef21759

formatting

0de5cf4

formatting

91c97d5

formatting

de0450e

fix tests

0d8ea86

Fix tests

87c698e

formatting

da8cad0

updates

c94f723

update test

440e5b4

formatting

c473bab

Address some reviewer comments

c9f3be5

formatting

0f08d80

extract runLoop code into a re-usable function

f2ed64a

Renzo-Olivares force-pushed the ios-deltas-runloop branch from c316d2b to f2ed64a Compare July 19, 2023 21:31

Renzo-Olivares added 3 commits July 19, 2023 15:43

use dispatch async instead of runLoop

2d461e9

use __weak in case FlutterTextInputView has been deallocated before d…

770a6a9

…elta block has been invoked

formatting

fb90de5

Renzo-Olivares requested a review from LongCatIsLooong July 20, 2023 18:16

LongCatIsLooong reviewed Jul 20, 2023

View reviewed changes

Renzo-Olivares added 2 commits July 20, 2023 13:01

address comments

77433a3

Delta block should fail gracefully when FlutterTextInputView has been…

ad67e50

… lost

LongCatIsLooong approved these changes Jul 26, 2023

View reviewed changes

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 27, 2023

auto-submit bot merged commit 78171a6 into flutter:main Jul 27, 2023

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

Roll Flutter Engine from 284771da323c to 196474bd96b0 (3 revisions) flutter/flutter#131443

Merged

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

78171a6 [iOS] TextInputPlugin should batch TextEditingDeltas and disp…

f00cdb5

…atch on the next runLoop (flutter/engine#43267)

angelosilvestre mentioned this pull request Jul 30, 2023

[SuperEditor][Mobile][Bug] Two consecutive dashes throws Exception Flutter-Bounty-Hunters/super_editor#1147

Closed

gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023

[iOS] TextInputPlugin should batch TextEditingDeltas and dispatch on …

6f548bf

…the next runLoop (flutter#43267) Fixes flutter/flutter#129061

Renzo-Olivares mentioned this pull request Nov 8, 2023

[iOS] - Dictation deltas don't respect app-side content updates flutter/flutter#120724

Open

[iOS] TextInputPlugin should batch TextEditingDeltas and dispatch on the next runLoop #43267

[iOS] TextInputPlugin should batch TextEditingDeltas and dispatch on the next runLoop #43267

Uh oh!

Conversation

Renzo-Olivares commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Renzo-Olivares commented Jul 6, 2023

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jul 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Jul 19, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Jul 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Renzo-Olivares commented Jun 27, 2023 •

edited

Loading

Renzo-Olivares Jul 19, 2023 •

edited

Loading

Renzo-Olivares Jul 27, 2023 •

edited

Loading