Bug: (Macos) pressing cmd+c or any shortcut that has cmd in it will break any registered shortcuts unless the window goes blur

[whoami-amrit]

Suppose we have a sample app like below. Assume <App /> is wrapped with <ShortcutProvider>

Sample application. Please click me!

import { useState, useEffect } from 'react'
import { useShortcut } from 'react-keybind'
import './App.css'

function App() {
    const [show, setShow] = useState(false)
    const { registerShortcut, unregisterShortcut } = useShortcut()

    console.log('hello')

    const handleShortcut = (e) => {
        console.log(e)
        setShow((prev) => !prev)
    }

    useEffect(() => {
        registerShortcut(handleShortcut, ['k'], 'show', 'show') 

        return () => {
            unregisterShortcut(['k'])
        }
        // eslint-disable-next-line
    }, [])

    return show ? (
        <h1>Shown</h1>
    ) : (
        <h1>Not shown</h1>
    )
}

export default App

If I press the cmd key or press cmd+c, the registered shortcut 'k' stops working. This is not the case in Windows though.

[tlackemann]

Thanks for filing an issue.

Taking a quick look at the sample app attached, there's an issue that could possibly be related.

handleShortcut should be a useCallback and passed as a dependency to the useEffect

const handleShortcut = useCallback(() => {
  setShow(prev => !prev);
}, []);

useEffect(() => {
  registerShortcut(handleShortcut, ['k'], 'show', 'show') 

  return () => {
    unregisterShortcut(['k'])
  }
}, [handleShortcut]);

Past that, the test suite of this library should encompass any edge-cases and they currently pass. If there's an issue with unsetting shortcuts, you're welcome to submit a PR with a failing test case and fix.

[whoami-amrit]

I don't mean to brag or be rude but not wrapping it in useCallback and not putting it in the dependency array is only a bad practise but it definitely is not the cause of the issue. I am well aware that not putting the callback in the dep array can potentially cause issues (stale functional reference) but that's precisely why I am passing a callback to setShow instead of using show state value.

The problem might be related to this https://stackoverflow.com/questions/27380018/when-cmd-key-is-kept-pressed-keyup-is-not-triggered-for-any-other-key

To demonstrate this when I am pressing & holding the cmd key followed by pressing and holding the k key. So now I have both cmd & k on hold. The keydown fired for both. But if I now release k there won't be a keyup event for k since cmd is pressed. So to get the keyup event for k I need to release the cmd key before releasing k.

Looking at the code it seems that you are not clearing the keyDowns array since there is no keyup for k when cmd is being held. A simple solution would be to reset the keyDowns ref array after a slight delay irrespective of the keyup event was found or not.

If there's an issue with unsetting shortcuts, you're welcome to submit a PR with a failing test case and fix.

This can't be replicated through a testing library because you are firing the events yourself. While irl behaviour is out of the ordinary.

[tlackemann]

It's certainly not bragging to be wrong about how to use hooks properly. Regardless, as stated in the original reply, you're welcome to submit a PR with a new test case and fix to address the problem.

[whoami-amrit]

Hi ! @tlackemann . I opened a little PR for this issue. Thanks.

[algonzale]

Bump. I am experiencing this as well.

[tapegram]

Also experiencing this!