-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: Handle Numeric AccessKey #16076
base: master
Are you sure you want to change the base?
feat: Handle Numeric AccessKey #16076
Conversation
You can test this PR using the following package version. |
You can test this PR using the following package version. |
I don't have a lot of experience with the AccessKey side of things, so I can't comment on why it's done the way it is but I personally feel it would probably be a better idea to register the Key rather than Char in the AccessKeyHandler dictionary and parse the Key once on registration rather than trying to deal with synonyms every time a key is pressed. While this probably handles the numeric AccessKey situation, I suspect it doesn't deal with the non-English AccessKey situation well. |
@IanRawley Changing from Avalonia/src/Avalonia.Controls/Primitives/AccessText.cs Lines 45 to 49 in 143399f
The i18n is probably an issue, too, as you pointed out. |
I chose KeyGestureFormatInfo because it is extensible.
In the future we could add in AppBuilder which allows you to configure KeyGestureFormatInfo to solve the problem you expose. |
Yeah, that's the bit I wasn't aware of. I was just looking at the AccessKeyHandler. If AccessText is pulling the char out of the dictionary, then keeping the dictionary as a char makes sense. If the AccessText is what's feeding the char to AccessKeyHandler to populate the dictionary, then the dictionary could become a Key without breaking anything, but is also less likely to solve all the other issues non-English access keys are likely to cause. I'm specifically thinking Japanese myself, as I have some experience with Japanese keyboards, but not enough with Japanese apps to know if developers are likely to use access keys with Hiragana or Katakana characters. |
You can test this PR using the following package version. |
You can test this PR using the following package version. |
You can test this PR using the following package version. |
You can test this PR using the following package version. |
You can test this PR using the following package version. |
should I make any other changes? |
@@ -224,7 +231,10 @@ protected virtual void OnPreviewKeyUp(object? sender, KeyEventArgs e) | |||
{ | |||
MainMenu.Open(); | |||
} | |||
|
|||
if (MainMenu is null) |
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 may want to also add || !MainMenu.IsVisible
here or combine with pattern matching. If it's not visible, then we currently get the same "sticky" behavior as the original issue where alt is released, but the access keys are still underlined.
You can test this PR using the following package version. |
What does the pull request do?
Handle Numeric AccessKey
What is the current behavior?
Access Key like
<Label Content="_1 Oprions"/>
does not workWhat is the updated/expected behavior with this PR?
Access Key like
<Label Content="_1 Oprions"/>
work with Numeric Key, does not with Numpad KeyHow was the solution implemented (if it's not obvious)?
Using
Platform.KeyGestureFormatInfo.FormatKey
insteand ofKey.ToString()
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Fixes #15957