Include Shift key with uppercase letter hotkeys
#115
Conversation
| @@ -22,7 +22,7 @@ | |||
| // "forwardPorts": [], | |||
|
|
|||
| // Use 'postCreateCommand' to run commands after the container is created. | |||
| // "postCreateCommand": "yarn install", | |||
There was a problem hiding this comment.
Unrelated change I tacked on because it's annoying that I have to install deps in every codespace (and that means that my ESLint and Prettier plugins don't work initially)
| @@ -262,19 +262,18 @@ describe('hotkey', function () { | |||
|
|
|||
| describe('eventToHotkeyString', function () { | |||
| const tests = [ | |||
| ['Control+J', {ctrlKey: true, shiftKey: true, code: 'KeyJ', key: 'J'}], | |||
| ['Control+Shift+j', {ctrlKey: true, shiftKey: true, code: 'KeyJ', key: 'j'}], | |||
There was a problem hiding this comment.
This test doesn't really make sense - it's Shift but with a lowercase letter. I deleted it.
There was a problem hiding this comment.
it looks like maybe this can be traced to a8e22ee ?
Although that test should be Meta+Shift+j? I see there's a test for that below on line 268. So maybe this is just there for completeness.
There was a problem hiding this comment.
At any rate this does bring up an important point: are we sure that Mod+Shift+M will work consistantly across platforms?
We may want to lowercase the keynames or something?
There was a problem hiding this comment.
This test doesn't really make sense - it's Shift but with a lowercase letter. I deleted it.
This probably could have had a comment to describe why it's necessary - but it is in the commit information Ned points out. The (incorrect) assumption here is that a Shift key turns printable characters into their uppercase variants, however the Shift key does not always mean that the casing of the printable key will be uppercase. See for e.g. https://bugs.webkit.org/show_bug.cgi?id=174782. This issue is complex because if we try to coerce macos to do the right thing, it means some character shortcuts cannot be represented.
There was a problem hiding this comment.
Makes sense, added it back 👍
are we sure that
Mod+Shift+Mwill work consistantly across platforms?
Actually, Mod+Shift+M won't work across any real-world (non-test) platform. This would be Mod+Shift+m, because keys are always lowercase when Mod is held regardless of platform.
There was a problem hiding this comment.
Mod+Shift only results in a lowercase letter on Mac AFAIK. which is what my comment was trying to say.
There was a problem hiding this comment.
I am 95% sure it does on Windows as well, but I'll double check that.
There was a problem hiding this comment.
Well crap, on Windows it results in an uppercase key.
This is not the conclusion we came to two months ago when we had this discussion on Slack, and we made some critical decisions based on that conclusion. This is really going to complicate things 🙃
There was a problem hiding this comment.
Yes, Mac+Safari will not uppercase the printable character if the Cmd key is in the chord. Linux and (I believe) windows will uppercase the printable character if the Ctrl key is in the chord.
Linux will also uppercase the printable character if the Meta key is in the chord.
Effectively you can consider it that Win+Linux will always uppercase the printable character if the Shift key is in the chord while Mac will only uppercase the printable character for the Ctrl+Shift and Shift planes (but not Meta+Shift.
This means we should either normalise Mod shortcuts to always lowercase (in line with Mac) or uppercase (inline with Windows+Linux). FWIW when we visited this problem in 2017 there were certain combinations with either choice prevented, but I'm fuzzy on the details, it being 6 years ago now.
README.md
Outdated
| @@ -111,6 +111,7 @@ for (const el of document.querySelectorAll('[data-shortcut]')) { | |||
| 3. Due to the inconsistent lowercasing of `event.key` on Mac and iOS when `Meta` is pressed along with `Shift`, it is recommended to avoid hotkey strings containing both `Mod` and `Shift`. | |||
| 7. `"Plus"` and `"Space"` are special key names to represent the `+` and ` ` keys respectively, because these symbols cannot be represented in the normal hotkey string syntax. | |||
| 8. You can use the comma key `,` as a hotkey, e.g. `a,,` would activate if the user typed `a` or `,`. `Control+,,x` would activate for `Control+,` or `x`. | |||
| 9. `"Shift"` should be included if it would be held, and the key name should match the 'uppercase' key name: `Shift+?`, `Shift+A`, `Shift+$` | |||
There was a problem hiding this comment.
We should alter this explanation as I don't think it holds true.
There was a problem hiding this comment.
Which part of it? It should be correct if this PR works as expected.
There was a problem hiding this comment.
we do have point #3 above:
- Due to the inconsistent lowercasing of
event.keyon Mac and iOS whenMetais pressed along withShift, it is recommended to avoid hotkey strings containing bothModandShift.
but I think what Keith is saying is this isbn't true on a Mac when the Cmd key is held.
There was a problem hiding this comment.
Updated to
"Shift"should be included if it would be held and the key is uppercase: ie,Shift+AnotA. Note however that MacOS outputs lowercase keys whenMeta+Shiftis held (ie,Meta+Shift+a); see 6.3 above.
Currently we only include the
Shiftkey in the hotkey string if the user is not typing a letter. This is inconsistent: for any other modifier, we always include it in the string if held. But forShift, we only sometimes include it in the string if held.To make hotkey strings easier to reason with, this PR changes the logic to always include
Shiftif held.Important
This is a breaking change we should release with a major version bump (
3.0.0). Any existing hotkeys that use uppercase letters withoutShiftwill need to be updated to addShift. SoAwould becomeShift+A,Bwould becomeShift+B, etc. This does mean that these hotkeys will no longer fire if the letter is pressed alone whenCapsLockis on. I think this is acceptable because this behavior was never clearly defined in the docs and it is unintuitive that different effects might occur depending on whether caps lock is on or not.This closes #54 by making
Shifthandling consistent across all keys and platforms.