[macOS] Ensure first responder is consistent during and after text input (flutter/engine#46032)

Fixes https://github.com/flutter/flutter/issues/134906 Fixes https://github.com/flutter/flutter/issues/133832 This ensures that there are only two first responder widgets - `FlutterView` when text input is not active and `TextInputPlugin` when text input is active. The PR also prevents `FlutterView` stealing first responder status on mouse click events during text input. Previously when `TextInputClient` resigned it made `nextResponder` the first responder, but that was incorrect - `nextResponder` being the superview (`FlutterViewWrapper`). ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I signed the [CLA]. - [X] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
2023-09-29 11:02:42 +02:00
parent 4e5c685073
commit d4d0a41bd7
6 changed files with 73 additions and 20 deletions
--- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm
+++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm
@@ -384,10 +384,7 @@ static char markerKey;

 - (void)resignAndRemoveFromSuperview {
  if (self.superview != nil) {
-    // With accessiblity enabled TextInputPlugin is inside _client, so take the
-    // nextResponder from the _client.
-    NSResponder* nextResponder = _client != nil ? _client.nextResponder : self.nextResponder;
-    [self.window makeFirstResponder:nextResponder];
+    [self.window makeFirstResponder:_flutterViewController.flutterView];
    [self removeFromSuperview];
  }
 }
--- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputPluginTest.mm
+++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputPluginTest.mm
@@ -2049,6 +2049,41 @@ TEST(FlutterTextInputPluginTest, IsAddedAndRemovedFromViewHierarchy) {
  ASSERT_FALSE(window.firstResponder == viewController.textInputPlugin);
 }

+TEST(FlutterTextInputPluginTest, FirstResponderIsCorrect) {
+  FlutterEngine* engine = CreateTestEngine();
+  FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engine
+                                                                                nibName:nil
+                                                                                 bundle:nil];
+  [viewController loadView];
+
+  NSWindow* window = [[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 800, 600)
+                                                 styleMask:NSBorderlessWindowMask
+                                                   backing:NSBackingStoreBuffered
+                                                     defer:NO];
+  window.contentView = viewController.view;
+
+  ASSERT_TRUE(viewController.flutterView.acceptsFirstResponder);
+
+  [window makeFirstResponder:viewController.flutterView];
+
+  [viewController.textInputPlugin
+      handleMethodCall:[FlutterMethodCall methodCallWithMethodName:@"TextInput.show" arguments:@[]]
+                result:^(id){
+                }];
+
+  ASSERT_TRUE(window.firstResponder == viewController.textInputPlugin);
+
+  ASSERT_FALSE(viewController.flutterView.acceptsFirstResponder);
+
+  [viewController.textInputPlugin
+      handleMethodCall:[FlutterMethodCall methodCallWithMethodName:@"TextInput.hide" arguments:@[]]
+                result:^(id){
+                }];
+
+  ASSERT_TRUE(viewController.flutterView.acceptsFirstResponder);
+  ASSERT_TRUE(window.firstResponder == viewController.flutterView);
+}
+
 TEST(FlutterTextInputPluginTest, HasZeroSizeAndClipsToBounds) {
  id engineMock = flutter::testing::CreateMockFlutterEngine(@"");
  id binaryMessengerMock = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
--- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterView.h
+++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterView.h
@@ -23,13 +23,19 @@ typedef int64_t FlutterViewId;
 constexpr FlutterViewId kFlutterImplicitViewId = 0ll;

 /**
- * Listener for view resizing.
+ * Delegate for FlutterView.
 */
-@protocol FlutterViewReshapeListener <NSObject>
+@protocol FlutterViewDelegate <NSObject>
 /**
 * Called when the view's backing store changes size.
 */
 - (void)viewDidReshape:(nonnull NSView*)view;
+
+/**
+ * Called to determine whether the view should accept first responder status.
+ */
+- (BOOL)viewShouldAcceptFirstResponder:(nonnull NSView*)view;
+
@end

 /**
@@ -43,7 +49,7 @@ constexpr FlutterViewId kFlutterImplicitViewId = 0ll;
 */
 - (nullable instancetype)initWithMTLDevice:(nonnull id<MTLDevice>)device
                              commandQueue:(nonnull id<MTLCommandQueue>)commandQueue
-                           reshapeListener:(nonnull id<FlutterViewReshapeListener>)reshapeListener
+                                  delegate:(nonnull id<FlutterViewDelegate>)delegate
                        threadSynchronizer:(nonnull FlutterThreadSynchronizer*)threadSynchronizer
                                    viewId:(int64_t)viewId NS_DESIGNATED_INITIALIZER;

--- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterView.mm
+++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterView.mm
@@ -11,7 +11,7 @@

@interface FlutterView () <FlutterSurfaceManagerDelegate> {
  int64_t _viewId;
-  __weak id<FlutterViewReshapeListener> _reshapeListener;
+  __weak id<FlutterViewDelegate> _viewDelegate;
  FlutterThreadSynchronizer* _threadSynchronizer;
  FlutterSurfaceManager* _surfaceManager;
 }
@@ -22,7 +22,7 @@

 - (instancetype)initWithMTLDevice:(id<MTLDevice>)device
                     commandQueue:(id<MTLCommandQueue>)commandQueue
-                  reshapeListener:(id<FlutterViewReshapeListener>)reshapeListener
+                         delegate:(id<FlutterViewDelegate>)delegate
               threadSynchronizer:(FlutterThreadSynchronizer*)threadSynchronizer
                           viewId:(int64_t)viewId {
  self = [super initWithFrame:NSZeroRect];
@@ -31,7 +31,7 @@
    [self setBackgroundColor:[NSColor blackColor]];
    [self setLayerContentsRedrawPolicy:NSViewLayerContentsRedrawDuringViewResize];
    _viewId = viewId;
-    _reshapeListener = reshapeListener;
+    _viewDelegate = delegate;
    _threadSynchronizer = threadSynchronizer;
    _surfaceManager = [[FlutterSurfaceManager alloc] initWithDevice:device
                                                       commandQueue:commandQueue
@@ -54,7 +54,7 @@
  [_threadSynchronizer beginResizeForView:_viewId
                                     size:scaledSize
                                   notify:^{
-                                     [_reshapeListener viewDidReshape:self];
+                                     [_viewDelegate viewDidReshape:self];
                                   }];
 }

@@ -89,7 +89,9 @@
 }

 - (BOOL)acceptsFirstResponder {
-  return YES;
+  // This is to ensure that FlutterView does not take first responder status from TextInputPlugin
+  // on mouse clicks.
+  return [_viewDelegate viewShouldAcceptFirstResponder:self];
 }

 - (void)cursorUpdate:(NSEvent*)event {
@@ -104,7 +106,7 @@
 - (void)viewDidChangeBackingProperties {
  [super viewDidChangeBackingProperties];
  // Force redraw
-  [_reshapeListener viewDidReshape:self];
+  [_viewDelegate viewDidReshape:self];
 }

 - (BOOL)layer:(CALayer*)layer
--- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
+++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
@@ -169,7 +169,7 @@ NSData* currentKeyboardLayoutData() {
 /**
 * Private interface declaration for FlutterViewController.
 */
-@interface FlutterViewController () <FlutterViewReshapeListener>
+@interface FlutterViewController () <FlutterViewDelegate>

 /**
 * The tracking area used to generate hover events, if enabled.
@@ -831,7 +831,7 @@ static void CommonInit(FlutterViewController* controller, FlutterEngine* engine)
                                          commandQueue:(id<MTLCommandQueue>)commandQueue {
  return [[FlutterView alloc] initWithMTLDevice:device
                                   commandQueue:commandQueue
-                                reshapeListener:self
+                                       delegate:self
                             threadSynchronizer:_threadSynchronizer
                                         viewId:_viewId];
 }
@@ -851,15 +851,24 @@ static void CommonInit(FlutterViewController* controller, FlutterEngine* engine)
  return [FlutterDartProject lookupKeyForAsset:asset fromPackage:package];
 }

-#pragma mark - FlutterViewReshapeListener
+#pragma mark - FlutterViewDelegate

 /**
 * Responds to view reshape by notifying the engine of the change in dimensions.
 */
 - (void)viewDidReshape:(NSView*)view {
+  FML_DCHECK(view == _flutterView);
  [_engine updateWindowMetricsForViewController:self];
 }

+- (BOOL)viewShouldAcceptFirstResponder:(NSView*)view {
+  FML_DCHECK(view == _flutterView);
+  // Only allow FlutterView to become first responder if TextInputPlugin is
+  // not active. Otherwise a mouse event inside FlutterView would cause the
+  // TextInputPlugin to lose first responder status.
+  return !_textInputPlugin.isFirstResponder;
+}
+
 #pragma mark - FlutterPluginRegistry

 - (id<FlutterPluginRegistrar>)registrarForPlugin:(NSString*)pluginName {
--- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewTest.mm
+++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewTest.mm
@@ -10,25 +10,29 @@

 constexpr int64_t kImplicitViewId = 0ll;

-@interface TestReshapeListener : NSObject <FlutterViewReshapeListener>
+@interface TestFlutterViewDelegate : NSObject <FlutterViewDelegate>

@end

-@implementation TestReshapeListener
+@implementation TestFlutterViewDelegate

 - (void)viewDidReshape:(nonnull NSView*)view {
 }

+- (BOOL)viewShouldAcceptFirstResponder:(NSView*)view {
+  return YES;
+}
+
@end

 TEST(FlutterView, ShouldInheritContentsScaleReturnsYes) {
  id<MTLDevice> device = MTLCreateSystemDefaultDevice();
  id<MTLCommandQueue> queue = [device newCommandQueue];
-  TestReshapeListener* listener = [[TestReshapeListener alloc] init];
+  TestFlutterViewDelegate* delegate = [[TestFlutterViewDelegate alloc] init];
  FlutterThreadSynchronizer* threadSynchronizer = [[FlutterThreadSynchronizer alloc] init];
  FlutterView* view = [[FlutterView alloc] initWithMTLDevice:device
                                                commandQueue:queue
-                                             reshapeListener:listener
+                                                    delegate:delegate
                                          threadSynchronizer:threadSynchronizer
                                                      viewId:kImplicitViewId];
  EXPECT_EQ([view layer:view.layer shouldInheritContentsScale:3.0 fromWindow:view.window], YES);