From b25eacd2ffe15836c434336552dbbf893f5adb28 Mon Sep 17 00:00:00 2001 From: Bruno Leroux Date: Tue, 28 Mar 2023 08:20:17 +0200 Subject: [PATCH 1/2] Fix Ctrl+Tab is broken on MacOS --- .../framework/Source/FlutterViewController.mm | 60 ++++++++++++++++++- .../Source/FlutterViewControllerTest.mm | 44 ++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm index 48e725542c40d..f8db00437ac1b 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm @@ -6,6 +6,7 @@ #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h" #include +#import #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h" #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterCodecs.h" @@ -163,6 +164,8 @@ @interface FlutterViewWrapper : NSView - (void)setBackgroundColor:(NSColor*)color; +- (BOOL)performKeyEquivalent:(NSEvent*)event; + @end /** @@ -239,6 +242,37 @@ - (void)onKeyboardLayoutChanged; @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 { @@ -258,12 +292,15 @@ void OnKeyboardLayoutChanged(CFNotificationCenterRef center, @implementation FlutterViewWrapper { FlutterView* _flutterView; + FlutterViewController* _controller; } -- (instancetype)initWithFlutterView:(FlutterView*)view { +- (instancetype)initWithFlutterView:(FlutterView*)view + controller:(FlutterViewController*)controller { self = [super initWithFrame:NSZeroRect]; if (self) { _flutterView = view; + _controller = controller; view.autoresizingMask = NSViewWidthSizable | NSViewHeightSizable; [self addSubview:view]; } @@ -274,6 +311,24 @@ - (void)setBackgroundColor:(NSColor*)color { [_flutterView setBackgroundColor:color]; } +- (BOOL)performKeyEquivalent:(NSEvent*)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 endless loop, because FlutterViewController will + // send the event back to [keyboardManager handleEvent:]. + return NO; + } + [event markAsKeyEquivalent]; + [_flutterView keyDown:event]; + return YES; +} + - (NSArray*)accessibilityChildren { return @[ _flutterView ]; } @@ -405,7 +460,8 @@ - (void)loadView { if (_backgroundColor != nil) { [flutterView setBackgroundColor:_backgroundColor]; } - FlutterViewWrapper* wrapperView = [[FlutterViewWrapper alloc] initWithFlutterView:flutterView]; + FlutterViewWrapper* wrapperView = [[FlutterViewWrapper alloc] initWithFlutterView:flutterView + controller:self]; self.view = wrapperView; _flutterView = flutterView; } diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm index b48953923a38f..c2ec3e8f09178 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm @@ -59,6 +59,7 @@ @interface FlutterViewControllerTestObjC : NSObject - (bool)testKeyEventsAreSentToFramework; - (bool)testKeyEventsArePropagatedIfNotHandled; - (bool)testKeyEventsAreNotPropagatedIfHandled; +- (bool)testCtrlTabKeyEventIsPropagated; - (bool)testFlagsChangedEventsArePropagatedIfNotHandled; - (bool)testKeyboardIsRestartedOnEngineRestart; - (bool)testTrackpadGesturesAreSentToFramework; @@ -207,6 +208,10 @@ id MockGestureEvent(NSEventType type, NSEventPhase phase, double magnification, ASSERT_TRUE([[FlutterViewControllerTestObjC alloc] testKeyEventsAreNotPropagatedIfHandled]); } +TEST(FlutterViewControllerTest, TestCtrlTabKeyEventIsPropagated) { + ASSERT_TRUE([[FlutterViewControllerTestObjC alloc] testCtrlTabKeyEventIsPropagated]); +} + TEST(FlutterViewControllerTest, TestFlagsChangedEventsArePropagatedIfNotHandled) { ASSERT_TRUE( [[FlutterViewControllerTestObjC alloc] testFlagsChangedEventsArePropagatedIfNotHandled]); @@ -280,6 +285,45 @@ - (bool)testKeyEventsAreSentToFramework { return true; } +// Regression test for https://github.com/flutter/flutter/issues/122084. +- (bool)testCtrlTabKeyEventIsPropagated { + id engineMock = flutter::testing::CreateMockFlutterEngine(@""); + __block bool called = false; + __block FlutterKeyEvent last_event; + OCMStub([[engineMock ignoringNonObjectArgs] sendKeyEvent:FlutterKeyEvent {} + callback:nil + userData:nil]) + .andDo((^(NSInvocation* invocation) { + FlutterKeyEvent* event; + [invocation getArgument:&event atIndex:2]; + called = true; + last_event = *event; + })); + FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engineMock + nibName:@"" + bundle:nil]; + // Ctrl+tab + NSEvent* event = [NSEvent keyEventWithType:NSEventTypeKeyDown + location:NSZeroPoint + modifierFlags:0x40101 + timestamp:0 + windowNumber:0 + context:nil + characters:@"" + charactersIgnoringModifiers:@"" + isARepeat:NO + keyCode:48]; + const uint64_t kPhysicalKeyTab = 0x7002b; + + [viewController viewWillAppear]; // Initializes the event channel. + [viewController.view performKeyEquivalent:event]; + + EXPECT_TRUE(called); + EXPECT_EQ(last_event.type, kFlutterKeyEventTypeDown); + EXPECT_EQ(last_event.physical, kPhysicalKeyTab); + return true; +} + - (bool)testKeyEventsArePropagatedIfNotHandled { id engineMock = flutter::testing::CreateMockFlutterEngine(@""); id binaryMessengerMock = OCMProtocolMock(@protocol(FlutterBinaryMessenger)); From e0b6fa82e0db6f1b3a24d824b23429ce0340efb3 Mon Sep 17 00:00:00 2001 From: Bruno Leroux Date: Tue, 18 Apr 2023 08:19:12 +0200 Subject: [PATCH 2/2] Address review comment --- .../darwin/macos/framework/Source/FlutterViewController.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm index f8db00437ac1b..acd07a827c569 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm @@ -320,7 +320,7 @@ - (BOOL)performKeyEquivalent:(NSEvent*)event { // 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 endless loop, because FlutterViewController will + // and there would be an infinite loop, because FlutterViewController will // send the event back to [keyboardManager handleEvent:]. return NO; }