fix: remove some key codes from keyhandler #5667
Conversation
WofWca
left a comment
WofWca
left a comment
There was a problem hiding this comment.
Does anyone else implement such a solution?
I feel like this will still cause some problems, and I wonder if we should keep playing this whack-a-mole instead of utilizing an established solution / library, or providing another way to solve these, e.g. adding shortcut remapping.
Anyway, let's try it, IDK.
| // When modifying this, don't forget to also update the corresponding | ||
| // `aria-keyshortcuts` properties, and the "Keybindings" help window. | ||
| if (!ev.repeat) { | ||
| const isLatin = ev.key && /^\p{Script=Latin}$/u.test(ev.key) |
There was a problem hiding this comment.
How about some comments? At least link to an issue?
There was a problem hiding this comment.
git blame will show the commit and the comment in this PR. The issue is not about non-latin but Dvorak right-handed.
But I will add an explaining comment why we care for isLatin
There was a problem hiding this comment.
I don't think git history is a good place to explain the semantics of the code. One still properly names variables even if you can look at git blame to find the issue that a commit solves. And the same should go for comments.
There was a problem hiding this comment.
As said the issue is about Dvorak right-handed to about non-latin keys
It's based on the approach described here https://www.velopen.com/blog/keyboard-shortcuts-on-non-latin-alphabet-layouts/ if the key is non-latin go for the keyCode. We could use that library but I'm not sure if we need the contexts which the lib introduces. If there is a solid library that covers all cases and is used by many I agree to use it (as said before) Basically we rely on the event.key (since that is what the user should see when typing the key as text). Only if the key is non latin we go for the code. As discussed there is no proper solution for all scenarios, so I would prefer this simple approach. |
resolves #5595
This is just a workaround to solve the issue that copy & paste shortcuts (and others) might not be working on different keyboard layouts.
Since copy & paste usage has a much higher priority than maybe having no shortcut to open the settings. And any keyCode can collide with some generic shortcut depending on the keyboard layout on the current machine.
The current approach with only checking the event.key is not a final solution since it might not be possible to use non latin keyboards for shortcuts.
A more proper solution would be to use a library (maybe like https://github.com/mckravchyk/shocut which supports non-latin keyboards it seems) or implement a similar approach ourself
edit: added a simple isLatin check to provide a fallback for non latin keyboards