[iOS] Fix duplicated keys when typing quickly on HW keyboard (flutter#29113)

moffatman · dkwingsmt · web-flow · commit b83eabca5a95 · 2021-10-19T14:57:11.000-07:00
* [iOS] Fix duplicated keys when typing quickly on HW keyboard

* Address review (refactor constant)

* Change message loop constant name

* Apply suggestions from code review

Co-authored-by: Tong Mu &lt;dkwingsmt@users.noreply.github.com&gt;

Co-authored-by: Tong Mu &lt;dkwingsmt@users.noreply.github.com&gt;
diff --git a/fml/platform/darwin/message_loop_darwin.h b/fml/platform/darwin/message_loop_darwin.h
@@ -16,6 +16,12 @@
 namespace fml {
 
 class MessageLoopDarwin : public MessageLoopImpl {
+ public:
+  // A custom CFRunLoop mode used when processing flutter messages,
+  // so that the CFRunLoop can be run without being interrupted by UIKit,
+  // while still being able to receive and be interrupted by framework messages.
+  static CFStringRef kMessageLoopCFRunLoopMode;
+
  private:
   std::atomic_bool running_;
   CFRef<CFRunLoopTimerRef> delayed_wake_timer_;
diff --git a/fml/platform/darwin/message_loop_darwin.mm b/fml/platform/darwin/message_loop_darwin.mm
@@ -13,6 +13,8 @@
 
 static constexpr CFTimeInterval kDistantFuture = 1.0e10;
 
+CFStringRef MessageLoopDarwin::kMessageLoopCFRunLoopMode = CFSTR("fmlMessageLoop");
+
 MessageLoopDarwin::MessageLoopDarwin()
     : running_(false), loop_((CFRunLoopRef)CFRetain(CFRunLoopGetCurrent())) {
   FML_DCHECK(loop_ != nullptr);
@@ -29,11 +31,14 @@
                            &timer_context /* context */));
   FML_DCHECK(delayed_wake_timer_ != nullptr);
   CFRunLoopAddTimer(loop_, delayed_wake_timer_, kCFRunLoopCommonModes);
+  // This mode will be used by FlutterKeyboardManager.
+  CFRunLoopAddTimer(loop_, delayed_wake_timer_, kMessageLoopCFRunLoopMode);
 }
 
 MessageLoopDarwin::~MessageLoopDarwin() {
   CFRunLoopTimerInvalidate(delayed_wake_timer_);
   CFRunLoopRemoveTimer(loop_, delayed_wake_timer_, kCFRunLoopCommonModes);
+  CFRunLoopRemoveTimer(loop_, delayed_wake_timer_, kMessageLoopCFRunLoopMode);
 }
 
 void MessageLoopDarwin::Run() {
diff --git a/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.mm b/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.mm
@@ -4,6 +4,9 @@
 
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.h"
 #include "flutter/fml/memory/weak_ptr.h"
+#include "flutter/fml/platform/darwin/message_loop_darwin.h"
+
+static constexpr CFTimeInterval kDistantFuture = 1.0e10;
 
 @interface FlutterKeyboardManager ()
 
@@ -102,7 +105,10 @@ - (void)handlePress:(nonnull FlutterUIPressProxy*)press
       // framework. Once the completeCallback is called, this run loop will exit
       // and the main one will resume. The completeCallback MUST be called, or
       // the app will get stuck in this run loop indefinitely.
-      CFRunLoopRun();
+      //
+      // We need to run in this mode so that UIKit doesn't give us new
+      // events until we are done processing this one.
+      CFRunLoopRunInMode(fml::MessageLoopDarwin::kMessageLoopCFRunLoopMode, kDistantFuture, NO);
       break;
     }
     case UIPressPhaseChanged:
diff --git a/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm b/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm
@@ -8,6 +8,7 @@
 #import <XCTest/XCTest.h>
 #include <_types/_uint32_t.h>
 
+#include "flutter/fml/platform/darwin/message_loop_darwin.h"
 #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterFakeKeyEvents.h"
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.h"
@@ -59,7 +60,8 @@ - (bool)emptyNextResponder;
 
 namespace {
 
-typedef void (^KeyCallbackSetter)(FlutterAsyncKeyCallback callback);
+typedef void (^KeyCallbackSetter)(FlutterUIPressProxy* press, FlutterAsyncKeyCallback callback)
+    API_AVAILABLE(ios(13.4));
 typedef BOOL (^BoolGetter)();
 
 }  // namespace
@@ -100,11 +102,14 @@ - (id)checkKeyDownEvent:(UIKeyboardHIDUsage)keyCode API_AVAILABLE(ios(13.4)) {
       OCMStrictProtocolMock(@protocol(FlutterKeyPrimaryResponder));
   OCMStub([mock handlePress:[OCMArg any] callback:[OCMArg any]])
       .andDo((^(NSInvocation* invocation) {
+        FlutterUIPressProxy* press;
         FlutterAsyncKeyCallback callback;
+        [invocation getArgument:&press atIndex:2];
         [invocation getArgument:&callback atIndex:3];
-        CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopDefaultMode, ^() {
-          callbackSetter(callback);
-        });
+        CFRunLoopPerformBlock(CFRunLoopGetCurrent(),
+                              fml::MessageLoopDarwin::kMessageLoopCFRunLoopMode, ^() {
+                                callbackSetter(press, callback);
+                              });
       }));
   return mock;
 }
@@ -149,7 +154,8 @@ - (void)testSinglePrimaryResponder API_AVAILABLE(ios(13.4)) {
   FlutterKeyboardManager* manager = [[FlutterKeyboardManager alloc] init];
   __block BOOL primaryResponse = FALSE;
   __block int callbackCount = 0;
-  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterAsyncKeyCallback callback) {
+  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterUIPressProxy* press,
+                                                            FlutterAsyncKeyCallback callback) {
              callbackCount++;
              callback(primaryResponse);
            }]];
@@ -181,14 +187,16 @@ - (void)testDoublePrimaryResponder API_AVAILABLE(ios(13.4)) {
 
   __block BOOL callback1Response = FALSE;
   __block int callback1Count = 0;
-  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterAsyncKeyCallback callback) {
+  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterUIPressProxy* press,
+                                                            FlutterAsyncKeyCallback callback) {
              callback1Count++;
              callback(callback1Response);
            }]];
 
   __block BOOL callback2Response = FALSE;
   __block int callback2Count = 0;
-  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterAsyncKeyCallback callback) {
+  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterUIPressProxy* press,
+                                                            FlutterAsyncKeyCallback callback) {
              callback2Count++;
              callback(callback2Response);
            }]];
@@ -242,7 +250,8 @@ - (void)testSingleSecondaryResponder API_AVAILABLE(ios(13.4)) {
 
   __block BOOL primaryResponse = FALSE;
   __block int callbackCount = 0;
-  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterAsyncKeyCallback callback) {
+  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterUIPressProxy* press,
+                                                            FlutterAsyncKeyCallback callback) {
              callbackCount++;
              callback(primaryResponse);
            }]];
@@ -291,4 +300,73 @@ - (void)testSingleSecondaryResponder API_AVAILABLE(ios(13.4)) {
   XCTAssertFalse(completeHandled);
 }
 
+- (void)testEventsProcessedSequentially API_AVAILABLE(ios(13.4)) {
+  constexpr UIKeyboardHIDUsage keyId1 = (UIKeyboardHIDUsage)0x50;
+  constexpr UIKeyboardHIDUsage keyId2 = (UIKeyboardHIDUsage)0x51;
+  FlutterUIPressProxy* event1 = keyDownEvent(keyId1);
+  FlutterUIPressProxy* event2 = keyDownEvent(keyId2);
+  __block FlutterAsyncKeyCallback key1Callback;
+  __block FlutterAsyncKeyCallback key2Callback;
+  __block bool key1Handled = true;
+  __block bool key2Handled = true;
+
+  FlutterKeyboardManager* manager = [[FlutterKeyboardManager alloc] init];
+  [manager addPrimaryResponder:[self mockPrimaryResponder:^(FlutterUIPressProxy* press,
+                                                            FlutterAsyncKeyCallback callback) {
+             if (press == event1) {
+               key1Callback = callback;
+             } else if (press == event2) {
+               key2Callback = callback;
+             }
+           }]];
+
+  // Add both presses into the main CFRunLoop queue
+  CFRunLoopTimerRef timer0 = CFRunLoopTimerCreateWithHandler(
+      kCFAllocatorDefault, CFAbsoluteTimeGetCurrent(), 0, 0, 0, ^(CFRunLoopTimerRef timerRef) {
+        [manager handlePress:event1
+                  nextAction:^() {
+                    key1Handled = false;
+                  }];
+      });
+  CFRunLoopAddTimer(CFRunLoopGetCurrent(), timer0, kCFRunLoopCommonModes);
+  CFRunLoopTimerRef timer1 = CFRunLoopTimerCreateWithHandler(
+      kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + 1, 0, 0, 0, ^(CFRunLoopTimerRef timerRef) {
+        // key1 should be completely finished by now
+        XCTAssertFalse(key1Handled);
+        [manager handlePress:event2
+                  nextAction:^() {
+                    key2Handled = false;
+                  }];
+        // End the nested CFRunLoop
+        CFRunLoopStop(CFRunLoopGetCurrent());
+      });
+  CFRunLoopAddTimer(CFRunLoopGetCurrent(), timer1, kCFRunLoopCommonModes);
+
+  // Add the callbacks to the CFRunLoop with mode kMessageLoopCFRunLoopMode
+  // This allows them to interrupt the loop started within handlePress
+  CFRunLoopTimerRef timer2 = CFRunLoopTimerCreateWithHandler(
+      kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + 2, 0, 0, 0, ^(CFRunLoopTimerRef timerRef) {
+        // No processing should be done on key2 yet
+        XCTAssertTrue(key1Callback != nil);
+        XCTAssertTrue(key2Callback == nil);
+        key1Callback(false);
+      });
+  CFRunLoopAddTimer(CFRunLoopGetCurrent(), timer2,
+                    fml::MessageLoopDarwin::kMessageLoopCFRunLoopMode);
+  CFRunLoopTimerRef timer3 = CFRunLoopTimerCreateWithHandler(
+      kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + 3, 0, 0, 0, ^(CFRunLoopTimerRef timerRef) {
+        // Both keys should be processed by now
+        XCTAssertTrue(key1Callback != nil);
+        XCTAssertTrue(key2Callback != nil);
+        key2Callback(false);
+      });
+  CFRunLoopAddTimer(CFRunLoopGetCurrent(), timer3,
+                    fml::MessageLoopDarwin::kMessageLoopCFRunLoopMode);
+
+  // Start a nested CFRunLoop so we can wait for both presses to complete before exiting the test
+  CFRunLoopRun();
+  XCTAssertFalse(key2Handled);
+  XCTAssertFalse(key1Handled);
+}
+
 @end
diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
@@ -5,6 +5,7 @@
 #import <OCMock/OCMock.h>
 #import <XCTest/XCTest.h>
 
+#include "flutter/fml/platform/darwin/message_loop_darwin.h"
 #import "flutter/lib/ui/window/viewport_metrics.h"
 #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h"
 #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"
@@ -59,9 +60,10 @@ - (void)sendKeyEvent:(const FlutterKeyEvent&)event
   // NSAssert(callback != nullptr, @"Invalid callback");
   // Response is async, so we have to post it to the run loop instead of calling
   // it directly.
-  CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopDefaultMode, ^() {
-    callback(true, userData);
-  });
+  CFRunLoopPerformBlock(CFRunLoopGetCurrent(), fml::MessageLoopDarwin::kMessageLoopCFRunLoopMode,
+                        ^() {
+                          callback(true, userData);
+                        });
 }
 @end
 
@@ -763,9 +765,10 @@ - (void)sendMessage:(id _Nullable)message reply:(FlutterReply _Nullable)callback
   // Response is async, so we have to post it to the run loop instead of calling
   // it directly.
   self.messageSent = message;
-  CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopDefaultMode, ^() {
-    callback(replyMessage);
-  });
+  CFRunLoopPerformBlock(CFRunLoopGetCurrent(), fml::MessageLoopDarwin::kMessageLoopCFRunLoopMode,
+                        ^() {
+                          callback(replyMessage);
+                        });
 }
 
 - (void)testValidKeyUpEvent API_AVAILABLE(ios(13.4)) {
