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: (Macos) pressing cmd+c or any shortcut that has cmd in it will break any registered shortcuts unless the window goes blur #576

Open
Elessar1802 opened this issue Oct 7, 2024 · 6 comments · May be fixed by #577

Comments

@Elessar1802
Copy link

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
Copy link
Contributor

tlackemann commented Oct 7, 2024

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.

@Elessar1802
Copy link
Author

Elessar1802 commented Oct 8, 2024

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.

Screenshot 2024-10-08 at 10 02 32 AM

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
Copy link
Contributor

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.

@Elessar1802
Copy link
Author

Elessar1802 commented Oct 14, 2024

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

@algonzale
Copy link

Bump. I am experiencing this as well.

@tapegram
Copy link

Also experiencing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants