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

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

Merged
merged 24 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
15a2349
changes files pattern to match any test file
theinterned Dec 17, 2021
272e313
introduces hotkeyCompare and normalizeHotkey
theinterned Dec 17, 2021
89b6475
cleanup
theinterned Dec 17, 2021
0daa605
formatting
theinterned Dec 20, 2021
bf39656
account for hotkey containing both Control and Meta
theinterned Dec 20, 2021
846a166
clarify var name
theinterned Dec 20, 2021
796e5b9
rename regex vars
theinterned Dec 20, 2021
6e85b53
fix some prettier formatting and linting
theinterned Dec 20, 2021
10b7698
reverse funcitonality of normalizeHotkey
theinterned Dec 20, 2021
eb49074
remove unused hotkeyCompare()
theinterned Dec 20, 2021
d4be521
removes handling of shift key for now
theinterned Dec 30, 2021
5f69e39
normalize the hotkey stored in trie edge
theinterned Dec 30, 2021
8705871
rename hotkey-compare > normalize-hotkey
theinterned Dec 30, 2021
4ab901f
update JSDoc
theinterned Dec 31, 2021
867d12e
move export to top
theinterned Dec 31, 2021
b14ab35
normalizeHotkey sorts modifiers in consistent order
theinterned Dec 31, 2021
9bb0699
document Mod key
theinterned Dec 31, 2021
96eb941
Update README.md
theinterned Jan 4, 2022
f9dc082
Update README.md
theinterned Jan 4, 2022
2a61fcb
mention linux in mod tests
theinterned Jan 4, 2022
9171795
add test for all apple platform strings
theinterned Jan 4, 2022
730ed3e
Merge branch 'main' of https://github.com/github/hotkey into theinter…
iansan5653 Oct 9, 2023
70ca175
Update README.md
theinterned Oct 10, 2023
6693f1f
restore changes to demo.html from main
theinterned Oct 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,20 @@ $ npm install @github/hotkey
```

## Usage

### HTML

``` html
```html
<!-- Single character hotkey: triggers when "j" is pressed-->
<a href="/page/2" data-hotkey="j">Next</a>
<a href="/help" data-hotkey="Control+h">Help</a>
<a href="/rails/rails" data-hotkey="g c">Code</a>
<!-- Multiple hotkey aliases: triggers on both "s" and "/" -->
<a href="/search" data-hotkey="s,/">Search</a>
<!-- Key-sequence hotkey: triggers when "g" is pressed followed by "c"-->
<a href="/rails/rails" data-hotkey="g c">Code</a>
<!-- Hotkey with modifiers: triggers when "Control", "Alt", and "h" are pressed at the same time -->
<a href="/help" data-hotkey="Control+Alt+h">Help</a>
<!-- Special "Mod" modifier localizes to "Meta" on mac, "Control" on Windows or Linux-->
<a href="/settings" data-hotkey="Mod+s">Search</a>
```

See [the list of `KeyboardEvent` key values](https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values) for a list of supported key values.
Expand Down Expand Up @@ -97,15 +104,19 @@ for (const el of document.querySelectorAll('[data-shortcut]')) {
2. At minimum a hotkey string must specify one bare key.
3. Multiple hotkeys (aliases) are separated by a `,`. For example the hotkey `a,b` would activate if the user typed `a` or `b`.
4. Multiple keys separated by a blank space represent a key sequence. For example the hotkey `g n` would activate when a user types the `g` key followed by the `n` key.
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 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. 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`.

### Example

The following hotkey would match if the user typed the key sequence `a` and then `b`, OR if the user held down the `Control`, `Alt` and `/` keys at the same time.

```js
"a b,Control+Alt+/"
'a b,Control+Alt+/'
```

🔬 **Hotkey Mapper** is a tool to help you determine the correct hotkey string for your key combination: https://github.github.io/hotkey/examples/hotkey_mapper.html
Expand Down
5 changes: 2 additions & 3 deletions karma.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ module.exports = function (config) {
config.set({
frameworks: ['mocha', 'chai'],
files: [
{ pattern: 'dist/index.js', type: 'module' },
{ pattern: 'test/test.js', type: 'module' },
{ pattern: 'test/test-radix-trie.js', type: 'module' }
{pattern: 'dist/index.js', type: 'module'},
{pattern: 'test/test*.js', type: 'module'}
],
reporters: ['mocha'],
port: 9876,
Expand Down
2 changes: 2 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import {fireDeterminedAction, expandHotkeyToEdges, isFormField} from './utils'
import eventToHotkeyString from './hotkey'
import SequenceTracker from './sequence'

export * from './normalize-hotkey'

const hotkeyRadixTrie = new RadixTrie<HTMLElement>()
const elementsLeaves = new WeakMap<HTMLElement, Array<Leaf<HTMLElement>>>()
let currentTriePosition: RadixTrie<HTMLElement> | Leaf<HTMLElement> = hotkeyRadixTrie
Expand Down
35 changes: 35 additions & 0 deletions src/normalize-hotkey.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* Normalizes a hotkey string before comparing it to the serialized event
* string produced by `eventToHotkeyString`.
* - Replaces the `Mod` modifier with `Meta` on mac, `Control` on other
* platforms.
* - Ensures modifiers are sorted in a consistent order
* @param hotkey a hotkey string
* @param platform NOTE: this param is only intended to be used to mock `navigator.platform` in tests
* @returns {string} normalized representation of the given hotkey string
*/
export function normalizeHotkey(hotkey: string, platform?: string | undefined): string {
let result: string
result = localizeMod(hotkey, platform)
result = sortModifiers(result)
return result
}

const matchApplePlatform = /Mac|iPod|iPhone|iPad/i

function localizeMod(hotkey: string, platform: string = navigator.platform): string {
const localModifier = matchApplePlatform.test(platform) ? 'Meta' : 'Control'
return hotkey.replace('Mod', localModifier)
}

function sortModifiers(hotkey: string): string {
const key = hotkey.split('+').pop()
const modifiers = []
for (const modifier of ['Control', 'Alt', 'Meta', 'Shift']) {
if (hotkey.includes(modifier)) {
modifiers.push(modifier)
}
}
modifiers.push(key)
return modifiers.join('+')
}
4 changes: 3 additions & 1 deletion src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import {normalizeHotkey} from './normalize-hotkey'

export function isFormField(element: Node): boolean {
if (!(element instanceof HTMLElement)) {
return false
Expand Down Expand Up @@ -62,5 +64,5 @@ export function expandHotkeyToEdges(hotkey: string): string[][] {
output.push(acc)

// Remove any empty hotkeys/sequences
return output.map(h => h.filter(k => k !== '')).filter(h => h.length > 0)
return output.map(h => h.map(k => normalizeHotkey(k)).filter(k => k !== '')).filter(h => h.length > 0)
}
39 changes: 39 additions & 0 deletions test/test-normalize-hotkey.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import {normalizeHotkey} from '../dist/index.js'

describe('normalizeHotkey', () => {
it('should exist', () => {
assert.isDefined(normalizeHotkey)
})

const tests = [
// Base case control tests
['a', 'a'],
['Control+a', 'Control+a'],
['Meta+a', 'Meta+a'],
['Control+Meta+a', 'Control+Meta+a'],
// Mod should be localized based on platform
['Mod+a', 'Control+a', 'win / linux'],
['Mod+a', 'Meta+a', 'mac'],
['Mod+a', 'Meta+a', 'iPod'],
['Mod+a', 'Meta+a', 'iPhone'],
['Mod+a', 'Meta+a', 'iPad'],
['Mod+A', 'Control+A', 'win / linux'],
['Mod+A', 'Meta+A', 'mac'], // TODO: on a mac upper-case keys are lowercased when Meta is pressed
['Mod+9', 'Control+9', 'win / linux'],
['Mod+9', 'Meta+9', 'mac'],
['Mod+)', 'Control+)', 'win / linux'],
['Mod+)', 'Meta+)', 'mac'], // TODO: on a mac upper-case keys are lowercased when Meta is pressed
['Mod+Alt+a', 'Control+Alt+a', 'win / linux'],
['Mod+Alt+a', 'Alt+Meta+a', 'mac'],
// Modifier sorting
['Shift+Alt+Meta+Control+m', 'Control+Alt+Meta+Shift+m'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@theinterned theinterned Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

['Shift+Alt+Mod+m', 'Control+Alt+Shift+m', 'win']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make this change for sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

]

for (const [input, expected, platform = 'any platform'] of tests) {
it(`given "${input}", returns "${expected}" on ${platform}`, function (done) {
assert.equal(normalizeHotkey(input, platform), expected)
done()
})
}
})
Loading