-
Notifications
You must be signed in to change notification settings - Fork 151
Support for modifier keys in sequence hotkeys #1261
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
base: main
Are you sure you want to change the base?
Support for modifier keys in sequence hotkeys #1261
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
I seem to have pushed the .lock file by accident, if any maintainer can remove it please do, otherwise I'll do so soon |
I think you have to remove that yourself |
test('should trigger sequence with modifier keys with useKey', async () => { | ||
const user = userEvent.setup() | ||
const callback = vi.fn() | ||
|
||
renderHook(() => useHotkeys('shift>alt>caps>ctrl', callback, { useKey: true })) | ||
|
||
await user.keyboard(`{Shift}{Alt}{CapsLock}{Control}`) | ||
|
||
expect(callback).toHaveBeenCalledTimes(1) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand what use cases this behavior unlocks. You can just remove useKey
to get the same effect.
The purpose of useKey
is to listen to the produced key, no matter how it got produced, so we don't care about the keyboard layout. Checking for modifiers when setting useKey
to true will counter this idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of useKey
from reading the docs is as you said, we just need the user to reach the referenced key no matter how they got to it, and the first argument of useHotkeys
is simplified syntax of referencing keys regardless of useKey
's state; e.g up
means arrowup
in all contexts:
// Both of these trigger when user presses the arrow up
useHotkeys('up', callback, { useKey: false }));
useHotkeys('up', callback, { useKey: true }));
And so it follows that the same should happen to modifiers, I verified that with this:
// Both of these trigger when user presses either LeftShift of RightShift
useHotkeys('shift', callback, { useKey: false }));
useHotkeys('shift', callback, { useKey: true }));
The above behavior seems to be intended because *Shift
maps to shift
, therefor it makes sense for a sequences that's composed of a modifier to trigger like any other sequence.
Previously, sequences didnt work with modifiers, they would just be ignored, a sequence like shift>i>alt
was identical to i
because shift
and alt
would get ignored, but with this PR the user would actually have to press shift
, i,
then alt
to trigger the callback.
useKey
would be noticeable in a sequence like this:
test('should not trigger sequence without useKey', async () => {
const user = userEvent.setup({delay: 200})
const callback = vi.fn()
renderHook(() => useHotkeys('%>!', callback, { useKey: false }))
await user.keyboard(`{Shift>}{%}{/Shift}{Shift>}{!}{/Shift}`)
expect(callback).toHaveBeenCalledTimes(0)
})
Notice how the sequence doesnt actually get triggered when the user presses shift
first, because the sequence explicitly states that the first character be %
, but given in most circumstances the user would actually not be able to directly send %
as a key event, we can set useKey
to true
to completely ignore the modifiers the user used to create the %
key event:
test('should trigger sequence with useKey', async () => {
const user = userEvent.setup()
const callback = vi.fn()
renderHook(() => useHotkeys('%>!', callback, { useKey: true }))
await user.keyboard(`{Shift>}{%}{/Shift}`)
vi.advanceTimersByTime(200)
await user.keyboard(`{Shift>}{!}{/Shift}`)
expect(callback).toHaveBeenCalledTimes(1)
})
So now with useKey
set to true
, the implementation will check if the modifier key the pressed was actually used to create the %
key event. In the above test case, when the user presses shift
, the code will ignore it and wait for the next key, if the latter is actually what we expected, in our case %
, we will verify that the last modifier key was a direct cause of %
, if so then we process the next hotkey in the sequence.
test('should trigger sequence with capslock as modifer without useKey', async () => { | ||
const user = userEvent.setup({delay: 200}) | ||
const callback = vi.fn() | ||
|
||
renderHook(() => useHotkeys('1>2>3', callback, {useKey: false})) | ||
|
||
await user.keyboard(`{CapsLock>}{1}{2}{3}`) | ||
|
||
expect(callback).toHaveBeenCalledTimes(1) | ||
}) | ||
|
||
test('should trigger sequence with capslock as modifer with useKey', async () => { | ||
const user = userEvent.setup({delay: 200}) | ||
const callback = vi.fn() | ||
|
||
renderHook(() => useHotkeys('1>2>3', callback, { useKey: true })) | ||
|
||
await user.keyboard(`{CapsLock>}{1}{2}{3}`) | ||
|
||
expect(callback).toHaveBeenCalledTimes(1) | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here, why is the behavior identical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some layouts such as the french azerty that requires you to have capslock on before being able to send a digit keystroke because their default is the symbol not the digit, if we dont respect that then they wont be able to trigger sequences of digits.
Those two tests take two different code paths but both of them trigger the callback.
- With
useKey
set tofalse
, the user will presscapslock
first which is not part of the sequence so nothing will happen, then they can press 1, 2 and 3 to trigger. - With
useKey
set totrue
, the user will presscapslock
first which will be processed as a modifier so it will be put aside until the next key comes in, if it is1
then the sequence can complete and trigger.
@Aspecky If you can clean up the PR, I'll merge it 🙂 Sorry for the late response! |
I've re-implemented the sequence hotkey to add support for modifier keys even when
useKey
istrue
.I've also added a
caps
andctl
to the modifier keys map for greater compatibility,caps
being shorthand forcapslock
andctl
forctrl
per the mdn docs: