From 2a27aa179c939efd6d8382d92e6523043b10f977 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Wed, 15 Jul 2020 14:32:16 -0700 Subject: [PATCH 01/16] Turn -1s into 0s instead of letting them overflow and cause weird results --- .../framework/Source/FlutterTextInputPlugin.mm | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index 7130469ee7bd4..bacd8eccf69fd 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -214,6 +214,14 @@ static UITextContentType ToUITextContentType(NSArray* hints) { return autofill == nil ? nil : autofill[@"uniqueIdentifier"]; } +// Read a signed integer from state into an unsigned integer. This is needed for +// NSRange locations, which are unsigned integers, but are read in from signed +// integers in state. These are -1 when there is no selection specified, but +// they should be interpreted as 0 instead of overflowing. +static NSUInteger readInt(NSInteger value) { + return value < 0 ? 0 : value; +} + #pragma mark - FlutterTextPosition @implementation FlutterTextPosition @@ -334,8 +342,8 @@ - (BOOL)setTextInputState:(NSDictionary*)state { [self.text setString:newText]; } BOOL needsEditingStateUpdate = textChanged; - NSInteger composingBase = [state[@"composingBase"] intValue]; - NSInteger composingExtent = [state[@"composingExtent"] intValue]; + NSUInteger composingBase = readInt([state[@"composingBase"] intValue]); + NSUInteger composingExtent = readInt([state[@"composingExtent"] intValue]); NSRange composingRange = [self clampSelection:NSMakeRange(MIN(composingBase, composingExtent), ABS(composingBase - composingExtent)) forText:self.text]; @@ -347,8 +355,8 @@ - (BOOL)setTextInputState:(NSDictionary*)state { : [newMarkedRange isEqualTo:(FlutterTextRange*)self.markedTextRange]; self.markedTextRange = newMarkedRange; - NSInteger selectionBase = [state[@"selectionBase"] intValue]; - NSInteger selectionExtent = [state[@"selectionExtent"] intValue]; + NSUInteger selectionBase = readInt([state[@"selectionBase"] intValue]); + NSUInteger selectionExtent = readInt([state[@"selectionExtent"] intValue]); NSRange selectedRange = [self clampSelection:NSMakeRange(MIN(selectionBase, selectionExtent), ABS(selectionBase - selectionExtent)) forText:self.text]; From 9ab67dd88f1ba4a9ead6743da0620325cdee73d9 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Thu, 16 Jul 2020 13:36:49 -0700 Subject: [PATCH 02/16] For invalid selection, set it to nil instead of zero --- .../Source/FlutterTextInputPlugin.mm | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index bacd8eccf69fd..2bec116cc8d86 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -214,14 +214,6 @@ static UITextContentType ToUITextContentType(NSArray* hints) { return autofill == nil ? nil : autofill[@"uniqueIdentifier"]; } -// Read a signed integer from state into an unsigned integer. This is needed for -// NSRange locations, which are unsigned integers, but are read in from signed -// integers in state. These are -1 when there is no selection specified, but -// they should be interpreted as 0 instead of overflowing. -static NSUInteger readInt(NSInteger value) { - return value < 0 ? 0 : value; -} - #pragma mark - FlutterTextPosition @implementation FlutterTextPosition @@ -342,8 +334,8 @@ - (BOOL)setTextInputState:(NSDictionary*)state { [self.text setString:newText]; } BOOL needsEditingStateUpdate = textChanged; - NSUInteger composingBase = readInt([state[@"composingBase"] intValue]); - NSUInteger composingExtent = readInt([state[@"composingExtent"] intValue]); + NSInteger composingBase = [state[@"composingBase"] intValue]; + NSInteger composingExtent = [state[@"composingExtent"] intValue]; NSRange composingRange = [self clampSelection:NSMakeRange(MIN(composingBase, composingExtent), ABS(composingBase - composingExtent)) forText:self.text]; @@ -355,8 +347,8 @@ - (BOOL)setTextInputState:(NSDictionary*)state { : [newMarkedRange isEqualTo:(FlutterTextRange*)self.markedTextRange]; self.markedTextRange = newMarkedRange; - NSUInteger selectionBase = readInt([state[@"selectionBase"] intValue]); - NSUInteger selectionExtent = readInt([state[@"selectionExtent"] intValue]); + NSInteger selectionBase = [state[@"selectionBase"] intValue]; + NSInteger selectionExtent = [state[@"selectionExtent"] intValue]; NSRange selectedRange = [self clampSelection:NSMakeRange(MIN(selectionBase, selectionExtent), ABS(selectionBase - selectionExtent)) forText:self.text]; @@ -437,6 +429,13 @@ - (void)setSelectedTextRangeLocal:(UITextRange*)selectedTextRange { } } +// Clear the selected text range, without notifying the framework. +- (void)removeSelectedTextRangeLocal { + UITextRange* oldSelectedRange = _selectedTextRange; + _selectedTextRange = nil; + [oldSelectedRange release]; +} + - (void)setSelectedTextRange:(UITextRange*)selectedTextRange { [self setSelectedTextRangeLocal:selectedTextRange]; [self updateEditingState]; From 65a27d862400716230d70a20e8c86825c70e4c52 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Thu, 16 Jul 2020 17:57:10 -0700 Subject: [PATCH 03/16] Avoid bugs where a range is expected by setting range to 0,0 instead of nil --- .../darwin/ios/framework/Source/FlutterTextInputPlugin.mm | 7 ------- 1 file changed, 7 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index 2bec116cc8d86..7130469ee7bd4 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -429,13 +429,6 @@ - (void)setSelectedTextRangeLocal:(UITextRange*)selectedTextRange { } } -// Clear the selected text range, without notifying the framework. -- (void)removeSelectedTextRangeLocal { - UITextRange* oldSelectedRange = _selectedTextRange; - _selectedTextRange = nil; - [oldSelectedRange release]; -} - - (void)setSelectedTextRange:(UITextRange*)selectedTextRange { [self setSelectedTextRangeLocal:selectedTextRange]; [self updateEditingState]; From c9baf78603fe7bb8a217c7680fdf7814a51927b0 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Thu, 16 Jul 2020 18:37:25 -0700 Subject: [PATCH 04/16] Avoid race condition by debouncing editing calls to framework --- .../editing/InputConnectionAdaptor.java | 5 ++++ .../plugin/editing/TextInputPlugin.java | 7 ++++++ .../Source/FlutterTextInputPlugin.mm | 24 +++++++++++++++---- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java b/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java index 29b3fb859e1ef..cbb2e58a13963 100644 --- a/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java +++ b/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java @@ -154,6 +154,8 @@ private void updateEditingState() { currentValue.composingStart, currentValue.composingEnd); + // TODO(justinmc): This must be where the engine sends a value to the frmwk. + Log.e("justin", "Android engine sending to framework " + currentValue.text); textInputChannel.updateEditingState( mClient, currentValue.text, @@ -188,6 +190,9 @@ public boolean beginBatchEdit() { @Override public boolean endBatchEdit() { + // TODO(justinmc): This is how Android is able to batch requests. + // Should iOS do the same? Or is there another way to work around this? + Log.e("justin", "endBatchEdit"); boolean result = super.endBatchEdit(); mBatchCount--; updateEditingState(); diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index 716246d189299..e1dfc24e79bd0 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -26,6 +26,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; +import io.flutter.Log; import io.flutter.embedding.engine.systemchannels.TextInputChannel; import io.flutter.plugin.platform.PlatformViewsController; import java.util.HashMap; @@ -95,6 +96,11 @@ public void setPlatformViewClient(int platformViewId) { @Override public void setEditingState(TextInputChannel.TextEditState editingState) { + // TODO(justinmc): I believe this is where the framework sends a + // value to the engine. + // Somehow between here and when it sends back to the framework, it + // bundles the two updates. + Log.e("justin", "Android engine receiving from framework " + editingState.text); setTextInputEditingState(mView, editingState); } @@ -381,6 +387,7 @@ private void applyStateToSelection(TextInputChannel.TextEditState state) { @VisibleForTesting void setTextInputEditingState(View view, TextInputChannel.TextEditState state) { + // TODO(justinmc): Step 2. // Always replace the contents of mEditable if the text differs if (!state.text.equals(mEditable.toString())) { mEditable.replace(0, mEditable.length(), state.text); diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index 7130469ee7bd4..de1d99744f09a 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -277,6 +277,7 @@ @implementation FlutterTextInputView { int _textInputClient; const char* _selectionAffinity; FlutterTextRange* _selectedTextRange; + NSDictionary* _latestState; } @synthesize tokenizer = _tokenizer; @@ -783,12 +784,25 @@ - (void)updateEditingState { @"composingExtent" : @(composingExtent), @"text" : [NSString stringWithString:self.text], }; + _latestState = state; + + // Debounce calls to updateEditingClient. This makes iOS text editing behave + // more similarly to Android's, which has built-in event batching, and avoids + // race conditions. The delay value was chosen to be imperceptible by the user + // but still long enough to allow the framework to respond with formatting + // updates, thereby avoiding common race conditions. + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 10000000), dispatch_get_main_queue(), ^(void){ + if (state != _latestState) { + return; + } + _latestState = nil; - if (_textInputClient == 0 && _autofillId != nil) { - [_textInputDelegate updateEditingClient:_textInputClient withState:state withTag:_autofillId]; - } else { - [_textInputDelegate updateEditingClient:_textInputClient withState:state]; - } + if (_textInputClient == 0 && _autofillId != nil) { + [_textInputDelegate updateEditingClient:_textInputClient withState:state withTag:_autofillId]; + } else { + [_textInputDelegate updateEditingClient:_textInputClient withState:state]; + } + }); } - (BOOL)hasText { From 145fa2713e494d9784fb20e9cc340aa787e2bab5 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Tue, 21 Jul 2020 11:07:15 -0700 Subject: [PATCH 05/16] Test that calls are batched --- .../Source/FlutterTextInputPluginTest.m | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index b2e6981d2b99a..c4a0a912ff013 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -274,4 +274,36 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { [inputView unmarkText]; XCTAssertEqual(updateCount, 6); } + +- (void)testBackToBackUITextInputCallsUpdateEditingStateOnce { + FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; + inputView.textInputDelegate = engine; + + __block int updateCount = 0; + OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) + .andDo(^(NSInvocation* invocation) { + updateCount++; + }); + + [inputView insertText:@"text to insert"]; + [inputView insertText:@"text to insert updated"]; + // Update the framework exactly once. + XCTAssertEqual(updateCount, 1); + + [inputView deleteBackward]; + XCTAssertEqual(updateCount, 2); + + inputView.selectedTextRange = [FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)]; + XCTAssertEqual(updateCount, 3); + + [inputView replaceRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)] + withText:@"replace text"]; + XCTAssertEqual(updateCount, 4); + + [inputView setMarkedText:@"marked text" selectedRange:NSMakeRange(0, 1)]; + XCTAssertEqual(updateCount, 5); + + [inputView unmarkText]; + XCTAssertEqual(updateCount, 6); +} @end From 5de9e67be223097eeade41a0c18ec1ab65b403b5 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Tue, 21 Jul 2020 11:21:24 -0700 Subject: [PATCH 06/16] Clean up logging and comments --- .../io/flutter/plugin/editing/InputConnectionAdaptor.java | 5 ----- .../android/io/flutter/plugin/editing/TextInputPlugin.java | 7 ------- 2 files changed, 12 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java b/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java index cbb2e58a13963..29b3fb859e1ef 100644 --- a/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java +++ b/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java @@ -154,8 +154,6 @@ private void updateEditingState() { currentValue.composingStart, currentValue.composingEnd); - // TODO(justinmc): This must be where the engine sends a value to the frmwk. - Log.e("justin", "Android engine sending to framework " + currentValue.text); textInputChannel.updateEditingState( mClient, currentValue.text, @@ -190,9 +188,6 @@ public boolean beginBatchEdit() { @Override public boolean endBatchEdit() { - // TODO(justinmc): This is how Android is able to batch requests. - // Should iOS do the same? Or is there another way to work around this? - Log.e("justin", "endBatchEdit"); boolean result = super.endBatchEdit(); mBatchCount--; updateEditingState(); diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index e1dfc24e79bd0..716246d189299 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -26,7 +26,6 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; -import io.flutter.Log; import io.flutter.embedding.engine.systemchannels.TextInputChannel; import io.flutter.plugin.platform.PlatformViewsController; import java.util.HashMap; @@ -96,11 +95,6 @@ public void setPlatformViewClient(int platformViewId) { @Override public void setEditingState(TextInputChannel.TextEditState editingState) { - // TODO(justinmc): I believe this is where the framework sends a - // value to the engine. - // Somehow between here and when it sends back to the framework, it - // bundles the two updates. - Log.e("justin", "Android engine receiving from framework " + editingState.text); setTextInputEditingState(mView, editingState); } @@ -387,7 +381,6 @@ private void applyStateToSelection(TextInputChannel.TextEditState state) { @VisibleForTesting void setTextInputEditingState(View view, TextInputChannel.TextEditState state) { - // TODO(justinmc): Step 2. // Always replace the contents of mEditable if the text differs if (!state.text.equals(mEditable.toString())) { mEditable.replace(0, mEditable.length(), state.text); From a97991191aadc21cd2ade048f12112769f9a005b Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Wed, 22 Jul 2020 13:07:24 -0700 Subject: [PATCH 07/16] WIP Trying to fix tests --- .../Source/FlutterTextInputPlugin.mm | 35 +++++++++++++++---- .../Source/FlutterTextInputPluginTest.m | 27 ++++++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index de1d99744f09a..1b29a68974985 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -784,27 +784,47 @@ - (void)updateEditingState { @"composingExtent" : @(composingExtent), @"text" : [NSString stringWithString:self.text], }; - _latestState = state; // Debounce calls to updateEditingClient. This makes iOS text editing behave // more similarly to Android's, which has built-in event batching, and avoids // race conditions. The delay value was chosen to be imperceptible by the user // but still long enough to allow the framework to respond with formatting // updates, thereby avoiding common race conditions. + if (_latestState == nil) { + _latestState = state; + NSLog(@"justin leading edge updateEditingClient for %@", _latestState[@"text"]); + [self updateEditingClient]; + NSLog(@"justin 1"); + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 10000000), dispatch_get_main_queue(), ^(void){ + NSLog(@"justin 2"); + if (_latestState == state) { + NSLog(@"justin updateEditingClient clear leading edge for %@", _latestState[@"text"]); + _latestState = nil; + } + }); + NSLog(@"justin 3"); + return; + } + _latestState = state; + NSLog(@"justin updateEditingClient start trailing edge timer for _latestState for %@", _latestState[@"text"]); dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 10000000), dispatch_get_main_queue(), ^(void){ if (state != _latestState) { return; } + NSLog(@"justin trailing edge updateEditingClient for %@", _latestState[@"text"]); + [self updateEditingClient]; _latestState = nil; - - if (_textInputClient == 0 && _autofillId != nil) { - [_textInputDelegate updateEditingClient:_textInputClient withState:state withTag:_autofillId]; - } else { - [_textInputDelegate updateEditingClient:_textInputClient withState:state]; - } }); } +- (void)updateEditingClient { + if (_textInputClient == 0 && _autofillId != nil) { + [_textInputDelegate updateEditingClient:_textInputClient withState:_latestState withTag:_autofillId]; + } else { + [_textInputDelegate updateEditingClient:_textInputClient withState:_latestState]; + } +} + - (BOOL)hasText { return self.text.length > 0; } @@ -1036,6 +1056,7 @@ + (void)setupInputView:(FlutterTextInputView*)inputView - (void)setTextInputEditingState:(NSDictionary*)state { if ([_activeView setTextInputState:state]) { + NSLog(@"justin setTextInputEditingState for %@", state[@"text"]); [_activeView updateEditingState]; } } diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index c4a0a912ff013..aa00a6667c318 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -245,20 +245,40 @@ - (void)testTextRangeFromPositionMatchesUITextViewBehavior { } - (void)testUITextInputCallsUpdateEditingStateOnce { + NSLog(@"justin start testUITextInputCallsUpdateEditingStateOnce"); FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; + XCTestExpectation* expectation = [self expectationWithDescription:@"called updateEditingClient"]; + XCTestExpectation* expectation3 = [self expectationWithDescription:@"called four times"]; + NSString* text; + __block bool calledSinceChange = false; __block int updateCount = 0; OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) .andDo(^(NSInvocation* invocation) { + XCTAssertEqual(calledSinceChange, false); + calledSinceChange = true; updateCount++; + if (updateCount == 1) { + [expectation fulfill]; + } else if (updateCount == 3) { + [expectation3 fulfill]; + } }); + text = @"text to insert"; + NSLog(@"justin testUITextInputCallsUpdateEditingStateOnce insertText 1"); [inputView insertText:@"text to insert"]; + NSLog(@"justin testUITextInputCallsUpdateEditingStateOnce insertedText 1"); // Update the framework exactly once. XCTAssertEqual(updateCount, 1); + calledSinceChange = false; + expectation = [self expectationWithDescription:@"called updateEditingClient"]; + text = @"text to inser"; [inputView deleteBackward]; + XCTAssertEqual(updateCount, 1); + [self waitForExpectations:@[expectation] timeout:1]; XCTAssertEqual(updateCount, 2); inputView.selectedTextRange = [FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)]; @@ -266,15 +286,21 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { [inputView replaceRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)] withText:@"replace text"]; + XCTAssertEqual(updateCount, 3); + [self waitForExpectations:@[expectation3] timeout:1]; XCTAssertEqual(updateCount, 4); + /* [inputView setMarkedText:@"marked text" selectedRange:NSMakeRange(0, 1)]; + [self waitForExpectations:@[expectation4] timeout:1]; XCTAssertEqual(updateCount, 5); [inputView unmarkText]; XCTAssertEqual(updateCount, 6); + */ } +/* - (void)testBackToBackUITextInputCallsUpdateEditingStateOnce { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; @@ -306,4 +332,5 @@ - (void)testBackToBackUITextInputCallsUpdateEditingStateOnce { [inputView unmarkText]; XCTAssertEqual(updateCount, 6); } +*/ @end From ea19ac2a157d75a2ec7cb669a1d2ab601d51c72e Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 11 Sep 2020 12:51:03 -0700 Subject: [PATCH 08/16] fix testUITextInputCallsUpdateEditingStateOnce --- .../Source/FlutterTextInputPlugin.mm | 8 -- .../Source/FlutterTextInputPluginTest.m | 83 +++++-------------- 2 files changed, 23 insertions(+), 68 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index b7f32e3ec2bba..dcc82a423a95f 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -1003,26 +1003,19 @@ - (void)updateEditingState { // updates, thereby avoiding common race conditions. if (_latestState == nil) { _latestState = state; - NSLog(@"justin leading edge updateEditingClient for %@", _latestState[@"text"]); [self updateEditingClient]; - NSLog(@"justin 1"); dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 10000000), dispatch_get_main_queue(), ^(void){ - NSLog(@"justin 2"); if (_latestState == state) { - NSLog(@"justin updateEditingClient clear leading edge for %@", _latestState[@"text"]); _latestState = nil; } }); - NSLog(@"justin 3"); return; } _latestState = state; - NSLog(@"justin updateEditingClient start trailing edge timer for _latestState for %@", _latestState[@"text"]); dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 10000000), dispatch_get_main_queue(), ^(void){ if (state != _latestState) { return; } - NSLog(@"justin trailing edge updateEditingClient for %@", _latestState[@"text"]); [self updateEditingClient]; _latestState = nil; }); @@ -1367,7 +1360,6 @@ - (void)addToInputParentViewIfNeeded:(FlutterTextInputView*)inputView { - (void)setTextInputEditingState:(NSDictionary*)state { if ([_activeView setTextInputState:state]) { - NSLog(@"justin setTextInputEditingState for %@", state[@"text"]); [_activeView updateEditingState]; } } diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index 5a900f28fa6d3..44dd8039d1fd4 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -174,30 +174,49 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; + XCTestExpectation* expectation2 = [self expectationWithDescription:@"called updateEditingClient twice"]; + XCTestExpectation* expectation4 = [self expectationWithDescription:@"called updateEditingClient four times"]; + XCTestExpectation* expectation6 = [self expectationWithDescription:@"called updateEditingClient six times"]; __block int updateCount = 0; OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) .andDo(^(NSInvocation* invocation) { updateCount++; + if (updateCount == 2) { + [expectation2 fulfill]; + } else if (updateCount == 4) { + [expectation4 fulfill]; + } else if (updateCount == 6) { + [expectation6 fulfill]; + } }); [inputView insertText:@"text to insert"]; - // Update the framework exactly once. + // The framework has been updated exactly once. It happened immediately, + // because calls to updateEditingState are debounced on the leading edge. XCTAssertEqual(updateCount, 1); [inputView deleteBackward]; + // Due to the debouncing, this call will happen after a short delay. + [self waitForExpectations:@[expectation2] timeout:1]; XCTAssertEqual(updateCount, 2); + // Subsequent calls follow this pattern of leading edge debouncing. Now that + // enough time has passed to allow the debouncing to call through, the + // debouncing is reset. The next call will happen immediately on the leading + // edge, and subsequent calls are debounced. inputView.selectedTextRange = [FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)]; XCTAssertEqual(updateCount, 3); [inputView replaceRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)] withText:@"replace text"]; + [self waitForExpectations:@[expectation4] timeout:1]; XCTAssertEqual(updateCount, 4); [inputView setMarkedText:@"marked text" selectedRange:NSMakeRange(0, 1)]; XCTAssertEqual(updateCount, 5); [inputView unmarkText]; + [self waitForExpectations:@[expectation6] timeout:1]; XCTAssertEqual(updateCount, 6); } @@ -309,6 +328,7 @@ - (void)testUpdateEditingClientNegativeSelection { }]]); } +/* - (void)testUpdateEditingClientSelectionClamping { // Regression test for https://github.com/flutter/flutter/issues/62992. FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; @@ -335,6 +355,7 @@ - (void)testUpdateEditingClientSelectionClamping { }]; [inputView updateEditingState]; + // TODO(justinmc): I'll have to wait for this to be called. OCMVerify([engine updateEditingClient:0 withState:[OCMArg checkWithBlock:^BOOL(NSDictionary* state) { return ([state[@"selectionBase"] intValue]) == 0 && @@ -364,6 +385,7 @@ - (void)testUpdateEditingClientSelectionClamping { ([state[@"selectionExtent"] intValue] == 9); }]]); } +*/ #pragma mark - Autofill - Utilities @@ -680,65 +702,6 @@ - (void)testClearAutofillContextClearsSelection { XCTAssert(NSEqualRanges(selectionRange.range, NSMakeRange(0, 0))); } -/* - * TODO(justinmc): I was messing with this test, but it was moved in master to - * above. Maybe modify that one and get rid of all this? -- (void)testUITextInputCallsUpdateEditingStateOnce { - NSLog(@"justin start testUITextInputCallsUpdateEditingStateOnce"); - FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; - inputView.textInputDelegate = engine; - - XCTestExpectation* expectation = [self expectationWithDescription:@"called updateEditingClient"]; - XCTestExpectation* expectation3 = [self expectationWithDescription:@"called four times"]; - NSString* text; - __block bool calledSinceChange = false; - __block int updateCount = 0; - OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) - .andDo(^(NSInvocation* invocation) { - XCTAssertEqual(calledSinceChange, false); - calledSinceChange = true; - updateCount++; - if (updateCount == 1) { - [expectation fulfill]; - } else if (updateCount == 3) { - [expectation3 fulfill]; - } - }); - - text = @"text to insert"; - NSLog(@"justin testUITextInputCallsUpdateEditingStateOnce insertText 1"); - [inputView insertText:@"text to insert"]; - NSLog(@"justin testUITextInputCallsUpdateEditingStateOnce insertedText 1"); - // Update the framework exactly once. - XCTAssertEqual(updateCount, 1); - calledSinceChange = false; - - expectation = [self expectationWithDescription:@"called updateEditingClient"]; - text = @"text to inser"; - [inputView deleteBackward]; - XCTAssertEqual(updateCount, 1); - [self waitForExpectations:@[expectation] timeout:1]; - XCTAssertEqual(updateCount, 2); - - inputView.selectedTextRange = [FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)]; - XCTAssertEqual(updateCount, 3); - - [inputView replaceRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)] - withText:@"replace text"]; - XCTAssertEqual(updateCount, 3); - [self waitForExpectations:@[expectation3] timeout:1]; - XCTAssertEqual(updateCount, 4); - -// TODO(justinmc): The below was commented out. - [inputView setMarkedText:@"marked text" selectedRange:NSMakeRange(0, 1)]; - [self waitForExpectations:@[expectation4] timeout:1]; - XCTAssertEqual(updateCount, 5); - - [inputView unmarkText]; - XCTAssertEqual(updateCount, 6); -} -*/ - - (void)testGarbageInputViewsAreNotRemovedImmediately { // Add a password field that should autofill. [self setClientId:123 configuration:self.mutablePasswordTemplateCopy]; From da167879a95f9fbcd506f4e8a22ce153cf593850 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 11 Sep 2020 12:55:40 -0700 Subject: [PATCH 09/16] Existing tests pass by waiting for debounced calls. --- .../Source/FlutterTextInputPluginTest.m | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index 44dd8039d1fd4..60e5cae4f770b 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -170,6 +170,7 @@ - (void)testNoZombies { #pragma mark - EditingState tests +// TODO(justinmc): Write a similar test that shows batching happening. - (void)testUITextInputCallsUpdateEditingStateOnce { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; @@ -328,12 +329,25 @@ - (void)testUpdateEditingClientNegativeSelection { }]]); } -/* - (void)testUpdateEditingClientSelectionClamping { // Regression test for https://github.com/flutter/flutter/issues/62992. FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; + // Debounced calls need to be waited for using these expectations. + XCTestExpectation* expectation2 = [self expectationWithDescription:@"called updateEditingClient twice"]; + XCTestExpectation* expectation4 = [self expectationWithDescription:@"called updateEditingClient four times"]; + __block int updateCount = 0; + OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) + .andDo(^(NSInvocation* invocation) { + updateCount++; + if (updateCount == 2) { + [expectation2 fulfill]; + } else if (updateCount == 4) { + [expectation4 fulfill]; + } + }); + [inputView.text setString:@"SELECTION"]; inputView.markedTextRange = nil; inputView.selectedTextRange = nil; @@ -355,7 +369,7 @@ - (void)testUpdateEditingClientSelectionClamping { }]; [inputView updateEditingState]; - // TODO(justinmc): I'll have to wait for this to be called. + [self waitForExpectations:@[expectation2] timeout:1]; OCMVerify([engine updateEditingClient:0 withState:[OCMArg checkWithBlock:^BOOL(NSDictionary* state) { return ([state[@"selectionBase"] intValue]) == 0 && @@ -379,13 +393,13 @@ - (void)testUpdateEditingClientSelectionClamping { @"selectionExtent" : @9999 }]; [inputView updateEditingState]; + [self waitForExpectations:@[expectation4] timeout:1]; OCMVerify([engine updateEditingClient:0 withState:[OCMArg checkWithBlock:^BOOL(NSDictionary* state) { return ([state[@"selectionBase"] intValue]) == 9 && ([state[@"selectionExtent"] intValue] == 9); }]]); } -*/ #pragma mark - Autofill - Utilities From 39288609355171a2dc27dd3c3c2e97b5dce81987 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Mon, 14 Sep 2020 11:48:46 -0700 Subject: [PATCH 10/16] Test batching --- .../Source/FlutterTextInputPluginTest.m | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index 60e5cae4f770b..e4d3f8a499b4d 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -170,7 +170,6 @@ - (void)testNoZombies { #pragma mark - EditingState tests -// TODO(justinmc): Write a similar test that shows batching happening. - (void)testUITextInputCallsUpdateEditingStateOnce { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; @@ -198,6 +197,7 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { [inputView deleteBackward]; // Due to the debouncing, this call will happen after a short delay. + XCTAssertEqual(updateCount, 1); [self waitForExpectations:@[expectation2] timeout:1]; XCTAssertEqual(updateCount, 2); @@ -210,6 +210,7 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { [inputView replaceRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)] withText:@"replace text"]; + XCTAssertEqual(updateCount, 3); [self waitForExpectations:@[expectation4] timeout:1]; XCTAssertEqual(updateCount, 4); @@ -217,10 +218,40 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { XCTAssertEqual(updateCount, 5); [inputView unmarkText]; + XCTAssertEqual(updateCount, 5); [self waitForExpectations:@[expectation6] timeout:1]; XCTAssertEqual(updateCount, 6); } +- (void)testUITextInputCallsToUpdateEditingStateAreDebounced { + FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; + inputView.textInputDelegate = engine; + + XCTestExpectation* expectation2 = [self expectationWithDescription:@"called updateEditingClient twice"]; + __block int updateCount = 0; + OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) + .andDo(^(NSInvocation* invocation) { + updateCount++; + if (updateCount == 2) { + [expectation2 fulfill]; + } + }); + + [inputView insertText:@"text to insert"]; + // The framework has been updated exactly once. It happened immediately, + // because calls to updateEditingState are debounced on the leading edge. + XCTAssertEqual(updateCount, 1); + + // These calls will be batched and come through after a short delay. + [inputView deleteBackward]; + [inputView deleteBackward]; + [inputView deleteBackward]; + XCTAssertEqual(updateCount, 1); + [self waitForExpectations:@[expectation2] timeout:1]; + XCTAssertEqual(updateCount, 2); + XCTAssertTrue([inputView.text isEqualToString:@"text to ins"]); +} + - (void)testTextChangesTriggerUpdateEditingClient { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; From 44c766a5b8c2d1bc9411eba196b008d395ff8907 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Mon, 14 Sep 2020 12:05:20 -0700 Subject: [PATCH 11/16] Clean up --- .../Source/FlutterTextInputPlugin.mm | 10 ++-- .../Source/FlutterTextInputPluginTest.m | 52 ++++++++++--------- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index dcc82a423a95f..1aa0a094c355e 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -996,11 +996,11 @@ - (void)updateEditingState { @"text" : [NSString stringWithString:self.text], }; - // Debounce calls to updateEditingClient. This makes iOS text editing behave - // more similarly to Android's, which has built-in event batching, and avoids - // race conditions. The delay value was chosen to be imperceptible by the user - // but still long enough to allow the framework to respond with formatting - // updates, thereby avoiding common race conditions. + // Debounce calls to updateEditingClient on the leading edge. This makes iOS + // text editing behave more similarly to Android's, which has built-in event + // batching, and avoids race conditions. The delay value was chosen to be + // imperceptible by the user but still long enough to allow the framework to + // respond with formatting updates, thereby avoiding common race conditions. if (_latestState == nil) { _latestState = state; [self updateEditingClient]; diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index e4d3f8a499b4d..ca69aa641e663 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -174,19 +174,13 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; - XCTestExpectation* expectation2 = [self expectationWithDescription:@"called updateEditingClient twice"]; - XCTestExpectation* expectation4 = [self expectationWithDescription:@"called updateEditingClient four times"]; - XCTestExpectation* expectation6 = [self expectationWithDescription:@"called updateEditingClient six times"]; + __block XCTestExpectation* expectation; __block int updateCount = 0; OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) .andDo(^(NSInvocation* invocation) { updateCount++; - if (updateCount == 2) { - [expectation2 fulfill]; - } else if (updateCount == 4) { - [expectation4 fulfill]; - } else if (updateCount == 6) { - [expectation6 fulfill]; + if (expectation != nil) { + [expectation fulfill]; } }); @@ -195,10 +189,12 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { // because calls to updateEditingState are debounced on the leading edge. XCTAssertEqual(updateCount, 1); + expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView deleteBackward]; // Due to the debouncing, this call will happen after a short delay. XCTAssertEqual(updateCount, 1); - [self waitForExpectations:@[expectation2] timeout:1]; + [self waitForExpectations:@[expectation] timeout:1]; + expectation = nil; XCTAssertEqual(updateCount, 2); // Subsequent calls follow this pattern of leading edge debouncing. Now that @@ -208,18 +204,22 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { inputView.selectedTextRange = [FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)]; XCTAssertEqual(updateCount, 3); + expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView replaceRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)] withText:@"replace text"]; XCTAssertEqual(updateCount, 3); - [self waitForExpectations:@[expectation4] timeout:1]; + [self waitForExpectations:@[expectation] timeout:1]; + expectation = nil; XCTAssertEqual(updateCount, 4); [inputView setMarkedText:@"marked text" selectedRange:NSMakeRange(0, 1)]; XCTAssertEqual(updateCount, 5); + expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView unmarkText]; XCTAssertEqual(updateCount, 5); - [self waitForExpectations:@[expectation6] timeout:1]; + [self waitForExpectations:@[expectation] timeout:1]; + expectation = nil; XCTAssertEqual(updateCount, 6); } @@ -227,13 +227,13 @@ - (void)testUITextInputCallsToUpdateEditingStateAreDebounced { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; - XCTestExpectation* expectation2 = [self expectationWithDescription:@"called updateEditingClient twice"]; + __block XCTestExpectation* expectation; __block int updateCount = 0; OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) .andDo(^(NSInvocation* invocation) { updateCount++; - if (updateCount == 2) { - [expectation2 fulfill]; + if (expectation != nil) { + [expectation fulfill]; } }); @@ -243,11 +243,12 @@ - (void)testUITextInputCallsToUpdateEditingStateAreDebounced { XCTAssertEqual(updateCount, 1); // These calls will be batched and come through after a short delay. + expectation = [self expectationWithDescription:@"called updateEditingClient twice"]; [inputView deleteBackward]; [inputView deleteBackward]; [inputView deleteBackward]; XCTAssertEqual(updateCount, 1); - [self waitForExpectations:@[expectation2] timeout:1]; + [self waitForExpectations:@[expectation] timeout:1]; XCTAssertEqual(updateCount, 2); XCTAssertTrue([inputView.text isEqualToString:@"text to ins"]); } @@ -365,17 +366,14 @@ - (void)testUpdateEditingClientSelectionClamping { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; - // Debounced calls need to be waited for using these expectations. - XCTestExpectation* expectation2 = [self expectationWithDescription:@"called updateEditingClient twice"]; - XCTestExpectation* expectation4 = [self expectationWithDescription:@"called updateEditingClient four times"]; + // Debounced calls need to be waited for using this expectation. + __block XCTestExpectation* expectation; __block int updateCount = 0; OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) .andDo(^(NSInvocation* invocation) { updateCount++; - if (updateCount == 2) { - [expectation2 fulfill]; - } else if (updateCount == 4) { - [expectation4 fulfill]; + if (expectation != nil) { + [expectation fulfill]; } }); @@ -393,6 +391,7 @@ - (void)testUpdateEditingClientSelectionClamping { }]]); // Needs clamping. + expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView setTextInputState:@{ @"text" : @"SELECTION", @"selectionBase" : @0, @@ -400,7 +399,8 @@ - (void)testUpdateEditingClientSelectionClamping { }]; [inputView updateEditingState]; - [self waitForExpectations:@[expectation2] timeout:1]; + [self waitForExpectations:@[expectation] timeout:1]; + expectation = nil; OCMVerify([engine updateEditingClient:0 withState:[OCMArg checkWithBlock:^BOOL(NSDictionary* state) { return ([state[@"selectionBase"] intValue]) == 0 && @@ -418,13 +418,15 @@ - (void)testUpdateEditingClientSelectionClamping { }]]); // Both ends need clamping. + expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView setTextInputState:@{ @"text" : @"SELECTION", @"selectionBase" : @9999, @"selectionExtent" : @9999 }]; [inputView updateEditingState]; - [self waitForExpectations:@[expectation4] timeout:1]; + [self waitForExpectations:@[expectation] timeout:1]; + expectation = nil; OCMVerify([engine updateEditingClient:0 withState:[OCMArg checkWithBlock:^BOOL(NSDictionary* state) { return ([state[@"selectionBase"] intValue]) == 9 && From 994962338d569fb2e65a39b53abd1490c4574fed Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Thu, 17 Sep 2020 18:22:20 -0700 Subject: [PATCH 12/16] release _latestState --- .../darwin/ios/framework/Source/FlutterTextInputPlugin.mm | 1 + 1 file changed, 1 insertion(+) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index 1aa0a094c355e..c6cfcdb058952 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -514,6 +514,7 @@ - (void)dealloc { [_selectedTextRange release]; [_tokenizer release]; [_autofillId release]; + [_latestState release]; [super dealloc]; } From a87230bac5bf61dce28e2b93975221cb3123c87f Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Wed, 23 Sep 2020 12:53:45 -0700 Subject: [PATCH 13/16] Dont send an ack back to the framework for text changes. The reason we did that (onchanged) is no longer necessary. --- .../Source/FlutterTextInputPlugin.mm | 44 ++----------------- 1 file changed, 4 insertions(+), 40 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm index 826fb07e0d305..507b9cae8434a 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm @@ -432,7 +432,6 @@ @implementation FlutterTextInputView { int _textInputClient; const char* _selectionAffinity; FlutterTextRange* _selectedTextRange; - NSDictionary* _latestState; CGRect _cachedFirstRect; } @@ -531,7 +530,6 @@ - (void)dealloc { [_selectedTextRange release]; [_tokenizer release]; [_autofillId release]; - [_latestState release]; [super dealloc]; } @@ -539,10 +537,7 @@ - (void)setTextInputClient:(int)client { _textInputClient = client; } -// Return true if the new input state needs to be synced back to the framework. -// TODO(LongCatIsLooong): setTextInputState should never call updateEditingState. Sending the -// editing value back may overwrite the framework's updated editing value. -- (BOOL)setTextInputState:(NSDictionary*)state { +- (void)setTextInputState:(NSDictionary*)state { NSString* newText = state[@"text"]; BOOL textChanged = ![self.text isEqualToString:newText]; if (textChanged) { @@ -577,9 +572,6 @@ - (BOOL)setTextInputState:(NSDictionary*)state { if (textChanged) { [self.inputDelegate textDidChange:self]; } - - // For consistency with Android behavior, send an update to the framework if the text changed. - return textChanged; } // Extracts the selection information from the editing state dictionary. @@ -1069,36 +1061,10 @@ - (void)updateEditingState { @"text" : [NSString stringWithString:self.text], }; - // Debounce calls to updateEditingClient on the leading edge. This makes iOS - // text editing behave more similarly to Android's, which has built-in event - // batching, and avoids race conditions. The delay value was chosen to be - // imperceptible by the user but still long enough to allow the framework to - // respond with formatting updates, thereby avoiding common race conditions. - if (_latestState == nil) { - _latestState = state; - [self updateEditingClient]; - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 10000000), dispatch_get_main_queue(), ^(void){ - if (_latestState == state) { - _latestState = nil; - } - }); - return; - } - _latestState = state; - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 10000000), dispatch_get_main_queue(), ^(void){ - if (state != _latestState) { - return; - } - [self updateEditingClient]; - _latestState = nil; - }); -} - -- (void)updateEditingClient { if (_textInputClient == 0 && _autofillId != nil) { - [_textInputDelegate updateEditingClient:_textInputClient withState:_latestState withTag:_autofillId]; + [_textInputDelegate updateEditingClient:_textInputClient withState:state withTag:_autofillId]; } else { - [_textInputDelegate updateEditingClient:_textInputClient withState:_latestState]; + [_textInputDelegate updateEditingClient:_textInputClient withState:state]; } } @@ -1451,9 +1417,7 @@ - (void)addToInputParentViewIfNeeded:(FlutterTextInputView*)inputView { } - (void)setTextInputEditingState:(NSDictionary*)state { - if ([_activeView setTextInputState:state]) { - [_activeView updateEditingState]; - } + [_activeView setTextInputState:state]; } - (void)clearTextInputClient { From 255efbbf18d844ee953d6c66e82127901096908a Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Wed, 23 Sep 2020 12:59:53 -0700 Subject: [PATCH 14/16] Test that the client isn't updated with its own text changes --- .../Source/FlutterTextInputPluginTest.m | 131 ++---------------- 1 file changed, 10 insertions(+), 121 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index 0f00968d0c006..251d51aca0088 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -184,150 +184,56 @@ - (void)testUITextInputCallsUpdateEditingStateOnce { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; - __block XCTestExpectation* expectation; __block int updateCount = 0; OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) .andDo(^(NSInvocation* invocation) { updateCount++; - if (expectation != nil) { - [expectation fulfill]; - } }); [inputView insertText:@"text to insert"]; - // The framework has been updated exactly once. It happened immediately, - // because calls to updateEditingState are debounced on the leading edge. + // Update the framework exactly once. XCTAssertEqual(updateCount, 1); - expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView deleteBackward]; - // Due to the debouncing, this call will happen after a short delay. - XCTAssertEqual(updateCount, 1); - [self waitForExpectations:@[expectation] timeout:1]; - expectation = nil; XCTAssertEqual(updateCount, 2); - // Subsequent calls follow this pattern of leading edge debouncing. Now that - // enough time has passed to allow the debouncing to call through, the - // debouncing is reset. The next call will happen immediately on the leading - // edge, and subsequent calls are debounced. inputView.selectedTextRange = [FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)]; XCTAssertEqual(updateCount, 3); - expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView replaceRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)] withText:@"replace text"]; - XCTAssertEqual(updateCount, 3); - [self waitForExpectations:@[expectation] timeout:1]; - expectation = nil; XCTAssertEqual(updateCount, 4); [inputView setMarkedText:@"marked text" selectedRange:NSMakeRange(0, 1)]; XCTAssertEqual(updateCount, 5); - expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView unmarkText]; - XCTAssertEqual(updateCount, 5); - [self waitForExpectations:@[expectation] timeout:1]; - expectation = nil; XCTAssertEqual(updateCount, 6); } -- (void)testUITextInputCallsToUpdateEditingStateAreDebounced { +- (void)testTextChangesDoNotTriggerUpdateEditingClient { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; - __block XCTestExpectation* expectation; __block int updateCount = 0; OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) .andDo(^(NSInvocation* invocation) { updateCount++; - if (expectation != nil) { - [expectation fulfill]; - } }); - [inputView insertText:@"text to insert"]; - // The framework has been updated exactly once. It happened immediately, - // because calls to updateEditingState are debounced on the leading edge. - XCTAssertEqual(updateCount, 1); - - // These calls will be batched and come through after a short delay. - expectation = [self expectationWithDescription:@"called updateEditingClient twice"]; - [inputView deleteBackward]; - [inputView deleteBackward]; - [inputView deleteBackward]; - XCTAssertEqual(updateCount, 1); - [self waitForExpectations:@[expectation] timeout:1]; - XCTAssertEqual(updateCount, 2); - XCTAssertTrue([inputView.text isEqualToString:@"text to ins"]); -} - -- (void)testTextChangesTriggerUpdateEditingClient { - FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; - inputView.textInputDelegate = engine; - [inputView.text setString:@"BEFORE"]; - inputView.markedTextRange = nil; - inputView.selectedTextRange = nil; - - // Text changes trigger update. - XCTAssertTrue([inputView setTextInputState:@{@"text" : @"AFTER"}]); - - // Don't send anything if there's nothing new. - XCTAssertFalse([inputView setTextInputState:@{@"text" : @"AFTER"}]); -} - -- (void)testSelectionChangeDoesNotTriggerUpdateEditingClient { - FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; - inputView.textInputDelegate = engine; - - [inputView.text setString:@"SELECTION"]; - inputView.markedTextRange = nil; - inputView.selectedTextRange = nil; - - BOOL shouldUpdate = [inputView - setTextInputState:@{@"text" : @"SELECTION", @"selectionBase" : @0, @"selectionExtent" : @3}]; - XCTAssertFalse(shouldUpdate); - - shouldUpdate = [inputView - setTextInputState:@{@"text" : @"SELECTION", @"selectionBase" : @1, @"selectionExtent" : @3}]; - XCTAssertFalse(shouldUpdate); - - shouldUpdate = [inputView - setTextInputState:@{@"text" : @"SELECTION", @"selectionBase" : @1, @"selectionExtent" : @2}]; - XCTAssertFalse(shouldUpdate); - - // Don't send anything if there's nothing new. - shouldUpdate = [inputView - setTextInputState:@{@"text" : @"SELECTION", @"selectionBase" : @1, @"selectionExtent" : @2}]; - XCTAssertFalse(shouldUpdate); -} - -- (void)testComposingChangeDoesNotTriggerUpdateEditingClient { - FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; - inputView.textInputDelegate = engine; + XCTAssertEqual(updateCount, 0); - // Reset to test marked text. - [inputView.text setString:@"COMPOSING"]; inputView.markedTextRange = nil; inputView.selectedTextRange = nil; + XCTAssertEqual(updateCount, 1); - BOOL shouldUpdate = [inputView - setTextInputState:@{@"text" : @"COMPOSING", @"composingBase" : @0, @"composingExtent" : @3}]; - XCTAssertFalse(shouldUpdate); - - shouldUpdate = [inputView - setTextInputState:@{@"text" : @"COMPOSING", @"composingBase" : @1, @"composingExtent" : @3}]; - XCTAssertFalse(shouldUpdate); - - shouldUpdate = [inputView - setTextInputState:@{@"text" : @"COMPOSING", @"composingBase" : @1, @"composingExtent" : @2}]; - XCTAssertFalse(shouldUpdate); - - shouldUpdate = [inputView - setTextInputState:@{@"text" : @"COMPOSING", @"composingBase" : @1, @"composingExtent" : @2}]; - XCTAssertFalse(shouldUpdate); + // Text changes don't trigger an update. + XCTAssertEqual(updateCount, 1); + [inputView setTextInputState:@{@"text" : @"AFTER"}]; + XCTAssertEqual(updateCount, 1); + [inputView setTextInputState:@{@"text" : @"AFTER"}]; + XCTAssertEqual(updateCount, 1); } - (void)testUITextInputAvoidUnnecessaryUndateEditingClientCalls { @@ -398,17 +304,6 @@ - (void)testUpdateEditingClientSelectionClamping { FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; inputView.textInputDelegate = engine; - // Debounced calls need to be waited for using this expectation. - __block XCTestExpectation* expectation; - __block int updateCount = 0; - OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) - .andDo(^(NSInvocation* invocation) { - updateCount++; - if (expectation != nil) { - [expectation fulfill]; - } - }); - [inputView.text setString:@"SELECTION"]; inputView.markedTextRange = nil; inputView.selectedTextRange = nil; @@ -423,7 +318,6 @@ - (void)testUpdateEditingClientSelectionClamping { }]]); // Needs clamping. - expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView setTextInputState:@{ @"text" : @"SELECTION", @"selectionBase" : @0, @@ -431,8 +325,6 @@ - (void)testUpdateEditingClientSelectionClamping { }]; [inputView updateEditingState]; - [self waitForExpectations:@[expectation] timeout:1]; - expectation = nil; OCMVerify([engine updateEditingClient:0 withState:[OCMArg checkWithBlock:^BOOL(NSDictionary* state) { return ([state[@"selectionBase"] intValue]) == 0 && @@ -450,15 +342,12 @@ - (void)testUpdateEditingClientSelectionClamping { }]]); // Both ends need clamping. - expectation = [self expectationWithDescription:@"called updateEditingClient"]; [inputView setTextInputState:@{ @"text" : @"SELECTION", @"selectionBase" : @9999, @"selectionExtent" : @9999 }]; [inputView updateEditingState]; - [self waitForExpectations:@[expectation] timeout:1]; - expectation = nil; OCMVerify([engine updateEditingClient:0 withState:[OCMArg checkWithBlock:^BOOL(NSDictionary* state) { return ([state[@"selectionBase"] intValue]) == 9 && From bca94b6057a83c0481b474266a3a056f42c7cf0d Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Thu, 1 Oct 2020 09:27:43 -0700 Subject: [PATCH 15/16] Update return type in test --- .../darwin/ios/framework/Source/FlutterTextInputPluginTest.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index 251d51aca0088..61bcc159d1826 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -16,7 +16,7 @@ @interface FlutterTextInputView () @property(nonatomic, copy) NSString* autofillId; - (void)setEditableTransform:(NSArray*)matrix; -- (BOOL)setTextInputState:(NSDictionary*)state; +- (void)setTextInputState:(NSDictionary*)state; - (void)setMarkedRect:(CGRect)markedRect; - (void)updateEditingState; - (BOOL)isVisibleToAutofill; From c36382cdef2062801f40910ccbd8b1cdeeecd20a Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Thu, 1 Oct 2020 09:30:52 -0700 Subject: [PATCH 16/16] Also test selection and composing changes --- .../Source/FlutterTextInputPluginTest.m | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m index 61bcc159d1826..88683abb087f6 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m @@ -234,6 +234,22 @@ - (void)testTextChangesDoNotTriggerUpdateEditingClient { XCTAssertEqual(updateCount, 1); [inputView setTextInputState:@{@"text" : @"AFTER"}]; XCTAssertEqual(updateCount, 1); + + // Selection changes don't trigger an update. + [inputView + setTextInputState:@{@"text" : @"SELECTION", @"selectionBase" : @0, @"selectionExtent" : @3}]; + XCTAssertEqual(updateCount, 1); + [inputView + setTextInputState:@{@"text" : @"SELECTION", @"selectionBase" : @1, @"selectionExtent" : @3}]; + XCTAssertEqual(updateCount, 1); + + // Composing region changes don't trigger an update. + [inputView + setTextInputState:@{@"text" : @"COMPOSING", @"composingBase" : @1, @"composingExtent" : @2}]; + XCTAssertEqual(updateCount, 1); + [inputView + setTextInputState:@{@"text" : @"COMPOSING", @"composingBase" : @1, @"composingExtent" : @3}]; + XCTAssertEqual(updateCount, 1); } - (void)testUITextInputAvoidUnnecessaryUndateEditingClientCalls {