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] keyup event is not working in 2.0.0-pre5 #166

Closed
shtrih opened this issue Apr 25, 2019 · 7 comments
Closed

[BUG] keyup event is not working in 2.0.0-pre5 #166

shtrih opened this issue Apr 25, 2019 · 7 comments

Comments

@shtrih
Copy link

shtrih commented Apr 25, 2019

Describe the bug
keyup key event is not trigger a callback.

Code

class TodoApp extends React.Component {
    constructor(props) {
        super(props);
        this.state = {
            controlDown: null
        };

        this.hotKeys = {
            controlDown: [
                {sequence: 'ctrl', action: 'keydown'},
                {sequence: 'ctrl', action: 'keyup'},
            ]
        };

        this.hotKeyHandlers = {
            controlDown: (e) => {
                if (typeof e.persist === "function") {
                    e.persist();
                }

                console.log(e);

                if (e.repeat) {
                    return;
                }

                this.setState({
                    controlDown: e.type !== 'keyup'
                });
            },
        };
    }

    render() {
        return (
            <HotKeys keyMap={this.hotKeys} handlers={this.hotKeyHandlers}>
                <h2>Control is down: {this.state.controlDown + ''}</h2>
            </HotKeys>
        )
    }
}

Expected behavior
Expected call of keyup event handler.

Here live for 2.0.0-pre5: https://jsfiddle.net/shtrih/w6oqux43/
Here for 1.1.4 (work as expected): https://jsfiddle.net/shtrih/w6oqux43/100/

Platform (please complete the following information):

  • Version of react-hotkeys: 2.0.0-pre5
  • Browser: Chrome 73.0.3683.103 (Official, 64 bit)
  • OS: Windows 10

APPLICABLE TO v2.0.0-pre1 AND ABOVE: ======================

Include the smallest log that includes your issue:

HotKeys (F0📕-E1💚-C1⭐️-P0🔺:) New 'Control' keydown event.
HotKeys (F0📕-E1💚-C0🔺-P1⭐️:) Found action that matches 'Control' (sub-match: 'Control'): controlDown. Calling handler . . .
HotKeys (F0📕-E1💚-C0🔺-P1⭐️:) Ignored 'Control' keydown as it has already been handled.
HotKeys (F0📕-E3💛-C1⭐️-P0🔺:) New (simulated) 'Control' keypress event.
HotKeys (F0📕-E4💜-C1⭐️-P0🔺:) New 'Control' keyup event.
(2)HotKeys (F0📕-E4💜-C0🔺-P1⭐️:) No matching actions found for 'Control' keyup.
HotKeys (F0📕-E4💜-C1⭐️-P0🔺:) Doesn't define a handler for 'Control' keyup.

What Configuration options are you using?

configure({
  logLevel: 'debug'
})
@ghost
Copy link

ghost commented Apr 28, 2019

Same here. 😕

@greena13
Copy link
Owner

Hey @shtrih, thank you for the great write-up. Eye-balling the logs, it seems the keyup event is indeed being triggered/registered correctly - but it's not finding the correct matcher. I'll take a look further into this.

@greena13
Copy link
Owner

@shtrih , after looking into this a tiny bit more, I wanted to clarify what your expectations are. Are you expecting the same handler to be called when the ctrl key is pressed down (keydown) and when it is released (keyup)?

This is somewhat unusual, but should in theory work as described. If you could humour me and help diagnose the issue - if you split the handling of the keydown and the keyup events into two handlers, are both called correctly (on keydown and keyup, respectively)?

The other thing that comes to mind is that your keydown event is setting state on the component. This may be the reason why the component is not properly registering the keyup handler, as it is somewhere in the process of updating the same component that is still expected to be listening for the keyup event. This is unfortunately very hand-wavey at this point, as it will take some time to get my headspace back into the particulars of how the code handles this, but it may be the source of your issue.

I have tested 2.0.0-pre5 and generally keyup events seem to be working - even those for the crtl character, which requires some special handling. So I suspect the issue may be in the particulars of your situation. Would you mind trying your handler with a key other than crtl to see if you get different results?

I look forward to hearing what results you get.

@shtrih
Copy link
Author

shtrih commented Apr 30, 2019

Are you expecting the same handler to be called when the ctrl key is pressed down (keydown) and when it is released (keyup)?

Yes.

handler with a key other than crtl to see if you get different results?

https://jsfiddle.net/shtrih/w6oqux43/112/F key keyup/keydown works fine.
And controlKeyUp handler with only keyup ctrl sequence does not work.

@greena13
Copy link
Owner

greena13 commented May 3, 2019

Ok, so what I am getting from your responses is that:

  1. This issue may be unique to the ctrl character (as it works fine with the f key)
  2. The keyup handler is still not called, when you use different handler functions for keydown and keyup.

Playing around with your JS fiddle a little more, the bug also seems to occur for other keys where the keypress event is simulated (such as shift).

Turning off simulating key press events seems to fix it (I believe @marcosabb previously suggested this, but I can no longer see the comment for some reason):

const {
    HotKeys, configure
  } = ReactHotkeys;
  
  configure({simulateMissingKeyPressEvents: false});

This gives me a great starting point for diagnosing what is causing this issue. Thanks for your help in troubleshooting. I'll look into this further and (hopefully) have a fix in the next pre-release.

@ghost
Copy link

ghost commented May 3, 2019

@greena13 Yes, the option { simulateMissingKeyPressEvents: false } did worked for me, I think I deleted the wrong comment, sorry. 😊

@greena13
Copy link
Owner

greena13 commented Jun 2, 2019

Fix should be available in v2.0.0-pre7.

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

No branches or pull requests

2 participants