From 01de84bfb0938e15944e08cdb316682bb413f4b0 Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Wed, 16 Sep 2020 17:45:04 -0700 Subject: [PATCH 1/4] Add delayed key event propagation to macOS --- .../framework/Headers/FlutterViewController.h | 16 +- .../framework/Source/FlutterViewController.mm | 90 +++++++-- .../Source/FlutterViewControllerTest.mm | 183 +++++++++++++++++- testing/run_tests.py | 4 +- 4 files changed, 273 insertions(+), 20 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h b/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h index b32af0ac32644..d66c50162858b 100644 --- a/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h +++ b/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h @@ -39,6 +39,19 @@ FLUTTER_EXPORT */ @property(nonatomic) FlutterMouseTrackingMode mouseTrackingMode; +/** + * Initializes this FlutterViewController with the specified `FlutterEngine`. + * + * The initialized viewcontroller will attach itself to the engine as part of this process. + * + * @param engine The `FlutterEngine` instance to attach to. Cannot be nil. + * @param nibName The NIB name to initialize this controller with. + * @param nibBundle The NIB bundle. + */ +- (nonnull instancetype)initWithEngine:(nonnull FlutterEngine*)engine + nibName:(nullable NSString*)nibName + bundle:(nullable NSBundle*)nibBundle NS_DESIGNATED_INITIALIZER; + /** * Initializes a controller that will run the given project. * @@ -51,6 +64,7 @@ FLUTTER_EXPORT - (nonnull instancetype)initWithNibName:(nullable NSString*)nibNameOrNil bundle:(nullable NSBundle*)nibBundleOrNil NS_DESIGNATED_INITIALIZER; -- (nonnull instancetype)initWithCoder:(nonnull NSCoder*)nibNameOrNil NS_DESIGNATED_INITIALIZER; + +- (nonnull instancetype)initWithCoder:(nonnull NSCoder*)coder NS_DESIGNATED_INITIALIZER; @end diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm index cc5eaba49bf39..18c79496096d2 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm @@ -135,7 +135,14 @@ - (void)dispatchMouseEvent:(nonnull NSEvent*)event; - (void)dispatchMouseEvent:(nonnull NSEvent*)event phase:(FlutterPointerPhase)phase; /** - * Converts |event| to a key event channel message, and sends it to the engine. + * Sends |event| to all responders in additionalKeyResponders and then to the nextResponder. + */ +- (void)propagateKeyEvent:(NSEvent*)event ofType:(NSString*)type; + +/** + * Converts |event| to a key event channel message, and sends it to the engine to + * hand to the framework. Once the framework responds, if the event was not handled, + * propagates the event to any additional key responders. */ - (void)dispatchKeyEvent:(NSEvent*)event ofType:(NSString*)type; @@ -206,9 +213,11 @@ @implementation FlutterViewController { * Performs initialization that's common between the different init paths. */ static void CommonInit(FlutterViewController* controller) { - controller->_engine = [[FlutterEngine alloc] initWithName:@"io.flutter" - project:controller->_project - allowHeadlessExecution:NO]; + if (controller->_engine == nullptr) { + controller->_engine = [[FlutterEngine alloc] initWithName:@"io.flutter" + project:controller->_project + allowHeadlessExecution:NO]; + } controller->_additionalKeyResponders = [[NSMutableOrderedSet alloc] init]; controller->_mouseTrackingMode = FlutterMouseTrackingModeInKeyWindow; } @@ -238,6 +247,27 @@ - (instancetype)initWithProject:(nullable FlutterDartProject*)project { return self; } +- (instancetype)initWithEngine:(nonnull FlutterEngine*)engine + nibName:(nullable NSString*)nibName + bundle:(nullable NSBundle*)nibBundle { + NSAssert(engine != nil, @"Engine is required"); + self = [super initWithNibName:nibName bundle:nibBundle]; + if (self) { + if (engine.viewController) { + NSLog(@"The supplied FlutterEngine %s is already used with FlutterViewController " + "instance %s. One instance of the FlutterEngine can only be attached to one " + "FlutterViewController at a time. Set FlutterEngine.viewController " + "to nil before attaching it to another FlutterViewController.", + [[engine description] UTF8String], [[engine.viewController description] UTF8String]); + } + _engine = engine; + CommonInit(self); + [engine setViewController:self]; + } + + return self; +} + - (void)loadView { NSOpenGLContext* resourceContext = _engine.resourceContext; if (!resourceContext) { @@ -293,6 +323,7 @@ - (void)addKeyResponder:(NSResponder*)responder { } - (void)removeKeyResponder:(NSResponder*)responder { + [self.additionalKeyResponders removeObject:responder]; } #pragma mark - Private methods @@ -460,19 +491,56 @@ - (void)dispatchMouseEvent:(NSEvent*)event phase:(FlutterPointerPhase)phase { } } +- (void)propagateKeyEvent:(NSEvent*)event ofType:(NSString*)type { + if ([type isEqual:@"keydown"]) { + for (NSResponder* responder in self.additionalKeyResponders) { + if ([responder respondsToSelector:@selector(keyDown:)]) { + [responder keyDown:event]; + } + } + if ([self.nextResponder respondsToSelector:@selector(keyDown:)]) { + [self.nextResponder keyDown:event]; + } + } else if ([type isEqual:@"keyup"]) { + for (NSResponder* responder in self.additionalKeyResponders) { + if ([responder respondsToSelector:@selector(keyUp:)]) { + [responder keyUp:event]; + } + } + if ([self.nextResponder respondsToSelector:@selector(keyUp:)]) { + [self.nextResponder keyUp:event]; + } + } +} + - (void)dispatchKeyEvent:(NSEvent*)event ofType:(NSString*)type { + if (event.type != NSEventTypeKeyDown && event.type != NSEventTypeKeyUp && + event.type != NSEventTypeFlagsChanged) { + return; + } NSMutableDictionary* keyMessage = [@{ @"keymap" : @"macos", @"type" : type, @"keyCode" : @(event.keyCode), @"modifiers" : @(event.modifierFlags), } mutableCopy]; - // Calling these methods on any other type of event will raise an exception. + // Calling these methods on any other type of event + // (e.g NSEventTypeFlagsChanged) will raise an exception. if (event.type == NSEventTypeKeyDown || event.type == NSEventTypeKeyUp) { keyMessage[@"characters"] = event.characters; keyMessage[@"charactersIgnoringModifiers"] = event.charactersIgnoringModifiers; } - [_keyEventChannel sendMessage:keyMessage]; + __weak __typeof__(self) weakSelf = self; + FlutterReply replyHandler = ^(id _Nullable reply) { + if (!reply) { + return; + } + // Only re-dispatch the event to other responders if the framework didn't handle it. + if (![[reply valueForKey:@"handled"] boolValue]) { + [weakSelf propagateKeyEvent:event ofType:type]; + } + }; + [_keyEventChannel sendMessage:keyMessage reply:replyHandler]; } - (void)onSettingsChanged:(NSNotification*)notification { @@ -571,20 +639,10 @@ - (BOOL)acceptsFirstResponder { - (void)keyDown:(NSEvent*)event { [self dispatchKeyEvent:event ofType:@"keydown"]; - for (NSResponder* responder in self.additionalKeyResponders) { - if ([responder respondsToSelector:@selector(keyDown:)]) { - [responder keyDown:event]; - } - } } - (void)keyUp:(NSEvent*)event { [self dispatchKeyEvent:event ofType:@"keyup"]; - for (NSResponder* responder in self.additionalKeyResponders) { - if ([responder respondsToSelector:@selector(keyUp:)]) { - [responder keyUp:event]; - } - } } - (void)flagsChanged:(NSEvent*)event { diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm index 671520f494923..b11a79c7e1640 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "FlutterBinaryMessenger.h" #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h" #import @@ -12,8 +13,39 @@ #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTestUtils.h" #include "flutter/testing/testing.h" +@interface FlutterViewControllerTestObjC : NSObject +- (bool)testKeyEventsAreSentToFramework; +- (bool)testKeyEventsArePropagatedIfNotHandled; +- (bool)testKeyEventsAreNotPropagatedIfHandled; +@end + namespace flutter::testing { +// Returns a mock FlutterViewController that is able to work in environments +// without a real pasteboard. +id mockViewController(NSString* pasteboardString) { + NSString* fixtures = @(testing::GetFixturesPath()); + FlutterDartProject* project = [[FlutterDartProject alloc] + initWithAssetsPath:fixtures + ICUDataPath:[fixtures stringByAppendingString:@"/icudtl.dat"]]; + FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project]; + + // Mock pasteboard so that this test will work in environments without a + // real pasteboard. + id pasteboardMock = OCMClassMock([NSPasteboard class]); + OCMExpect( // NOLINT(google-objc-avoid-throwing-exception) + [pasteboardMock stringForType:[OCMArg any]]) + .andDo(^(NSInvocation* invocation) { + NSString* returnValue = pasteboardString.length > 0 ? pasteboardString : nil; + [invocation setReturnValue:&returnValue]; + }); + id viewControllerMock = OCMPartialMock(viewController); + OCMStub( // NOLINT(google-objc-avoid-throwing-exception) + [viewControllerMock pasteboard]) + .andReturn(pasteboardMock); + return viewControllerMock; +} + TEST(FlutterViewController, HasStringsWhenPasteboardEmpty) { // Mock FlutterViewController so that it behaves like the pasteboard is empty. id viewControllerMock = CreateMockViewController(nil); @@ -53,4 +85,153 @@ EXPECT_TRUE(value); } -} // flutter::testing +TEST(FlutterViewControllerTest, TestKeyEventsAreSentToFramework) { + ASSERT_TRUE([[FlutterViewControllerTestObjC alloc] testKeyEventsAreSentToFramework]); +} + +TEST(FlutterViewControllerTest, TestKeyEventsArePropagatedIfNotHandled) { + ASSERT_TRUE([[FlutterViewControllerTestObjC alloc] testKeyEventsArePropagatedIfNotHandled]); +} + +TEST(FlutterViewControllerTest, TestKeyEventsAreNotPropagatedIfHandled) { + ASSERT_TRUE([[FlutterViewControllerTestObjC alloc] testKeyEventsAreNotPropagatedIfHandled]); +} + +} // namespace flutter::testing + +@implementation FlutterViewControllerTestObjC + +- (bool)testKeyEventsAreSentToFramework { + id engineMock = OCMClassMock([FlutterEngine class]); + id binaryMessengerMock = OCMProtocolMock(@protocol(FlutterBinaryMessenger)); + OCMStub( // NOLINT(google-objc-avoid-throwing-exception) + [engineMock binaryMessenger]) + .andReturn(binaryMessengerMock); + FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engineMock + nibName:@"" + bundle:nil]; + NSDictionary* expectedEvent = @{ + @"keymap" : @"macos", + @"type" : @"keydown", + @"keyCode" : @(65), + @"modifiers" : @(538968064), + @"characters" : @".", + @"charactersIgnoringModifiers" : @".", + }; + NSData* encodedKeyEvent = [[FlutterJSONMessageCodec sharedInstance] encode:expectedEvent]; + CGEventRef cgEvent = CGEventCreateKeyboardEvent(NULL, 65, TRUE); + NSEvent* event = [NSEvent eventWithCGEvent:cgEvent]; + [viewController viewWillAppear]; // Initializes the event channel. + [viewController keyDown:event]; + @try { + OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) + [binaryMessengerMock sendOnChannel:@"flutter/keyevent" + message:encodedKeyEvent + binaryReply:[OCMArg any]]); + } @catch (...) { + return false; + } + return true; +} + +- (bool)testKeyEventsArePropagatedIfNotHandled { + id engineMock = OCMClassMock([FlutterEngine class]); + id binaryMessengerMock = OCMProtocolMock(@protocol(FlutterBinaryMessenger)); + OCMStub( // NOLINT(google-objc-avoid-throwing-exception) + [engineMock binaryMessenger]) + .andReturn(binaryMessengerMock); + FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engineMock + nibName:@"" + bundle:nil]; + id responderMock = OCMClassMock([NSResponder class]); + [viewController addKeyResponder:responderMock]; + NSDictionary* expectedEvent = @{ + @"keymap" : @"macos", + @"type" : @"keydown", + @"keyCode" : @(65), + @"modifiers" : @(538968064), + @"characters" : @".", + @"charactersIgnoringModifiers" : @".", + }; + NSData* encodedKeyEvent = [[FlutterJSONMessageCodec sharedInstance] encode:expectedEvent]; + CGEventRef cgEvent = CGEventCreateKeyboardEvent(NULL, 65, TRUE); + NSEvent* event = [NSEvent eventWithCGEvent:cgEvent]; + OCMExpect( // NOLINT(google-objc-avoid-throwing-exception) + [binaryMessengerMock sendOnChannel:@"flutter/keyevent" + message:encodedKeyEvent + binaryReply:[OCMArg any]]) + .andDo((^(NSInvocation* invocation) { + FlutterBinaryReply handler; + [invocation getArgument:&handler atIndex:4]; + NSDictionary* reply = @{ + @"handled" : @(false), + }; + NSData* encodedReply = [[FlutterJSONMessageCodec sharedInstance] encode:reply]; + handler(encodedReply); + })); + [viewController viewWillAppear]; // Initializes the event channel. + [viewController keyDown:event]; + @try { + OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) + [responderMock keyDown:[OCMArg any]]); + OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) + [binaryMessengerMock sendOnChannel:@"flutter/keyevent" + message:encodedKeyEvent + binaryReply:[OCMArg any]]); + } @catch (...) { + return false; + } + return true; +} + +- (bool)testKeyEventsAreNotPropagatedIfHandled { + id engineMock = OCMClassMock([FlutterEngine class]); + id binaryMessengerMock = OCMProtocolMock(@protocol(FlutterBinaryMessenger)); + OCMStub( // NOLINT(google-objc-avoid-throwing-exception) + [engineMock binaryMessenger]) + .andReturn(binaryMessengerMock); + FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engineMock + nibName:@"" + bundle:nil]; + id responderMock = OCMClassMock([NSResponder class]); + [viewController addKeyResponder:responderMock]; + NSDictionary* expectedEvent = @{ + @"keymap" : @"macos", + @"type" : @"keydown", + @"keyCode" : @(65), + @"modifiers" : @(538968064), + @"characters" : @".", + @"charactersIgnoringModifiers" : @".", + }; + NSData* encodedKeyEvent = [[FlutterJSONMessageCodec sharedInstance] encode:expectedEvent]; + CGEventRef cgEvent = CGEventCreateKeyboardEvent(NULL, 65, TRUE); + NSEvent* event = [NSEvent eventWithCGEvent:cgEvent]; + OCMExpect( // NOLINT(google-objc-avoid-throwing-exception) + [binaryMessengerMock sendOnChannel:@"flutter/keyevent" + message:encodedKeyEvent + binaryReply:[OCMArg any]]) + .andDo((^(NSInvocation* invocation) { + FlutterBinaryReply handler; + [invocation getArgument:&handler atIndex:4]; + NSDictionary* reply = @{ + @"handled" : @(true), + }; + NSData* encodedReply = [[FlutterJSONMessageCodec sharedInstance] encode:reply]; + handler(encodedReply); + })); + [viewController viewWillAppear]; // Initializes the event channel. + [viewController keyDown:event]; + @try { + OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) + never(), [responderMock keyDown:[OCMArg any]]); + OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) + [binaryMessengerMock sendOnChannel:@"flutter/keyevent" + message:encodedKeyEvent + binaryReply:[OCMArg any]]); + } @catch (...) { + return false; + } + return true; +} + +@end diff --git a/testing/run_tests.py b/testing/run_tests.py index ca06939e2241b..4a0831d577593 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -399,7 +399,7 @@ def RunDartTests(build_dir, filter, verbose_dart_snapshot): # This one is a bit messy. The pubspec.yaml at flutter/testing/dart/pubspec.yaml # has dependencies that are hardcoded to point to the sky packages at host_debug_unopt/ # Before running Dart tests, make sure to run just that target (NOT the whole engine) - EnsureDebugUnoptSkyPackagesAreBuilt(); + EnsureDebugUnoptSkyPackagesAreBuilt() # Now that we have the Sky packages at the hardcoded location, run `pub get`. RunEngineExecutable(build_dir, os.path.join('dart-sdk', 'bin', 'pub'), None, flags=['get'], cwd=dart_tests_dir) @@ -443,7 +443,7 @@ def main(): parser = argparse.ArgumentParser() parser.add_argument('--variant', dest='variant', action='store', - default='host_debug_unopt', help='The engine build variant to run the tests for.'); + default='host_debug_unopt', help='The engine build variant to run the tests for.') parser.add_argument('--type', type=str, default='all') parser.add_argument('--engine-filter', type=str, default='', help='A list of engine test executables to run.') From 272d33dfa1eb055c3dd46a3d27ef1a4c2ed3b42e Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Tue, 1 Dec 2020 14:23:55 -0800 Subject: [PATCH 2/4] Replace NSResponder for custom class that can handle return type --- ci/licenses_golden/licenses_flutter | 2 ++ shell/platform/darwin/macos/BUILD.gn | 2 ++ .../Source/FlutterIntermediateKeyResponder.h | 26 +++++++++++++++++++ .../Source/FlutterIntermediateKeyResponder.mm | 19 ++++++++++++++ .../framework/Source/FlutterTextInputPlugin.h | 3 ++- .../Source/FlutterTextInputPlugin.mm | 11 +++++--- .../framework/Source/FlutterViewController.mm | 22 +++++++++------- .../Source/FlutterViewControllerTest.mm | 8 +++--- .../Source/FlutterViewController_Internal.h | 11 ++++---- 9 files changed, 81 insertions(+), 23 deletions(-) create mode 100644 shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h create mode 100644 shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.mm diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index e3463cd6c57a4..51708799d5827 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -1073,6 +1073,8 @@ FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterGLCom FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterGLCompositorUnittests.mm FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterIOSurfaceHolder.h FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterIOSurfaceHolder.mm +FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h +FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.mm FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterMouseCursorPlugin.h FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterMouseCursorPlugin.mm FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h diff --git a/shell/platform/darwin/macos/BUILD.gn b/shell/platform/darwin/macos/BUILD.gn index 79996675a9965..88a65da11f07c 100644 --- a/shell/platform/darwin/macos/BUILD.gn +++ b/shell/platform/darwin/macos/BUILD.gn @@ -60,6 +60,8 @@ source_set("flutter_framework_source") { "framework/Source/FlutterGLCompositor.mm", "framework/Source/FlutterIOSurfaceHolder.h", "framework/Source/FlutterIOSurfaceHolder.mm", + "framework/Source/FlutterIntermediateKeyResponder.h", + "framework/Source/FlutterIntermediateKeyResponder.mm", "framework/Source/FlutterMouseCursorPlugin.h", "framework/Source/FlutterMouseCursorPlugin.mm", "framework/Source/FlutterResizeSynchronizer.h", diff --git a/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h b/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h new file mode 100644 index 0000000000000..f3c6a84b6df0c --- /dev/null +++ b/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h @@ -0,0 +1,26 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#import + +/* + * An interface for adding key responders that are processed after the framework + * has had a chance to handle the key, but before the next responder is given + * the key. + * + * It differs from an NSResponder in that it returns a boolean from the keyUp + * and keyDown calls, allowing the callee to indicate whether or not it handled + * the given event. If handled, then the event is not passed to the next + * responder. + */ +@interface FlutterIntermediateKeyResponder : NSObject +/* + * Informs the receiver that the user has released a key. + */ +- (BOOL)handleKeyUp:(NSEvent*)event; +/* + * Informs the receiver that the user has pressed a key. + */ +- (BOOL)handleKeyDown:(NSEvent*)event; +@end diff --git a/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.mm b/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.mm new file mode 100644 index 0000000000000..71dd8c87cd7e4 --- /dev/null +++ b/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.mm @@ -0,0 +1,19 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h" + +@implementation FlutterIntermediateKeyResponder { +} + +#pragma mark - Default key handling methods + +- (BOOL)handleKeyUp:(NSEvent*)event { + return NO; +} + +- (BOOL)handleKeyDown:(NSEvent*)event { + return NO; +} +@end diff --git a/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.h b/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.h index e75cff8475699..2f4a5436e74d7 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.h @@ -6,6 +6,7 @@ #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h" #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h" +#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h" /** * A plugin to handle text input. @@ -16,7 +17,7 @@ * This is not an FlutterPlugin since it needs access to FlutterViewController internals, so needs * to be managed differently. */ -@interface FlutterTextInputPlugin : NSResponder +@interface FlutterTextInputPlugin : FlutterIntermediateKeyResponder /** * Initializes a text input plugin that coordinates key event handling with |viewController|. diff --git a/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm b/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm index 515e3772e1b40..87086290c7e6c 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm @@ -129,17 +129,20 @@ - (void)updateEditState { } #pragma mark - -#pragma mark NSResponder +#pragma mark FlutterIntermediateKeyResponder /** + * Handles key down events received from the view controller, responding TRUE if + * the event was handled. + * * Note, the Apple docs suggest that clients should override essentially all the * mouse and keyboard event-handling methods of NSResponder. However, experimentation * indicates that only key events are processed by the native layer; Flutter processes * mouse events. Additionally, processing both keyUp and keyDown results in duplicate - * processing of the same keys. So for now, limit processing to just keyDown. + * processing of the same keys. So for now, limit processing to just handleKeyDown. */ -- (void)keyDown:(NSEvent*)event { - [_textInputContext handleEvent:event]; +- (BOOL)handleKeyDown:(NSEvent*)event { + return [_textInputContext handleEvent:event]; } #pragma mark - diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm index 18c79496096d2..50375d9cb079b 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm @@ -84,7 +84,7 @@ @interface FlutterViewController () /** * A list of additional responders to keyboard events. Keybord events are forwarded to all of them. */ -@property(nonatomic) NSMutableOrderedSet* additionalKeyResponders; +@property(nonatomic) NSMutableOrderedSet* additionalKeyResponders; /** * The tracking area used to generate hover events, if enabled. @@ -318,11 +318,11 @@ - (FlutterView*)flutterView { return static_cast(self.view); } -- (void)addKeyResponder:(NSResponder*)responder { +- (void)addKeyResponder:(FlutterIntermediateKeyResponder*)responder { [self.additionalKeyResponders addObject:responder]; } -- (void)removeKeyResponder:(NSResponder*)responder { +- (void)removeKeyResponder:(FlutterIntermediateKeyResponder*)responder { [self.additionalKeyResponders removeObject:responder]; } @@ -493,18 +493,22 @@ - (void)dispatchMouseEvent:(NSEvent*)event phase:(FlutterPointerPhase)phase { - (void)propagateKeyEvent:(NSEvent*)event ofType:(NSString*)type { if ([type isEqual:@"keydown"]) { - for (NSResponder* responder in self.additionalKeyResponders) { - if ([responder respondsToSelector:@selector(keyDown:)]) { - [responder keyDown:event]; + for (FlutterIntermediateKeyResponder* responder in self.additionalKeyResponders) { + if ([responder respondsToSelector:@selector(handleKeyDown:)]) { + if ([responder handleKeyDown:event]) { + return; + } } } if ([self.nextResponder respondsToSelector:@selector(keyDown:)]) { [self.nextResponder keyDown:event]; } } else if ([type isEqual:@"keyup"]) { - for (NSResponder* responder in self.additionalKeyResponders) { - if ([responder respondsToSelector:@selector(keyUp:)]) { - [responder keyUp:event]; + for (FlutterIntermediateKeyResponder* responder in self.additionalKeyResponders) { + if ([responder respondsToSelector:@selector(handleKeyUp:)]) { + if ([responder handleKeyUp:event]) { + return; + } } } if ([self.nextResponder respondsToSelector:@selector(keyUp:)]) { diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm index b11a79c7e1640..87c7a0cd356f8 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm @@ -143,7 +143,7 @@ - (bool)testKeyEventsArePropagatedIfNotHandled { FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engineMock nibName:@"" bundle:nil]; - id responderMock = OCMClassMock([NSResponder class]); + id responderMock = OCMClassMock([FlutterIntermediateKeyResponder class]); [viewController addKeyResponder:responderMock]; NSDictionary* expectedEvent = @{ @"keymap" : @"macos", @@ -173,7 +173,7 @@ - (bool)testKeyEventsArePropagatedIfNotHandled { [viewController keyDown:event]; @try { OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) - [responderMock keyDown:[OCMArg any]]); + [responderMock handleKeyDown:[OCMArg any]]); OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) [binaryMessengerMock sendOnChannel:@"flutter/keyevent" message:encodedKeyEvent @@ -193,7 +193,7 @@ - (bool)testKeyEventsAreNotPropagatedIfHandled { FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engineMock nibName:@"" bundle:nil]; - id responderMock = OCMClassMock([NSResponder class]); + id responderMock = OCMClassMock([FlutterIntermediateKeyResponder class]); [viewController addKeyResponder:responderMock]; NSDictionary* expectedEvent = @{ @"keymap" : @"macos", @@ -223,7 +223,7 @@ - (bool)testKeyEventsAreNotPropagatedIfHandled { [viewController keyDown:event]; @try { OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) - never(), [responderMock keyDown:[OCMArg any]]); + never(), [responderMock handleKeyDown:[OCMArg any]]); OCMVerify( // NOLINT(google-objc-avoid-throwing-exception) [binaryMessengerMock sendOnChannel:@"flutter/keyevent" message:encodedKeyEvent diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h b/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h index a2202fe9498a9..599796bc321f0 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h @@ -4,6 +4,7 @@ #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h" +#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h" #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterView.h" @interface FlutterViewController () @@ -17,14 +18,14 @@ @property(nonatomic, readonly, nonnull) NSPasteboard* pasteboard; /** - * Adds a responder for keyboard events. Key up and key down events are forwarded to all added - * responders. + * Adds an intermediate responder for keyboard events. Key up and key down events are forwarded to + * all added responders, and they either handle the keys or not. */ -- (void)addKeyResponder:(nonnull NSResponder*)responder; +- (void)addKeyResponder:(nonnull FlutterIntermediateKeyResponder*)responder; /** - * Removes a responder for keyboard events. + * Removes an intermediate responder for keyboard events. */ -- (void)removeKeyResponder:(nonnull NSResponder*)responder; +- (void)removeKeyResponder:(nonnull FlutterIntermediateKeyResponder*)responder; @end From b83595eb9827f57c2bd6ebcfe179e59bf37b7299 Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Thu, 3 Dec 2020 15:34:39 -0800 Subject: [PATCH 3/4] Review Changes --- .../framework/Headers/FlutterViewController.h | 16 +------------- .../Source/FlutterIntermediateKeyResponder.h | 12 +++++----- .../framework/Source/FlutterViewController.mm | 22 ++++++++++--------- .../Source/FlutterViewController_Internal.h | 12 ++++++++++ 4 files changed, 30 insertions(+), 32 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h b/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h index d66c50162858b..b32af0ac32644 100644 --- a/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h +++ b/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h @@ -39,19 +39,6 @@ FLUTTER_EXPORT */ @property(nonatomic) FlutterMouseTrackingMode mouseTrackingMode; -/** - * Initializes this FlutterViewController with the specified `FlutterEngine`. - * - * The initialized viewcontroller will attach itself to the engine as part of this process. - * - * @param engine The `FlutterEngine` instance to attach to. Cannot be nil. - * @param nibName The NIB name to initialize this controller with. - * @param nibBundle The NIB bundle. - */ -- (nonnull instancetype)initWithEngine:(nonnull FlutterEngine*)engine - nibName:(nullable NSString*)nibName - bundle:(nullable NSBundle*)nibBundle NS_DESIGNATED_INITIALIZER; - /** * Initializes a controller that will run the given project. * @@ -64,7 +51,6 @@ FLUTTER_EXPORT - (nonnull instancetype)initWithNibName:(nullable NSString*)nibNameOrNil bundle:(nullable NSBundle*)nibBundleOrNil NS_DESIGNATED_INITIALIZER; - -- (nonnull instancetype)initWithCoder:(nonnull NSCoder*)coder NS_DESIGNATED_INITIALIZER; +- (nonnull instancetype)initWithCoder:(nonnull NSCoder*)nibNameOrNil NS_DESIGNATED_INITIALIZER; @end diff --git a/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h b/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h index f3c6a84b6df0c..f183b95b98b21 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h @@ -5,14 +5,12 @@ #import /* - * An interface for adding key responders that are processed after the framework - * has had a chance to handle the key, but before the next responder is given - * the key. + * An interface for adding key responders that can declare itself as the final + * responder of the event, terminating the event propagation. * - * It differs from an NSResponder in that it returns a boolean from the keyUp - * and keyDown calls, allowing the callee to indicate whether or not it handled - * the given event. If handled, then the event is not passed to the next - * responder. + * It differs from an NSResponder in that it returns a boolean from the + * handleKeyUp and handleKeyDown calls, where true means it has handled the + * given event. A handled event is not passed to the next responder. */ @interface FlutterIntermediateKeyResponder : NSObject /* diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm index 50375d9cb079b..f41ff803e7d84 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm @@ -82,7 +82,12 @@ void Reset() { @interface FlutterViewController () /** - * A list of additional responders to keyboard events. Keybord events are forwarded to all of them. + * A list of additional responders to keyboard events. + * + * Keyboard events received by FlutterViewController are first dispatched to + * each additional responder in order. If any of them handle the event (by + * returning true), the event is not dispatched to later additional responders + * or to the nextResponder. */ @property(nonatomic) NSMutableOrderedSet* additionalKeyResponders; @@ -135,7 +140,8 @@ - (void)dispatchMouseEvent:(nonnull NSEvent*)event; - (void)dispatchMouseEvent:(nonnull NSEvent*)event phase:(FlutterPointerPhase)phase; /** - * Sends |event| to all responders in additionalKeyResponders and then to the nextResponder. + * Sends |event| to all responders in additionalKeyResponders and then to the + * nextResponder if none of the additional responders handled the event. */ - (void)propagateKeyEvent:(NSEvent*)event ofType:(NSString*)type; @@ -494,10 +500,8 @@ - (void)dispatchMouseEvent:(NSEvent*)event phase:(FlutterPointerPhase)phase { - (void)propagateKeyEvent:(NSEvent*)event ofType:(NSString*)type { if ([type isEqual:@"keydown"]) { for (FlutterIntermediateKeyResponder* responder in self.additionalKeyResponders) { - if ([responder respondsToSelector:@selector(handleKeyDown:)]) { - if ([responder handleKeyDown:event]) { - return; - } + if ([responder handleKeyDown:event]) { + return; } } if ([self.nextResponder respondsToSelector:@selector(keyDown:)]) { @@ -505,10 +509,8 @@ - (void)propagateKeyEvent:(NSEvent*)event ofType:(NSString*)type { } } else if ([type isEqual:@"keyup"]) { for (FlutterIntermediateKeyResponder* responder in self.additionalKeyResponders) { - if ([responder respondsToSelector:@selector(handleKeyUp:)]) { - if ([responder handleKeyUp:event]) { - return; - } + if ([responder handleKeyUp:event]) { + return; } } if ([self.nextResponder respondsToSelector:@selector(keyUp:)]) { diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h b/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h index 599796bc321f0..acef9f23bae5e 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h @@ -28,4 +28,16 @@ */ - (void)removeKeyResponder:(nonnull FlutterIntermediateKeyResponder*)responder; +/** + * Initializes this FlutterViewController with the specified `FlutterEngine`. + * + * The initialized viewcontroller will attach itself to the engine as part of this process. + * + * @param engine The `FlutterEngine` instance to attach to. Cannot be nil. + * @param nibName The NIB name to initialize this controller with. + * @param nibBundle The NIB bundle. + */ +- (nonnull instancetype)initWithEngine:(nonnull FlutterEngine*)engine + nibName:(nullable NSString*)nibName + bundle:(nullable NSBundle*)nibBundle NS_DESIGNATED_INITIALIZER; @end From 96035f5982c0c6bd46e6c06027ef8e4a7394cb49 Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Fri, 11 Dec 2020 12:04:14 -0800 Subject: [PATCH 4/4] Review Changes --- .../Source/FlutterIntermediateKeyResponder.h | 12 ++++++++---- .../macos/framework/Source/FlutterViewController.mm | 8 ++++---- .../framework/Source/FlutterViewControllerTest.mm | 5 +++-- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h b/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h index f183b95b98b21..31113ec8dfef8 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h @@ -5,20 +5,24 @@ #import /* - * An interface for adding key responders that can declare itself as the final + * An interface for a key responder that can declare itself as the final * responder of the event, terminating the event propagation. * * It differs from an NSResponder in that it returns a boolean from the * handleKeyUp and handleKeyDown calls, where true means it has handled the - * given event. A handled event is not passed to the next responder. + * given event. */ @interface FlutterIntermediateKeyResponder : NSObject /* * Informs the receiver that the user has released a key. + * + * Default implementation returns NO. */ -- (BOOL)handleKeyUp:(NSEvent*)event; +- (BOOL)handleKeyUp:(nonnull NSEvent*)event; /* * Informs the receiver that the user has pressed a key. + * + * Default implementation returns NO. */ -- (BOOL)handleKeyDown:(NSEvent*)event; +- (BOOL)handleKeyDown:(nonnull NSEvent*)event; @end diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm index f41ff803e7d84..b0ce1f4c9fde9 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm @@ -219,7 +219,7 @@ @implementation FlutterViewController { * Performs initialization that's common between the different init paths. */ static void CommonInit(FlutterViewController* controller) { - if (controller->_engine == nullptr) { + if (!controller->_engine) { controller->_engine = [[FlutterEngine alloc] initWithName:@"io.flutter" project:controller->_project allowHeadlessExecution:NO]; @@ -260,11 +260,11 @@ - (instancetype)initWithEngine:(nonnull FlutterEngine*)engine self = [super initWithNibName:nibName bundle:nibBundle]; if (self) { if (engine.viewController) { - NSLog(@"The supplied FlutterEngine %s is already used with FlutterViewController " - "instance %s. One instance of the FlutterEngine can only be attached to one " + NSLog(@"The supplied FlutterEngine %@ is already used with FlutterViewController " + "instance %@. One instance of the FlutterEngine can only be attached to one " "FlutterViewController at a time. Set FlutterEngine.viewController " "to nil before attaching it to another FlutterViewController.", - [[engine description] UTF8String], [[engine.viewController description] UTF8String]); + [engine description], [engine.viewController description]); } _engine = engine; CommonInit(self); diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm index 87c7a0cd356f8..b4636c0bed5f6 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm @@ -2,16 +2,17 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "FlutterBinaryMessenger.h" +#import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h" #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h" #import +#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h" #import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h" #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterDartProject_Internal.h" #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h" #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTestUtils.h" -#include "flutter/testing/testing.h" +#import "flutter/testing/testing.h" @interface FlutterViewControllerTestObjC : NSObject - (bool)testKeyEventsAreSentToFramework;