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

Unify key decoding for keydown #1676

Closed
mrmr1993 opened this issue May 21, 2015 · 4 comments
Closed

Unify key decoding for keydown #1676

mrmr1993 opened this issue May 21, 2015 · 4 comments

Comments

@mrmr1993
Copy link
Contributor

At the moment, we decode keys seperately for each keydown handler. With a view to reducing bugs and handling international keyboard layouts correctly, I think we should centralise conversion from event to the key we want to handle.

Pros:

  • duplicated/similar code is moved to a library function.
  • we will have to make anomalies in keydown handling explicit, which will help with debugging
  • when (or before) Chromium issue 227231 is resolved, we have a centralised place to implement on the new feature. From this point, we can stop relying on the depreciated keypress event, by using event.key when it's available:
    • this will solve all keyboard internationalisation issues immediately (passing keys we are going to handle, and blocking the events for those we aren't)
    • we can't be pre-empted by keydown handlers for keys we want to handle, where event.keyCode disagrees with what we get from a keypress event (eg. a page handles a, but we handle an internationalised key that represents as a during keydown)
    • we move to a single key handler, since our keypress handlers will never get events for any key we handle
  • friendlier key-handling code

Cons:

  • code movement/refactoring
  • bugs from missed edge cases due to platfrom inconsistency
  • no visible benefit for the user
    • the Chromium issue will change this but will also require extra work
  • redundancy in the code until old Chrome versions are abandoned (for event.key considerations only)
@lydell
Copy link

lydell commented May 26, 2015

Switching to event.key won't fix everything. You need event.code as well. See akhodakivskiy/VimFx#331 (be sure to read the last comment) and vim-like-key-notation.

@mrmr1993
Copy link
Contributor Author

(be sure to read the last comment)

@lydell I can't seem to find the comment you're referring to, could you quote it here?

To my understanding, event.key looks to provide the OS's interpretation of the key, which is currently what the codebase expects (often incorrectly) to be handling everywhere we deal with keys. Is there some nuance I've missed?

@lydell
Copy link

lydell commented May 26, 2015

Sorry I linked to the wrong PR. I've updated my comment above.

@mrmr1993
Copy link
Contributor Author

Switching to event.key won't fix everything.

My suggestion in the issue was about replacing keypress handlers with keydown handlers that use event.key. This gives us the right behaviour under all internationalisations, fixing eg. #1411. Any mapping that currently works will work identically under this change, and those that don't will start to.

You need event.code as well.

I understand the motivation (keyboard layout agnosticism), but that seems like a separate feature request. Since this is also much harder to get right, I think we should do it separately. For example:

  • we need to be careful using event.code for users who use both a non-en_US (eg. en_GB, AZERTY, Dvorak) and a non-latin keyboard layout:
    • event.code will give a consistent code across all layouts, but doesn't always represent the printing character (eg. giving q for a in AZERTY),
    • event.keyCode will (mostly) give the key code corresponding to the label when using a Latin layout (code here), but otherwise falls back to the hardware code of the key, so eg. pressing a on an AZERTY keyboard in a Russian layout will give the keyCode for q. This is less consistent across layouts, but seems better to me for this.
    • For reference, Firefox falls back to giving event.keyCode as the keyCode for the key in the highest priority ASCII-capable keyboard layout for the system. This would be much better for our purposes (although means that the same key in the same keyboard layout can have differing keyCodes under different system configurations).
  • For users with only latin, non-en_US keyboards (eg. en_UK, es like me), there's no way to distinguish en_GB as the layout, let alone it being the main one, and so the #~ key will be wrong either often or always.

For these (and other reasons), I would rather see that treated as a separate issue.

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

No branches or pull requests

2 participants