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

Fix: Keybindings on Numpad special keys (#7315) #7329

Merged
merged 1 commit into from
Mar 18, 2020
Merged

Fix: Keybindings on Numpad special keys (#7315) #7329

merged 1 commit into from
Mar 18, 2020

Conversation

geropl
Copy link
Contributor

@geropl geropl commented Mar 12, 2020

Fixes #7315.

This adds an exception to KeyCode.toKey regarding special keys on numpads

Signed-off-by: Gero Posmyk-Leinemann [email protected]

What it does

This adds an exception for Numpad keys during the KeyboardEvent -> key bindings resolution. It ensures that with Numlock disabled, special keys like HOME (on Numpad7), END (on Numpad1) etc. are detected and handled correctly.

How to test

  • Open a regular editor
  • ensure that Numlock is disabled
  • press Numpad7 (HOME), Numpad1 (END), Numpad9 (Page Up), Numpad3 (Page Down`) and observe the respective cursor movement

Review checklist

Reminder for reviewers

@geropl geropl changed the title Fixes eclipse-theia/theia#7315 Fix: Keybindings on Numpad special keys (#7315) Mar 12, 2020
@akosyakov akosyakov requested a review from spoenemann March 12, 2020 13:33
@akosyakov akosyakov added the keybindings issues related to keybindings label Mar 12, 2020
@geropl
Copy link
Contributor Author

geropl commented Mar 12, 2020

@spoenemann @akosyakov Can you have a look? Thanks!

@akosyakov
Copy link
Member

@geropl you have to sign ECA: https://github.com/eclipse-theia/theia/blob/master/CONTRIBUTING.md#eclipse-contributor-agreement

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot test it with my keyboard, but the change makes sense to me.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really use keyCode though? MDN shows it as deprecated.

I tested with https://w3c.github.io/uievents/tools/key-event-viewer.html:

image

event.key seems to map to the same key representation, when event.code maps to the layout-independent identifier (see 1-and-2 vs 1-and-4)

@geropl
Copy link
Contributor Author

geropl commented Mar 16, 2020

Should we really use keyCode though? MDN shows it as deprecated.

@marechal-p Oh, that's news to me, thx. Switched to .key 👆 , still works nicely.

@geropl geropl requested a review from spoenemann March 16, 2020 06:54
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well, thanks!

@akosyakov
Copy link
Member

@spoenemann are you fine with changes, please merge if so

@spoenemann
Copy link
Contributor

Switched to .key 👆 , still works nicely.

Note that key takes modifiers into account. @geropl could you check whether pressing the numpad keys with any combination of shift, ctrl, alt leads to the same key value?

https://w3c.github.io/uievents/tools/key-event-viewer.html

@geropl
Copy link
Contributor Author

geropl commented Mar 18, 2020

@spoenemann key stays the same, irregardless of the modifiers:
image
(with alt it works as well but is hard to show because alt + home sets the current tab to "Home")

@spoenemann
Copy link
Contributor

Does it work also when Numlock is enabled? In that case key is just a string with a number, and we don't have any mapping for that in Key.getKey.

IMO it would be safer to switch back to keyCode.

@geropl
Copy link
Contributor Author

geropl commented Mar 18, 2020

@spoenemann and I found out that Shift + Numlock + Numpad7 resolves to 7 (on Linux), so we stay with .key which does the same.

We narrowed the corner case to "complex numpad keys" by adding the check for && event.key.length > 1.

This adds an exception to `KeyCode.toKey` regarding special keys on numpads

Signed-off-by: Gero Posmyk-Leinemann <[email protected]>
@spoenemann spoenemann merged commit e876b16 into eclipse-theia:master Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ux] Keybindings: numpad 7/1 (HOME/END) not working as expected (German layout)
4 participants