-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable KeyDown events unconditionally on MacOS #15740
Enable KeyDown events unconditionally on MacOS #15740
Conversation
@@ -544,26 +544,15 @@ - (void)keyDown:(NSEvent *)event | |||
|
|||
//InputMethod is active | |||
if(_parent->InputMethod->IsActive()){ |
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.
We can simplify this condition as well
if(_parent->InputMethod->IsActive())
{
[[self inputContext] handleEvent:event];
}
else
{
https://github.com/AvaloniaUI/Avalonia/blob/3fb7a820a11866a54130cfa595c95867655ebc0f/native/Avalonia.Native/src/OSX/AvnView.mm#L564
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.
I'm trying to put this simplification into context.. are you saying to remove the handleKeyDown call? That call is necessary, and needs to be made before allowing the inputContext to handle the event, as if the KeyDown call handles it, the inputContext should never get it.
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.
Wrap the input context call with that condition. Raise a synthetic text input event when the condition isn't met. Always process key down.
You can test this PR using the following package version. |
Please read the following Contributor License Agreement (CLA). If you agree with the CLA, please reply with the following:
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by AvaloniaUI OÜ and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant AvaloniaUI OÜ, and those who receive the Submission directly b. Patent License. You grant AvaloniaUI OÜ, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to AvaloniaUI OÜ. You agree to notify AvaloniaUI OÜ in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the Republic of Estonia, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and AvaloniaUI OÜ dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
@cla-avalonia agree |
Integration tests failing appears to be a tooling problem, they all pass fine on my machine. |
Can you apply the requested changes? |
App should never attempt to intercept keys before IME has a chance of processing those, since they don't know and can't know how IME would process events. Keys are being pre-filtered by IME, which is consistent with other platforms. If one wants to have a way to preview KeyDown they need to disable IME for a particular control or the entire window, that will switch Avalonia to the "traditional" KeyDown (app can do whatever it wants) -> TextInput -> KeyUp model at the cost of not supporting CJK input. |
But here's the thing.. It's not consistent, even in Avalonia.. The behavior this PR supplies on MacOS is the behavior that already exists in Avalonia, on both Windows & Linux. If you run the sample app attached, you WILL get the KeyDown called before the IME processes the Input. So either this behavior is broken on the other two platforms, which I would say it is not, or it was broken on MacOS & should be restored to parity with the other two platforms. |
Please use an actual IME that interacts with the keyboard |
When IM is active the owner of the keyboard is IM and not the app. Apps should never react to keys that are handled by IME, because they don't have information about the way the IM handled the key (e. g. it could have been used for navigating completion list, as a hotkey, etc, etc, the app has no knowledge of this) and the app shouldn't attempt to guess if it's safe to handle the key before IME sees it, because app author can not know about all possible IM systems in the world. If app tries to mess with the external logic it has no understanding of, it leads to text input being broken. If you are seeing a KeyDown event, it either means that IM is not active (which is the case for Linux most of the time, we only auto-enable IM for CJK locales) or that IM decided to not filter the key. There is a fallback to map keys to chars that is auto enabled for views that opt-out of IME, but it only works with input systems where there is 1-to-1 mapping between a key and a char. If you want to be able to handle all KeyDown events yourself and be able to filter out TextInput events based on KeyDown events, you can disable IM via We could perhaps implement Key.ImeProcessed and KeyDown from WPF for completeness, but handling it on the app sidet will NOT prevent the key being processed by IM and apps should never use such API for triggering actions like hotkeys. |
What does the pull request do?
KeyDown events were removed on MacOS from being fired for controls with an IME.
On Windows & Linux, every KeyStroke generates a KeyDown Event, then a TextInputEvent, followed by a KeyUp event.
This also happens on MacOS, unless focus is on a control with an IME, where they KeyDown Events are being consumed, unless there is a modifier key present.
This removes the ability to use a Tunneling Event Handler to validate input, or check for a localized hotkey, and set the event as handled, so that the keystroke never makes it to the IME.
Currently, the following code for a TextBox (MyText) is never executed on MacOS, it functions correctly on Windows & Linux
This PR fixes this behavior.
What is the current behavior?
KeyDown events are consumed by the IME.
What is the updated/expected behavior with this PR?
KeyDown events are fired unconditionally, allowing the equivalent functionality of a PreviewKeyDown from WPF land, implemented as the Tunnel routing strategy.
How was the solution implemented (if it's not obvious)?
Remove conditional checks before calling handleKeyDown
Checklist
Breaking changes
None Known, but because of the location of this code it obviously needs to be carefully considered.
Obsoletions / Deprecations
N/A
Fixed issues
Fixes #15727