feat: Add dynamic keycode for primary modifier key #9571
Conversation
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @gonfunko! It's a nice simplification. I have a couple of suggestions, PTAL.
core/utils/keycodes.ts
Outdated
|
|
||
| // The primary modifier key on the current platform, i.e. Command on Apple | ||
| // platforms and Control elsewhere. | ||
| CTRL_CMD = userAgent.MAC || userAgent.IPHONE || userAgent.IPAD ? 91 : 17, |
There was a problem hiding this comment.
Could this be changed to reference CTRL and MAC_WK_CMD_LEFT directly rather than repeating the constant values directly?
| 'hideChaff', | ||
| ); | ||
| }); | ||
| const testCases = [ |
There was a problem hiding this comment.
For here & below: should we perhaps keep these? It seems useful to actually test that both types of shortcuts work on their respective systems (though I assume this will require adapting the parametrization to support updating the user agent so that the correct key code is selected?
Or, are you able to confirm that both ctrl and cmd are tested based on whether these tests are run on Linux or OS X?
There was a problem hiding this comment.
They should be both tested on the respective platforms, but we don't actually run CI under macOS (see #2114). I'm also a bit stuck on how to actually deal with the user agent; Sinon can't stub the fields directly because ES Modules, and the useragent utility resolves the user agent once on load and stores the results in the non-stubbable constants, so even if you change navigator.userAgent you'd need to do a full page reload for that to be captured I think.
The basics
The details
Resolves
Fixes #8836
Proposed Changes
This PR adds
KeyCodes.CTRL_CMD, which dynamically resolves to either Control or Command depending on the current platform. This allows keyboard shortcuts to be registered with the expected modifier key on all platforms, and without also being registered for other platforms' modifier key.