Skip to content
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

Support non-English keyboards and characters by using KeyboardEvent.key #40

Closed
Bone008 opened this issue Oct 14, 2019 · 15 comments
Closed
Assignees
Labels

Comments

@Bone008
Copy link

Bone008 commented Oct 14, 2019

Describe the bug
Trying to register shortcuts for special characters (for example #) does not work with a non-US keyboard layout.
Registering letters not in the English alphabet (for example the German "ä") is completely impossible.

Cause
Currently this library seems to use the deprecated KeyboardEvent.which property with some manual mappings for special characters.

Suggested fix
I suggest switching to KeyboardEvent.key, which correctly handles foreign keyboard layouts and automatically assigns standardized names to non-printable characters such as arrow keys, home, end, etc.

It also handles characters only reachable by pressing Shift (e.g. to obtain '?' from Shift+/ on a US keyboard) automatically: When the combination is pressed, the key property will contain "?", so no manual mapping will be necessary and it will work across all layouts.

Browser support for that property is quite consistent nowadays, so compared to the current behavior it would be a big improvement. You can still use String.fromCharCode(e.which) if 'key' is unavailable as a fallback.

@Bone008
Copy link
Author

Bone008 commented Oct 14, 2019

Also, I haven't found any way to register the + key as a shortcut, since it has a special meaning for combinations and I cannot find any special name for it that the library understands :(

@omridevk
Copy link
Owner

@Bone008
Thanks a lot of reporting the issue.
Yes right now there's a known issue for registering the plus key, I will address it in the next release, I had some personal matters that had to be taken care, thus I was not able to provide a version.
But I assure you I will try to address this as soon as possible.
And please feel free to submit a PR, it will be much appreciated.

@omridevk omridevk added the bug label Oct 15, 2019
@omridevk omridevk self-assigned this Oct 15, 2019
@omridevk
Copy link
Owner

@Bone008
Thanks for the suggestion for using event.key, it will solve some of the issues (mapping of special characters), but I am still not sure how to resolve the issue of using non-english characters (I am from Israel so I tried to test it with Hebrew characters). The browser keydown event key property will contain the english letter when any of the meta keys are pressed while clicking, so if you define a key shortcut "control + ä" the event object will report the english character.

I need to dig further into it to fix this specific issue, I will try to address the following asap:

  1. Add plus sign alias so one can register it as a key.
  2. Add support for special characters in different keyboard layout
  3. Add support for non-english characters in sequences (this is possible since sequences are single keydown events with no meta characters clicked).

As for adding support for non-english characters in key combination this might take some more time.

@Bone008
Copy link
Author

Bone008 commented Nov 5, 2019

@omridevk I apologize for the super late reply, somehow I received no notification of your second response. Thank you so much for considering my feedback!

I am not sure what issue you are seeing with "control + ä" or other events with meta keys being pressed. According to my tests (I found keycode.info to be a good resource), when I press "control + ä", the keydown event correctly contains e.controlKey = true and e.key = "ä" - tested with Firefox and Chrome.

Maybe the behavior is different on a Hebrew keyboard, since English characters are replaced by the Hebrew alphabet as far as I can tell. I suppose common app shortcuts like "ctrl + c" wouldn't work with a Hebrew keyboard otherwise? That seems like an acceptable tradeoff to me.
On a German layout there are just 4 more letters (ä, ö, ü and ß) and all the punctuation keys have been moved around, but all English letters are still present.

Proper internationalization is hard :(

For my project, only having a solution to (1.) and (2.) from your comment above would already be a huge improvement. Thanks!

@omridevk
Copy link
Owner

omridevk commented Nov 5, 2019

@Bone008
Thanks for your reply, I will give it another go, I will try to address 1, 2 as soon as possible.
Will try to check with german layout.
And sorry for the delay.

omridevk pushed a commit that referenced this issue Dec 20, 2019
…signment using 'plus' keyword; issue 40 - use event.key if possible; issue 37 - do not use rxjs internal use operator map (#43)

#42
#40
#37
@omridevk
Copy link
Owner

Closing this for now as most of it was fixed in:
https://github.com/omridevk/ng-keyboard-shortcuts/releases/tag/8.2.0
@Bone008
If you can play around with this version and see if any issue still remains.
notice you can now use "plus" escape to bind to the + character.

@Bone008
Copy link
Author

Bone008 commented Dec 20, 2019

Thank you so much for looking into this! I updated and it works great. No matter which keyboard layout I pick, special characters get picked up exactly as expected (including the ones only reachable through a combination with shift, such as Shift + ß --> ? in German). Even keys that require "dead keys" to enter them work, such as the French é -- I don't think anyone in their right mind should use that as a shortcut, but hey.

There is a small exception: Some keys still don't get through, for example the German ä. I tracked it down to all keys which are in the _KEYCODE_MAP and different from the US layout. The ä key has keycode 222, which is ' on US keyboards, so that's what the library maps it to due to characterFromEvent(...) using event.which before accessing event.key.

I believe _KEYCODE_MAP should no longer be necessary and now does more harm than good, since event.key properly handles all special character transformations in a layout-aware fashion. There may be some edge cases I have missed, though.


On another note, I think people will be confused about the API change that "shift + letter" no longer works, which you mentioned in #42. Especially commands like "ctrl + shift + f" (which is still written like that in the readme btw) now need to be rewritten to "ctrl + F". Not sure if this was intentional.

This becomes more problematic for numbers, since registering e.g. the command shift + 8 no longer works. But pressing Shift + 8 on a US keyboard yields *, but on a German keyboard yields ( and on a French keyboard yields _. On top of that, Shift + Numpad8 yields PageUp. So now the developer cannot register number shortcuts with Shift in a compatible way at all.

I18N is hard :(


Note that I am not using any of the mentioned problematic cases in my project at the moment, so personally I don't need these things to work, I just discovered them while playing around and wanted to make sure they are mentioned somewhere.

Thanks for your time!

@omridevk
Copy link
Owner

omridevk commented Dec 20, 2019

On another note, I think people will be confused about the API change that "shift + letter" no longer works, which you mentioned in #42. Especially commands like "ctrl + shift + f" (which is still written like that in the readme btw) now need to be rewritten to "ctrl + F". Not sure if this was intentional.

This is not the change, perhaps the docs are not clear, all it means that if you want to use the character + you would have to write "plus" and not "+", since that never worked before.
all previous shortcuts should work the same and should not be affected by this change, otherwise it would have been a breaking change. which is not. at least not from the tests I have performed.

I see your point, let me check it real quick.

And happy to hear that it solves a lot of the issues, can you please open issues for the remaining bugs? so I can keep track of them?
Thanks for the effort and checking this.

@omridevk
Copy link
Owner

I have checked and all keyboards short cuts like "ctrl + shift + f" still works as expected.

@omridevk
Copy link
Owner

Guess i'll have to adress that issue, thanks a lot for bringing that up.

@Bone008
Copy link
Author

Bone008 commented Dec 20, 2019

Using version 8.2.0, registering the command "shift + n" definitely no longer works for me, neither does "shift + N", but uppercase "N" works, tried it with US, German and even Hebrew keyboard layout.

I thought your comment in issue 42 implied that the change was intentional.

@omridevk
Copy link
Owner

yes. I am fixing it now
will release version 8.2.3, please let me know if works.
intentional but without care for the side effects, happens when working long hours :P

@omridevk
Copy link
Owner

okay version 8.2.3 is up and running, can you please check, or better yet, create a sandbox for us to play with?
you can fork my original sandbox:
https://codesandbox.io/s/ngkeyboardshortcuts-example-oft3z

@omridevk
Copy link
Owner

And thank you for the help, it is much appreciated!!!

@omridevk
Copy link
Owner

i will update my comment in the original issue with the new solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants