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

[BUG] Pressing modifier keys while other keys is held triggers other key, and prevents other keys #1115

Open
psychedelicious opened this issue Jan 9, 2024 · 6 comments
Labels
enhancement New feature or request Version 5.x These tickets will be solved in Version 5.x

Comments

@psychedelicious
Copy link

Describe the bug

When a key is held down and a modifier key is pressed, the first key triggers.

To Reproduce

https://codesandbox.io/p/sandbox/long-waterfall-r3v76q?file=%2Fsrc%2FApp.tsx%3A21%2C3

Listeners are attached to window for "space" and also for the hook, logging to console on call

  1. Press and release space. Logs as expected from both window listener and hook
  2. Press and hold space, then press and release a modifier. The window listener does not fire (expected), but the hotkeys hook fires (unexpected)
  3. Press and hold a modifier, then press and release space. The window listener fires (expected) but the hook does not (unexpected)

Expected behavior

The behaviour should be the same as the window listener. Callbacks should not be called when a modifier is pressed.

I tried ignoreModifiers but that didn't change anything - I don't think we'd expect it to anyways base on my understanding of that option

Desktop (please complete the following information):

  • OS: macOS 14.0 (23A344)
  • Browser: chrome 120.0.6099.199, FF 122.0b6

Additional context

I tried versions 4.4.0, 4.4.1, 4.4.2 and 4.4.3 and they all seem to have the same issue.

@JohannesKlauss
Copy link
Owner

Yes, this is currently "by design". The hook listens to every keystroke and saves all currently held keys. If another key is pressed it looks at all held down keys and triggers each callback that matches. Otherwise combinations like a+s are not possible.
But, I agree that this is somewhat counter intuitive. Maybe it makes sense to only check for held down keys on combinations where it is necessary, something like a+s, because other combos like space or meta+s don't need that. All information is present in the event.

Thanks for raising this.

@JohannesKlauss JohannesKlauss added enhancement New feature or request Version 5.x These tickets will be solved in Version 5.x labels Jan 9, 2024
@psychedelicious
Copy link
Author

psychedelicious commented Jan 9, 2024

Thanks for the explanation. I should explain a bit more why this is a problem with a real-world example.

Our project has some modal hotkeys and this behaviour interferes.

For example, while shift is held, interactions with numbers use finer steps than normal. If you are dragging an object around, it normally snaps to a grid, but while you hold shift it doesn't snap.

While space is held, the move tool is temporarily activated (it's a different tool normally).

A common use pattern is to activate the move tool with space, move an object around a bit, then also hold shift to get finer control of it.

Unfortunately this doesn't work with the current behaviour, because the hook reports a space keyup/down when you keyup/down on shift, causing the tool to change when it shouldn't. I couldn't figure out how to derive the actual state of the space key when this occurs other than to store additional state outside the callback.

I suppose I could use e.g. isHotkeyPressed within the keydown callback to double-check that space is held down, but that feels like a lot of fanagling. There's also the second arg to the callback but it seems to not have accurate information in it.

@JohannesKlauss
Copy link
Owner

Yes I agree. This hook wasn't intended to be used in such complicated scenarios, haha, but I'll gladly improve it to get to work for this example. But it is not that simple to implement and would be a breaking change. So this needs to go into version 5.

@StefKors
Copy link

Hi @psychedelicious ! we have ran into similar issues, have you found a workaround / fix / different package that solves it for you?

@psychedelicious
Copy link
Author

Hi @StefKors , in places where I needed more control, I have just used addEventListener directly and wrote extra logic to handle my specific needs. I still use react-hotkeys-hook for most hotkeys. I haven't found any other comparable package.

In case this is helpful for you, here are some notes for how I went about managing some hotkeys:

  • I track which modifier keys are held down with window keydown/keyup listeners. I find it convenient to track their state using nanostores. The atoms are defined outside react, with a single global atom tracking each modifier key. Nanostores also provides react bindings. In a component, I can use const shift = useStore($shift) to have a shift boolean that triggers re-renders. When outside react or when I want to opt-out of react's render cycle, I can use $shift.get().
  • I reset the modifier state on window blur events so the modifiers don't get "stuck". For example, if the user presses shift and then changes to another tab before releasing shift, you don't get a keyup event, and your shift state is now incorrect.
  • I found react-hotkeys-hook's scoping to be inefficient. It triggers re-renders of components that consume the scope context, which was unacceptable for me. So I hacked together a separate scoping thing that uses nanostores instead of react context. It works similarly, with hooks like useScopeOnMount('scope-name') or const ref = useScopeOnFocus('scope-name').
  • One gotcha - react-hotkeys-hook's enabled option accepts a boolean or () => boolean. If you use both for a hotkey that shares a keycode, you can get weird conflicts. To avoid this, I never use the function form, only the boolean form of this option.

Hope that's helpful.

@StefKors
Copy link

@psychedelicious Wow that's super helpful! thanks for sharing, I was already bitten by not having blur events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Version 5.x These tickets will be solved in Version 5.x
Projects
None yet
Development

No branches or pull requests

3 participants