reactist icon indicating copy to clipboard operation
reactist copied to clipboard

Some thoughts regarding KeyboardShortcut component

Open proxi opened this issue 4 years ago • 8 comments

🐛 Bug report

Current behavior

KeyboardShortcut component seems to make multiple assumptions that seem specific to usage in our codebases, to web, to English locale, and more. In particular, it remaps CMD to CTRL on platforms other than macOS.

The problem is that this is not enough to meet platform conventions. On macOS, SHIFT should be first, and only replacing characters without reordering them does not result in display following platform guidelines. CMD + SHIFT + 1 renders as Ctrl Shift 1 on Windows and ⌘ ⇧ 1 on macOS, but it should be ⇧ ⌘ 1 on macOS.

Other potential issues:

  • are we 100% sure that keys are always named "Ctrl" and "Shift" in other locales? I know that some e.g. German locales used "Strg" instead of "Ctrl"

Possible solutions

The component could only do rendering, and leave all other decisions to the caller.

proxi avatar Nov 02 '20 18:11 proxi

about this

are we 100% sure that keys are always named "Ctrl" and "Shift" in other locales? I know that some e.g. German locales used "Strg" instead of "Ctrl"

The component currently offers a means for callers to provide translations (see translateKey and setTranslateKey in its source code).

As for the order, and leaving shift first, I did not know that. And I don't think we acknowledge that convention in either Twist or Todoist, even before the introduction of this component. So it was not taken into consideration when designing it. The order was something left to the caller to provide, with the assumption that the same order could apply to all platforms.

About your proposal:

The component could only do rendering, and leave all other decisions to the caller.

How would this help? I don't want apps to have to repeat the logic of Shift having to go first when on MacOS. That would make the calls to this component too verbose.

it remaps CMD to CTRL on platforms other than macOS.

This was inherited from what Twist has always done before this component was introduced. It reflects the fact that we often use Cmd in macOS as we use Ctrl in other platforms. What would you do with a keyboard shortcut specified with Cmd in it, if this key does not exist in other platforms?

All in all, yes, I agree that it could use a revamp better knowing its design goals.

gnapse avatar Nov 02 '20 18:11 gnapse

Thank you. I think translation approach makes sense, this should be enough (whether we need to translate something needs more research).

I don't want apps to have to repeat the logic of Shift having to go first when on MacOS.

Do you think component should handle this? E.g. we could have ordering callback, similarly to translate callback?

What would you do with a keyboard shortcut specified with Cmd in it, if this key does not exist in other platforms?

I think this decision was guided by in-the-browser-Web platform. CMD key is 91 code, which is WIN key on Windows (I am unsure about practical usability of WIN key).

proxi avatar Nov 02 '20 18:11 proxi

Do you think component should handle this? E.g. we could have ordering callback, similarly to translate callback?

Translation is different because each app has its own translation infrastructure so the lib cannot assume anything about it. But I'd say that with ordering it's not the same. If we know it is a hard rule that on macOS the shift key has to go first, but in Windows and Linux the opposite is true, then if the lib is able to absorve this complexity, it should. And I think it is perfectly able. It does not need to know something from the app, such as how it translates stuff.

I think this decision was guided by in-the-browser-Web platform. CMD key is 91 code, which is WIN key on Windows (I am unsure about practical usability of WIN key).

This component does not consider key codes. It handles conceptual key names only, from how the user perceives these keys to represent. So this key thing is irrelevant. The practical thing that this conversion is achieving is that a big chunk of the time (if not always) when a custom keyboard shortcut in our apps is Cmd + something it means that in a non-macOS environment we will recognize Ctrl + something for the same purpose.

Can you illustrate with an example a situation where having the component follow this logic would result in a problem?

gnapse avatar Nov 02 '20 19:11 gnapse

Can you illustrate with an example a situation where having the component follow this logic would result in a problem?

  1. Well, for one thing "Ctrl + Cmd + X" is a perfectly valid shortcut on macOS.
  2. On Windows, "Win + Ctrl + X" is a valid shortcut, too.

I don't see how Cmd has anything to do with Ctrl, I think the only reason for this mapping is that it was lifted from Twist codebase. The fact that Ctrl+F and Cmd+F are distinct on macOS but same (?) on Windows is just confusing IMO. I need to talk to someone on Windows to better understand it.

This component does not consider key codes. It handles conceptual key names only

Not sure if I understand what you mean here. I do realize that the whole 'keyCode' vs 'key' in JavaScript issue is very problematic. But I would say that this component only deals with display, so it's not relevant to it (e.g. on my keyboard RightOption+A is ą, but I would still expect it to be shown as RightOption A). hotkeys-js library seems to work the same regardless of keyboard map (at least in the macOS layouts I tried).

proxi avatar Nov 02 '20 19:11 proxi

Valid points. Indeed, this was not made with windows shortcuts in mind, and was made with some assumptions on our approach to shortcuts, that now with hybrid native-like apps in the horizon may need revisiting.

This component does have a mechanism to express the default platform-specific modifier, and it is via the "mod" key name. It interprets it as Cmd in macOS. We can enhance it so that this special key name is also interpreted as the Win in Windows. But this will only work if we truly make it so that any shortcut is the same in both platforms. Something that may prove problematic in the future when we realize that a given shortcut is clashing with a platform one in Windows but not in OSx (or viceversa).

Another modification that it may need is that it probably should support apps specifying different shortcuts for different platforms Not sure how complex this can get without making the calls to this component not too complex.

I'll think how this component's interface can be better defined for a more versatile use cross platforms without compromising the ease with which it's called. I'd hate for it too become a behemoth with too many props (e.g. a prop for the shortcut in macos, plus a prop for the shortcut in Windows, etc.)

gnapse avatar Nov 02 '20 21:11 gnapse

it probably should support apps specifying different shortcuts for different platforms Not sure how complex this can get without making the calls to this component not too complex.

I don't think this is the way to go. Shortcuts could just as well be dynamic (configurable by the user), could change by browser (to avoid clashes) etc. I think it's much easier to replace shortcuts on client level (e.g. the way we do here by replacing the whole keymap or tweaking some of the constants).

proxi avatar Nov 02 '20 22:11 proxi

Proper ordering of macOS modifier keys is (take from Apple Style Guide https://books.apple.com/us/book/apple-style-guide/id1161855204):

Fn (function), Control, Option, Shift, Command

proxi avatar Nov 03 '20 11:11 proxi

@proxi I can't find a similar guideline on the order for Windows, but e.g. look at some official shortcuts here: https://support.microsoft.com/en-us/windows/keyboard-shortcuts-in-windows-dcc61a57-8ff0-cffe-9796-cb9706c75eec

A common pattern is for Shift to come second: Ctrl + Shift Alt + Shift

In general, most shortcuts only use 2 modifiers Windows + Alt Windows + Shift Ctrl + Alt etc.

although sometimes, three appear too

Windows logo key  + Ctrl + Shift + B | Wake PC from blank or black screen

Windows key always goes first. So, my not very scientific deduction of order would be, more or less (it's somewhat flexible):

Windows, Ctrl, Alt, Shift

codedebugrepeat avatar Dec 10 '20 17:12 codedebugrepeat