-
Notifications
You must be signed in to change notification settings - Fork 6k
Listen to keyUp event on meta modified keys #13984
Conversation
gspencergoog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stuartmorgan-g
left a comment
There was a problem hiding this 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?
|
@stuartmorgan added filtering for the current window, PTAL! |
|
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. |
|
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? |
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. |
|
Agreed. Shortcuts are broken right now, so I added the check for now with a TODO and revisit it soon. PTAL! |

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