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

* [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 <dkwingsmt@users.noreply.github.com> Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
2021-10-19 17:57:11 -04:00
parent 42d7e2b450
commit cd18e95d73
5 changed files with 113 additions and 15 deletions
--- a/engine/src/flutter/fml/platform/darwin/message_loop_darwin.h
+++ b/engine/src/flutter/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_;
--- a/engine/src/flutter/fml/platform/darwin/message_loop_darwin.mm
+++ b/engine/src/flutter/fml/platform/darwin/message_loop_darwin.mm
@@ -13,6 +13,8 @@ namespace fml {

 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 @@ MessageLoopDarwin::MessageLoopDarwin()
                           &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() {
--- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.mm
+++ b/engine/src/flutter/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 @@
      // 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:
--- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm
+++ b/engine/src/flutter/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 @@ extern NSNotificationName const FlutterViewControllerWillDealloc;

 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 @@ typedef BOOL (^BoolGetter)();
      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 @@ typedef BOOL (^BoolGetter)();
  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 @@ typedef BOOL (^BoolGetter)();

  __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 @@ typedef BOOL (^BoolGetter)();

  __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 @@ typedef BOOL (^BoolGetter)();
  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
--- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
+++ b/engine/src/flutter/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 @@ class PointerDataPacket {};
  // 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 @@ typedef enum UIAccessibilityContrast : NSInteger {
  // 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)) {