Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@franciscojma86
Copy link
Contributor

@franciscojma86 franciscojma86 commented Nov 22, 2019

Fixes: flutter/flutter#44135

Macos does not report a keyUp: event when a meta-modified key is released while command is still pressed. This change listens to keyUp without modifying the NSEvent forwarded in the application.

@auto-assign auto-assign bot requested a review from gw280 November 22, 2019 23:03
@franciscojma86 franciscojma86 removed the request for review from gw280 November 22, 2019 23:04
@franciscojma86 franciscojma86 changed the title Meta Listen to keyUp event on meta modified keys Nov 22, 2019
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Yay!

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look like it's doing any view filtering. Won't this pick up events that are destined for other views?

@franciscojma86
Copy link
Contributor Author

@stuartmorgan added filtering for the current window, PTAL!

@stuartmorgan-g
Copy link
Contributor

There could be other native views in the window though. If you, for example, put a native view and a Flutter view side by side in the same window, focus the native view, then do a sequence that triggers this, it seems like the Flutter view will receive a random key-up.

I think key handling for macOS needs a design doc that lays out all the complexities and where we can discuss tradeoffs/side-effects.

@franciscojma86
Copy link
Contributor Author

Sounds good. I can get started on that hopefully by next week. I also need to learn more about how add2app works on desktop.

Would you still be ok with landing this to solve the immediate problem of not receiving the keyUp event?

@stuartmorgan-g
Copy link
Contributor

Would you still be ok with landing this to solve the immediate problem of not receiving the keyUp event?

My concern is that given the PRs and bugs that have come through so far it seems pretty clear that there are a bunch of edge cases in key handling, and landing hacks that fix one edge case at the known expense of creating another turns it into a game of whack-a-mole. If we're not very careful, we'll end up accumulating layers of hacks, with hacks to compensate for the first hacks, creating the need for even more hacks, etc. I have seen systems that have gone down that road (with the best of intentions) and they are nearly unmaintainable. Especially since once a change like this has landed it's very hard to remove later if the side effects turn out to be too problematic, because at that point people may depend on the behavior it fixed, so removing it (to fix the side effects) becomes a regression.

If you want to land a short-term version of this, I would like it to be as narrowly targeted as possible, erring on the side of not always working rather than being over-broad and creating new potential issues. In this case, I think you could accomplish that by adding a check to the current logic that the first responder of the window is the Flutter view itself. (I'm not 100% sure that won't have side effects, but at least it addresses the ones I can think of.) That'll need revisiting when we have platform views, but so will a lot of the keyboard code.

@franciscojma86
Copy link
Contributor Author

Agreed. Shortcuts are broken right now, so I added the check for now with a TODO and revisit it soon. PTAL!

@franciscojma86 franciscojma86 merged commit a68805b into flutter:master Dec 3, 2019
@franciscojma86 franciscojma86 deleted the meta branch December 3, 2019 00:17
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modifier key combinations don't send key up events to framework.

4 participants