Add a special `Mod` modifier that translates to either `Meta` on Mac or `Control` on other platforms #66

theinterned · 2021-12-17T23:03:35Z

resolves #56

What is this?

On Mac, Meta (command) is frequently used as the modifier for shortcuts: on Windows and Linux Control is used instead. Therefor in order to correctly localize hotkeys, it's common to see code that handles swapping platform-specific modifier keys in users of this library. It would make sense to do this in the library and eliminate all that custom code!

`Mod` modifier

This PR adds support for using a special Mod modifier that automatically localizes to Control on Windows / Linux, or Meta on Mac.

The rational is that this allows an easy way to define cross-platform shortcuts. Without this feature, an implementer would have to either support both Control and Meta forms of shortcuts, or write their own code to handle swapping these.

Consistent sorting of modifiers

Although it was only documented in #62 , hotkey has always had a hard requirement that modifiers are sorted in the order that matches the order eventToHotkeyString formats the modifier keys from an event.

Since the Control and Meta keys appear in different places in the hotkey string, in order to support the Mod key, I also had to ensure that modifiers in a hotkey string are consistently sorted before comparoson to an event that is serialized via eventToHotkeyString.

So this PR also removes the hard requirement that modifiers be sorted in a strict order.

Approach

I chose to add a new normalizeHotkey function to handle localizing the Mod key and sorting the modifiers. This normalizeHotkey is applied to the hotkey string in the expandHotkeyToEdges before it is added to the Trie. This ensures that when we later look for a serialized event in the trie, the hotkey string is already normalized to match the serialized output of eventToHotkeyString.

Limitations

The "Mod" key is not currently compatible with Shift and Alt modifiers.

Because Mac treats event.key differently than Windows or Linux when the Shift and / or Alt key is pressed, it means that the Mod modifier will not localize as expected when paired with Shift. I have noted this limitation in the tests and in the README. I was hoping to address #54 and #68 in this PR too but decided to defer to keep the size of the PR down.

TODO

implement normalizeHotkey function to allow comparing a eventToHotkey string to a hotkey string containing a Mod character.
- Account for hotkeys that contain both Control and Meta!
- Account for how Mac lowercases event.key when Meta+Shift are pressed.
Make normalizeHotkey platform specific: convert Mod to Meta on Mac or Control on other platforms.
Use this normalizeHotkey to map hotkey to Trie in expandHotkeyToEdges so that the Trie contains the platform-specific modifier: use it to allow keyDownHandler to match events to hotkey strings containing the Mod alias.
Add docs about Mod character
- Add special notes about Shift etc?

Previously: map ctrl / meta to Mod. Now: map mod to meta on mac / control on other platforms Also maps upper case key to `Shift+<key>`

theinterned · 2021-12-31T16:33:30Z

examples/hotkey_mapper.html

@@ -1,45 +1,61 @@
-<!doctype html>


there's no functional change — this is just the result form running prettier on this file.

theinterned · 2021-12-31T16:34:01Z

examples/index.html

@@ -1,29 +1,33 @@
-<!doctype html>


same — there's no functional change — this is just the result form running prettier on this file.

theinterned · 2021-12-31T16:40:43Z

src/utils.ts

@@ -25,5 +27,5 @@ export function fireDeterminedAction(el: HTMLElement, path: string[]): void {
 }

 export function expandHotkeyToEdges(hotkey: string): string[][] {
-  return hotkey.split(',').map(edge => edge.split(' '))
+  return hotkey.split(',').map(edge => edge.split(' ').map(k => normalizeHotkey(k)))


this is where the normalization is applied, so we take the user-provided hotkey string, normalize (or localize?) it for the current platform and store that on the trie. Then when we later serialize a keyboard event we compare it to the normalized version of the string.

theinterned · 2021-12-31T22:32:58Z

test/test-normalize-hotkey.js

+    ['Mod+A', 'Meta+A', 'mac'], // TODO: on a mac upper-case keys are lowercased when Meta is pressed
+    ['Mod+9', 'Control+9', 'win'],
+    ['Mod+9', 'Meta+9', 'mac'],
+    ['Mod+)', 'Control+)', 'win'],
+    ['Mod+)', 'Meta+)', 'mac'], // TODO: on a mac upper-case keys are lowercased when Meta is pressed


Note that the tests on line 18 and 22 are incorrect: the hotkey Meta+A won't match on Mac: instead it should be Meta+Shift+a in the current implementation. Likewise, Meta+) should be Meta+Shift+0.

I hope to address this in #54

theinterned · 2021-12-31T22:40:35Z

README.md

+6. `"Mod"` is a special modifier that localizes to `Meta` on MacOS/iOS, and `Control` on Windows/Linux.
+   1. `"Mod+"` can appear in any order in a hotkey string. For example: `"Mod+Alt+Shift+KEY"`
+   2. Neither the `Control` or `Meta` modifiers should appear in a hotkey string with `Mod`.
+   3. Due to the inconsistent lower-caseing 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`.


I've added this note for now, but hope to resolve this inconsistency in #54 — I have made a note to update the README in that issue.

khiga8

Thanks for adding the notes around inconsistent behavior 🙇‍♀️

khiga8 · 2022-01-04T21:17:35Z

test/test-normalize-hotkey.js

+    ['Mod+Alt+a', 'Alt+Meta+a', 'mac'],
+    // Modifier sorting
+    ['Shift+Alt+Meta+Control+m', 'Control+Alt+Meta+Shift+m'],
+    ['Shift+Alt+Mod+m', 'Control+Alt+Shift+m', 'win']


Should we include linux in these tests to demonstrate that it is considered, as noted in the README?
Also, should we have tests for the other apple platform matches: /Mac|iPod|iPhone|iPad/i?

I can make this change for sure!

Thank you!

I'm not sure if this is what you had in mind for consideration of linux? 2a61fcb

I also added a test for the other Mac platform strings 9171795

README.md

khiga8 · 2022-01-04T22:00:31Z

test/test-normalize-hotkey.js

+    ['Mod+Alt+a', 'Control+Alt+a', 'win'],
+    ['Mod+Alt+a', 'Alt+Meta+a', 'mac'],
+    // Modifier sorting
+    ['Shift+Alt+Meta+Control+m', 'Control+Alt+Meta+Shift+m'],


I know this test is only meant to demonstrate sorting works but wondering if we should exclude this given it's invalid. Or maybe we can add a note that this is invalid along with note to not use Mod with Meta or Control

I think it is valid to have shortcuts that pair Control and Meta (just not either of those keys with Mod).

✅ Control+Meta+a
🚫 Mod+Control+a ... would localize as Control+Meta+a on Mac / Control+a on windows / linux
🚫 Mod+Meta+a ... would localize as Meta+a on Mac / Control+Meta+a on windows / linux
⚠️ Mod+Control+Meta+a (would localize as Control+Meta+a ... which is okay but weird)

You're right, it's technically valid on mac!

Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>

…ned/mod-character

theinterned

I can't approve this because it's my own PR, but there are a few minor tweaks regardless. Besides that I think this all makes sense. Exciting to see this finally getting finalized 😃

theinterned · 2023-10-10T15:15:50Z

pages/demo.html

+    // import {install} from '../dist/index.js'
+    import {install} from 'https://unpkg.com/@github/hotkey@latest?module'


I think given the change @iansan5653 just made in #91 this change should no longer apply

removed this change 6693f1f

README.md

given changes from #91 this change was no longer relevant

JoyceZhu · 2023-10-10T15:35:03Z

README.md

-5. Modifier key combos are separated with a `+` and are prepended to a key in a consistent order as follows: `Control+Alt+Meta+Shift+KEY`.
-6. 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`.
+5. Modifier key combos are separated with a `+` and are prepended to a key in a consistent order as follows: `"Control+Alt+Meta+Shift+KEY"`.
+6. `"Mod"` is a special modifier that localizes to `Meta` on MacOS/iOS, and `Control` on Windows/Linux.


Super nitpick, control on Android as well? I'm not really familiar with that ecosystem

I think technically Android is Linux, so that might cover it?

oh, right -- that's fair! I've read enough "Android isn't REAL Linux" stuff on the internet I completely forgot about this 😆

theinterned added 3 commits December 17, 2021 17:08

changes files pattern to match any test file

15a2349

introduces hotkeyCompare and normalizeHotkey

272e313

cleanup

89b6475

theinterned self-assigned this Dec 17, 2021

theinterned added the enhancement New feature or request label Dec 17, 2021

theinterned added 7 commits December 20, 2021 12:23

formatting

0daa605

account for hotkey containing both Control and Meta

bf39656

clarify var name

846a166

rename regex vars

796e5b9

fix some prettier formatting and linting

6e85b53

reverse funcitonality of normalizeHotkey

10b7698

Previously: map ctrl / meta to Mod. Now: map mod to meta on mac / control on other platforms Also maps upper case key to `Shift+<key>`

remove unused hotkeyCompare()

eb49074

theinterned mentioned this pull request Dec 21, 2021

Hotkey version 2 #65

Closed

3 tasks

theinterned added 4 commits December 31, 2021 11:36

removes handling of shift key for now

d4be521

normalize the hotkey stored in trie edge

5f69e39

rename hotkey-compare > normalize-hotkey

8705871

update JSDoc

4ab901f

theinterned force-pushed the theinterned/mod-character branch from 112547a to 4ab901f Compare December 31, 2021 16:36

move export to top

867d12e

theinterned commented Dec 31, 2021

View reviewed changes

theinterned added 2 commits December 31, 2021 17:12

normalizeHotkey sorts modifiers in consistent order

b14ab35

document Mod key

9bb0699

theinterned commented Dec 31, 2021

View reviewed changes

theinterned marked this pull request as ready for review December 31, 2021 22:43

theinterned requested a review from a team as a code owner December 31, 2021 22:43

theinterned requested review from koddsson and JoseInTheArena December 31, 2021 22:43

theinterned mentioned this pull request Dec 31, 2021

eventToHotkeyString inconsistent shift key behaviour on Mac #54

Closed

theinterned changed the title ~~[WIP] Add a special Mod character that translates to either Meta on Mac or Control on other platforms~~ Add a special Mod character that translates to either Meta on Mac or Control on other platforms Dec 31, 2021

theinterned requested review from keithamus and khiga8 December 31, 2021 22:56

theinterned changed the title ~~Add a special Mod character that translates to either Meta on Mac or Control on other platforms~~ Add a special Mod modifier that translates to either Meta on Mac or Control on other platforms Dec 31, 2021

theinterned mentioned this pull request Jan 4, 2022

eventToHotKey inconsistent alt key behaviour on Mac #68

Closed

khiga8 approved these changes Jan 4, 2022

View reviewed changes

theinterned and others added 4 commits January 4, 2022 17:28

Update README.md

96eb941

Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>

Update README.md

f9dc082

Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>

mention linux in mod tests

2a61fcb

add test for all apple platform strings

9171795

theinterned mentioned this pull request Feb 22, 2022

Hotkey version 3 #74

Open

lb- mentioned this pull request Aug 20, 2023

Feature Request - Cross-platform "modify" for keydown filter. hotwired/stimulus#654

Open

Merge branch 'main' of https://github.com/github/hotkey into theinter…

730ed3e

…ned/mod-character

theinterned commented Oct 10, 2023

View reviewed changes

theinterned added 2 commits October 10, 2023 11:23

Update README.md

70ca175

restore changes to demo.html from main

6693f1f

given changes from #91 this change was no longer relevant

JoyceZhu approved these changes Oct 10, 2023

View reviewed changes

iansan5653 merged commit 6297512 into main Oct 10, 2023
2 checks passed

iansan5653 deleted the theinterned/mod-character branch October 10, 2023 17:03

stefannibrasil mentioned this pull request Sep 17, 2024

Lesson 1: small changes thoughtbot/botcasts#23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a special `Mod` modifier that translates to either `Meta` on Mac or `Control` on other platforms #66

Add a special `Mod` modifier that translates to either `Meta` on Mac or `Control` on other platforms #66

theinterned commented Dec 17, 2021 •

edited

Loading

theinterned Dec 31, 2021

theinterned Dec 31, 2021

theinterned Dec 31, 2021

theinterned Dec 31, 2021

theinterned Dec 31, 2021 •

edited

Loading

khiga8 left a comment

khiga8 Jan 4, 2022

theinterned Jan 4, 2022

theinterned Jan 4, 2022

khiga8 Jan 4, 2022

theinterned Jan 4, 2022 •

edited

Loading

khiga8 Jan 4, 2022

theinterned left a comment

theinterned Oct 10, 2023

theinterned Oct 10, 2023

JoyceZhu Oct 10, 2023

iansan5653 Oct 10, 2023

JoyceZhu Oct 10, 2023

		// import {install} from '../dist/index.js'
		import {install} from 'https://unpkg.com/@github/hotkey@latest?module'

Add a special Mod modifier that translates to either Meta on Mac or Control on other platforms #66

Add a special Mod modifier that translates to either Meta on Mac or Control on other platforms #66

Conversation

theinterned commented Dec 17, 2021 • edited Loading

What is this?

Mod modifier

Consistent sorting of modifiers

Approach

Limitations

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theinterned Dec 31, 2021 • edited Loading

Choose a reason for hiding this comment

khiga8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theinterned Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theinterned left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add a special `Mod` modifier that translates to either `Meta` on Mac or `Control` on other platforms #66

Add a special `Mod` modifier that translates to either `Meta` on Mac or `Control` on other platforms #66

theinterned commented Dec 17, 2021 •

edited

Loading

`Mod` modifier

theinterned Dec 31, 2021 •

edited

Loading

theinterned Jan 4, 2022 •

edited

Loading