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

Keymap based on .key or .code rather than .keyCode? #74

Open
jasongrout opened this issue May 19, 2020 · 17 comments · May be fixed by #291
Open

Keymap based on .key or .code rather than .keyCode? #74

jasongrout opened this issue May 19, 2020 · 17 comments · May be fixed by #291

Comments

@jasongrout
Copy link
Contributor

jasongrout commented May 19, 2020

Early in Phosphor/Lumino's development, we decided to use .keyCode instead of .key for the basis of the keyboard shortcut system. This came about in part because (among other things) we realized that browsers were inconsistent with the .key attribute when modifiers were used. For example (and I just checked this is still the case with Firefox 76), in Firefox we saw that on a mac, pressing the numbers on a en-us keyboard layout while holding shift, control, alt, and command, led to the following sequence of event.key attributes:

1@345^7890

On Chrome, we get:

1€345fl7890

I suppose once browsers can be consistent about .key, we can look again at a keyboard shortcut system that depends on it. A nice thing about using .key is that presumably it would transparently support different keyboard maps, instead of relying on manually creating and selecting keyboard maps.

Another option, as @vidartf points out below, is moving to the .code attribute, especially since .keyCode is both deprecated and a bit convoluted across different browsers.

@jasongrout
Copy link
Contributor Author

jasongrout commented May 19, 2020

Note on my mac that I was testing, Shift-Alt-2 prints in a text box, and Shift-Alt-6 prints , but Shift-Alt-3 prints , not 3, for example.

@vidartf
Copy link
Member

vidartf commented May 19, 2020

Use case: Someone wants the default shortcut key for an action to be Shift + {. On many en-US keyboards, this is nonsensical since { is only accessible by pressing Shift + the key that produces [ when unmodified, so the shortcut will always trigger when someone types {. On other keyboard layouts such a shortcut might make perfect sense.

@vidartf
Copy link
Member

vidartf commented May 19, 2020

Use case: As a user of an application, I don't like the default shortcut for an action. I decide to remap it to Alt + 0 while using an en-US keyboard (doesn't resolve to a printable character). Later on I connect my Bluetooth keyboard that has another layout. This layout has a printable character that is only accessible by pressing Alt Gr + 0 (e.g. on a nb-NO keyboard it produces }). Depending on the system chosen, either of these options will happen:

  • every time I type { on my nb-NO keyboard the action triggers
  • that action is not triggerable by a shortcut on a nb-NO keyboard

@vidartf
Copy link
Member

vidartf commented May 19, 2020

(updated by comments above to have the use cases listed alone without including my opinions)

@jasongrout
Copy link
Contributor Author

I should say that one very nice thing about our current system is that, given a keyboard mapping, it is unambiguous about what keys to press. The codes are tied to physical keys, and the name of that key is the primary character of that key, i.e., the thing silkscreened on the key as the primary character. For example, on my US-en keyboard, even if I change the OS keyboard mapping, a given key still produces the same .keyCode even if it produces a different .key.

Just curious, for your second usecase, aren't Alt and Alt Gr two different keys?

@jasongrout
Copy link
Contributor Author

I should say that one very nice thing about our current system

Also, since keyboard mappings are an explicit part of the Lumino shortcut system, it is easy for us to detect the user changing it and swap out completely different shortcuts for different layouts.

@jasongrout
Copy link
Contributor Author

For the record, I'm happy to close this issue and decide that using .keyCode is a superior system (especially once we get good keymaps for common international keyboards). However, I wanted it at least documented why we chose to go the way we did since it seems to come up every now and then.

@jasongrout jasongrout changed the title Keymap based on .key rather than .keyCode Keymap based on .key rather than .keyCode? May 19, 2020
@vidartf
Copy link
Member

vidartf commented May 20, 2020

For the record, I'm happy to close this issue and decide that using .keyCode is a superior system

Also for the record: I'm neither for nor against this change, I just saw it proposed without outlining what problem it would solve and tried to ask about it (in jupyterlab/jupyterlab#7579) and this was supposed to be the details for that, but I was still left confused.

As far as I can tell at this point, the main points of interest of the keymap design is (likely to have some mistakes, so please correct as needed):

  1. Assumes you know whether your shortcut represents a pure printable character or not. If you expect it to not be a printable character, but it ends up being one, things might get weird (see use case examples above).
  2. Is able to display the shortcut as the unmodified character (silkscreened as you said) + modifiers.
  3. Is able to serialize the shortcut as the unmodified character + modifiers (?? or does it serialize as keycode? Does the serialization store the keymap as well? Or is this outside the scope of lumino?).
  4. If the user switches keyboard layout:
    1. If a keyboard mapping exists for that keyboard, it will make a best effort attempt to translate the shortcuts based on the unmodified character + modifiers (??? or does it continue to use the keycode, just displays it differently to the user?). If that doesn't make sense on the new map, you might get weird behavior (for both cases).
    2. If a keyboard mapping does not exists for that keyboard, it will use the closest/default keymap and all shortcuts will behave and display as if that keyboard was in use. In these cases, weird behavior can occur if the shortcut conflicts with a regular printable character or be rendered unworkable if it conflicts with OS/browser shortcuts. In either case, the display of the shortcut might end up confusing the the user, as what it displays as the "unmodified" character can be incorrect.

@vidartf
Copy link
Member

vidartf commented May 20, 2020

From reading the W3C spec, it looks as if using key for shortcuts it probably not what we want either (e.g. given dead keys and other keyboard state). It seems as if KeyboardEvent.code might be the recommended way, and support seems to be coming along nicely. Maybe that is what we should be considering instead (i.e. have our keymaps map KeyboardEvent.code to keys)?

@jasongrout
Copy link
Contributor Author

this was supposed to be the details for that

The details I was referring to there was the evidence (in the OP above) that browsers are inconsistent with how they handle .key.

@blink1073
Copy link
Member

What a nice world it would be if everyone followed the spec 😄

@jasongrout
Copy link
Contributor Author

It seems as if KeyboardEvent.code might be the recommended way, and support seems to be coming along nicely. Maybe that is what we should be considering instead (i.e. have our keymaps map KeyboardEvent.code to keys)?

+1 to exploring this idea, basing the keyboard shortcut system on .code rather than .keyCode.

@jasongrout jasongrout changed the title Keymap based on .key rather than .keyCode? Keymap based on .key or .code rather than .keyCode? May 20, 2020
@vidartf
Copy link
Member

vidartf commented May 20, 2020

For summary/context, here are some concerns that would be relevant when having a shortcut system in an app:

  1. We want to be able to display shortcuts to users as [Modifiers +] Key legend e.g. as Ctrl + ].
  2. We want to ensure that the key the user will have to press is accurately represented by the above display:
    1. There actually exists a key on the keyboard with that key legend (] in the example above).
    2. Pressing that key and the listed modifier will trigger the shortcut.
  3. We want to ensure that the specific shortcuts we choose do not conflict with regular user input (see use cases I listed in previous comments)
    1. If we end up specifying a shortcut that conflicts with regular user input, we want the shortcut to be disabled.
  4. We want to be able to use extended keys as input (e.g. some inputs have an "Undo" key, or e.g. the "Play media" key).
  5. We want the user to be able to switch the keyboard layout on the fly.

Anything else? Can we expand on this and use it to form a recommendation for how to modify the system currently in lumino, and write a recommendation for how to build shortcut systems on top of that (i.e. documentation). My initial pass on that would be:

Changes to lumino:

  • Lumino should ideally have more keyboard maps by default.
  • We should change the lookup keys for the keymaps to the values for KeyboardEvent.code, and look into the possibility of defining some standard sets for keys that are likely to be shared across most layouts (e.g. e.code: F12 => e.key: F12 and e.code: MediaPlay => e.key: MediaPlay).
  • We should have a function in lumino to make a best efforts answer to the questions: is this keyboard event something that could be classified as "regular input" on the current keymap (e.g. event.isComposing===true || event.key === 'Dead' || event.key !== keymap.keyForCode(event.code), that last one would need to be checked for corner-cases). This would then also extend the current check in the command registry (currently using CommandRegistry.keystrokeForKeydownEvent(event)).

Recommendations for applications:

  • When displaying a shortcut to the user, look up the key legend for the code in the keymap (keymap.keyForCode), and mark any invalid ones as invalid (e.g. warning icon or red-tinted background).
  • The application should allow you to manually select the map in case the auto-detection fails (very limited information available in the browser), or if the user has a non-standard layout and wants to pick the one that is closest.
  • Ideally, the application would have some way for the user to add keymaps. Ideally these should be transferable between users/systems for easy sharing (this should also make it easy to contribute it back to lumino).
  • Developers and users should be able to specify a map of keymap-name -> shortcut for their actions.
  • When a user changes the keymap (manually, or if the dynamic value changes as a result of e.g. an onlanguagechange event), all keybindings should be disposed and re-registered. An alternative is that the user must refresh their browser tab after such a change, but this is not ideal e.g. for users that might need to input text in multiple languages in the same session.
  • If the application present a UI for editing shortcuts, it should allow the user to specify which keymap's shortcuts it wants to edit.

I think the current system is pretty close to this, so a change of the fundamental logic will hopefully not be needed.

@jasongrout
Copy link
Contributor Author

I think the current system is pretty close to this, so a change of the fundamental logic will hopefully not be needed.

I think so!

@vidartf
Copy link
Member

vidartf commented Feb 2, 2022

I'm having a look at implementing the lumino side of this.

@vidartf vidartf linked a pull request Feb 3, 2022 that will close this issue
4 tasks
@vidartf
Copy link
Member

vidartf commented Feb 4, 2022

I have a draft PR up (#291) with the code I think we will need, but I have some questions about how to best organize it into packages. For now I have:

  • Created a new package "keyboard-capture" that includes a basic widget for capturing a new keyboard layout from a user. I made this a new package to avoid adding a run-time dependency on "widgets" in the "keyboard" package.
  • I made an app in the examples folder that sets up a very basic UI for capturing and exporting new keyboard layouts (using the widget from "keyboard-capture").

Open questions I have:

  • Are applications likely to want to display their own UI for capturing keyboard layouts? If yes, then does the current API make sense? If no, then there is probably not a need to release the widget in a package, rather it can all be kept in the example folder.
  • What is the best way to distribute other keyboard layouts? Should they be added to the "keyboard" package, or should that be reserved for the abstract + default en_US implementation only? It is unclear to me how large the built package would be if it included a "representative" amount of keyboard layouts (I assume it would compress well?).

@vidartf
Copy link
Member

vidartf commented Feb 4, 2022

Other points of discussion that I mentioned previously that are not covered by that PR:

  1. We want to be able to use extended keys as input (e.g. some inputs have an "Undo" key, or e.g. the "Play media" key).

This conflicts partially with our wish "There actually exists a key on the keyboard with that key legend". E.g. the current en_US layout has numpad keys, but some keyboards might not include that. I think this probably sets the precedent for the layout to include keys that might not actually be present on the keyboard, i.e. changing the wish to "If the key actually exists on the keyboard, it has the correct key legend".

We might want to take the approach "All special key values are valid unmodified keys". I.e. if a user clicks a "MediaPlay" key without any modifiers then both isValidKey and keyForKeydownEvent will process it.

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

Successfully merging a pull request may close this issue.

3 participants