Skip to content

Commit

Permalink
Revert "[macOS] performKeyEquivalent cleanup (flutter#45946)" (flutte…
Browse files Browse the repository at this point in the history
…r#46374)

This broke some keyboard shortcut handling (e.g. cmd-a to select all).

This reverts commit 0087948.


## 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].

<!-- Links -->
[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
  • Loading branch information
cbracken authored Sep 28, 2023
1 parent 0087948 commit dbb6093
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ @interface FlutterViewWrapper : NSView

- (void)setBackgroundColor:(NSColor*)color;

- (BOOL)performKeyEquivalent:(NSEvent*)event;

@end

/**
Expand Down Expand Up @@ -240,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 {
Expand Down Expand Up @@ -279,15 +312,19 @@ - (void)setBackgroundColor:(NSColor*)color {
}

- (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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,13 +331,6 @@ - (bool)testCtrlTabKeyEventIsPropagated {
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);
Expand Down

0 comments on commit dbb6093

Please sign in to comment.