From 87e27ff2029aecc7f7b009d298da90691abe02ba Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Thu, 28 Sep 2023 18:45:11 +0100 Subject: [PATCH] Revert "[macOS] performKeyEquivalent cleanup (#45946)" (flutter/engine#46374) This broke some keyboard shortcut handling (e.g. cmd-a to select all). This reverts commit 0deacc6c7023789c4a75155e6798b3cccdc6384a. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] 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. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] 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 --- .../framework/Source/FlutterViewController.mm | 51 ++++++++++++++++--- .../Source/FlutterViewControllerTest.mm | 7 --- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm index 7e7e0c9cc6..5c22fe6c01 100644 --- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm +++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm @@ -164,6 +164,8 @@ NSData* currentKeyboardLayoutData() { - (void)setBackgroundColor:(NSColor*)color; +- (BOOL)performKeyEquivalent:(NSEvent*)event; + @end /** @@ -240,6 +242,37 @@ NSData* currentKeyboardLayoutData() { @end +#pragma mark - NSEvent (KeyEquivalentMarker) protocol + +@interface NSEvent (KeyEquivalentMarker) + +// Internally marks that the event was received through performKeyEquivalent:. +// When text editing is active, keyboard events that have modifier keys pressed +// are received through performKeyEquivalent: instead of keyDown:. If such event +// is passed to TextInputContext but doesn't result in a text editing action it +// needs to be forwarded by FlutterKeyboardManager to the next responder. +- (void)markAsKeyEquivalent; + +// Returns YES if the event is marked as a key equivalent. +- (BOOL)isKeyEquivalent; + +@end + +@implementation NSEvent (KeyEquivalentMarker) + +// This field doesn't need a value because only its address is used as a unique identifier. +static char markerKey; + +- (void)markAsKeyEquivalent { + objc_setAssociatedObject(self, &markerKey, @true, OBJC_ASSOCIATION_RETAIN); +} + +- (BOOL)isKeyEquivalent { + return [objc_getAssociatedObject(self, &markerKey) boolValue] == YES; +} + +@end + #pragma mark - Private dependant functions namespace { @@ -279,15 +312,19 @@ void OnKeyboardLayoutChanged(CFNotificationCenterRef center, } - (BOOL)performKeyEquivalent:(NSEvent*)event { - // Do not intercept the event if flutterView is not first responder, otherwise this would - // interfere with TextInputPlugin, which also handles key equivalents. - // - // Also do not intercept the event if key equivalent is a product of an event being - // redispatched by the TextInputPlugin, in which case it needs to bubble up so that menus - // can handle key equivalents. - if (self.window.firstResponder != _flutterView || [_controller isDispatchingKeyEvent:event]) { + if ([_controller isDispatchingKeyEvent:event]) { + // When NSWindow is nextResponder, keyboard manager will send to it + // unhandled events (through [NSWindow keyDown:]). If event has both + // control and cmd modifiers set (i.e. cmd+control+space - emoji picker) + // NSWindow will then send this event as performKeyEquivalent: to first + // responder, which might be FlutterTextInputPlugin. If that's the case, the + // plugin must not handle the event, otherwise the emoji picker would not + // work (due to first responder returning YES from performKeyEquivalent:) + // and there would be an infinite loop, because FlutterViewController will + // send the event back to [keyboardManager handleEvent:]. return NO; } + [event markAsKeyEquivalent]; [_flutterView keyDown:event]; return YES; } diff --git a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm index 7c0a96a1f8..7894e9258b 100644 --- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm +++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm @@ -331,13 +331,6 @@ TEST(FlutterViewControllerTest, testViewControllerIsReleased) { const uint64_t kPhysicalKeyTab = 0x7002b; [viewController viewWillAppear]; // Initializes the event channel. - // Creates a NSWindow so that FlutterView view can be first responder. - NSWindow* window = [[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 800, 600) - styleMask:NSBorderlessWindowMask - backing:NSBackingStoreBuffered - defer:NO]; - window.contentView = viewController.view; - [window makeFirstResponder:viewController.flutterView]; [viewController.view performKeyEquivalent:event]; EXPECT_TRUE(called);