[BUG] Global hotkeys throwing errors after dom changes

[hadihallak]

Describe the bug
When using the GlobalHotKeys component it gives errors after some components mount and unmount.
using any keys seems to give the following error even though the hotkeys are still working and actions are triggering as usual:

How are you using react hotkeys components? (HotKeys, GlobalHotKeys, IgnoreKeys etc)
GlobalHotKeys

---

Expected behavior
it should not error out.

Platform (please complete the following information):

• Version of react-hotkeys v2 prerelease 4
• Browser chrome
• OS: [windows, macos]

Are you willing and able to create a PR request to fix this issue?
I did a quick dive and by adding || [] at the end of this line, simply solves the issue in our usecase. not sure if this fix is right or if any deeper changes are needed

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

Include the smallest log that includes your issue:

Types seem to be broken so I couldn't import the configure function

[greena13]

Thanks for taking the time to write this issue up and look into its cause, @hadihallak.

I'll take a closer look into how this may be occurring and report back.

I've created a separate issue to track the fact that the configure function is not exported from the TypeScript file.

[hadihallak]

@greena13 nevermind. it actually breaks hotkeys but not all of them. thinking it has to do with triggering hotkeys events while a globalhotkeys component is un-mounted.
Will investigate more and let you know if I find anything useful

[hadihallak]

Hey, I was able to reproduce the bug here , where you can find all of the code and steps to reproduce:

[greena13]

Thanks for this, @hadihallak. It will help me tremendously.

I am hoping to get some time this weekend to look into this.

[hadihallak]

Awesome! let me know if I can help with anything 👍 .

[mrlubos]

Same issue here! ☝️

[mrlubos]

Hi @hadihallak @greena13, I have submitted a pull request fixing this issue in #157, would be great to push out an update release with this fix! 🔥

[greena13]

Hey @mrlubos. Apologies for the delay in replying.

Thanks for the pull request - I see that you closed it, however. What was the reason for this?

[mrlubos]

@greena13 I have created a private fork as I intend to make more modifications and needed it to be merged faster, sorry!

[greena13]

That's quite alright, @mrlubos. My available time to work on this library is unfortunately in fits and spurts - regrettably, there are periods where I cannot be responsive to all issues and PRs as they are submitted.

Do you have a more extensive set of fixes for react-hotkeys? I'd be glad to consider any improvements you may have made and that you feel would be useful to a wider set of users of react-hotkeys.

If not, is it still OK to incorporate the code changes you original submitted as a PR?

[mrlubos]

Understandable @greena13! Yes, there are more changes I'd like to see and bugs we need to get fixed. Please feel free to incorporate any code you like!

[StephenHaney]

Hi all... I did a deep dive and wanted to share my findings:

The Basics

AbstractKeyEventStrategy.js adds a property called unmatchedHandlerStatus. It initially sets this to null.

When _callMatchingHandlerClosestToEventTarget is invoked, if unmatchedHandlerStatus is null, it rebuilds unmatchedHandlerStatus by looping through the component list.

Sequence of Trouble

The trouble arises for us in this scenario: two GlobalHotkeys components mounted. One unmounts, the other stays. When the second component unmounts, it calls _initHandlerResolutionState which resets unmatchedHandlerStatus to null. This makes sense, now that we only have 1 component, unmatchedHandlerStatus needs to be rebuilt the next time through.

The user presses a key. _callMatchingHandlerClosestToEventTarget is invoked and rebuilds unmatchedHandlerStatus with the single GlobalHotkey that's remaining. unmatchedHandlerStatus is now an array with a single value. So far so good.

We re-mount a GlobalHotkey component again, so there are 2. This process never invokes _initHandlerResolutionState, so unmatchedHandlerStatus remains exactly as it was set when there was only 1 component (and array with a single value).

The user presses a key. _callMatchingHandlerClosestToEventTarget is invoked and does NOT rebuild unmatchedHandlerStatus since it's not null (with the outdated info from when there was only 1 GlobalHotkey component).

_callMatchingHandlerClosestToEventTarget is invoked twice as there are now 2 components. The second time it's given an index (1) that does not exist in the outdated unmatchedHandlerStatus array. This causes the crash bug originally posted in this thread.

Fixing It

@mrlubos original, closed, PR would fix this by invoking _initHandlerResolutionState whenever a new GlobalHotkey component is mounted. This will reset unmatchedHandlerStatus to null, which will cause _callMatchingHandlerClosestToEventTarget to rebuild unmatchedHandlerStatus the next time it's invoked after a new component is mounted. This might be the best approach without refactoring. I am unsure if the other reset code in _initHandlerResolutionState will cause unwanted side effects.

Alternatively, _callMatchingHandlerClosestToEventTarget could be hardened to rebuild the unmatchedHandlerStatus at the appropriate time.

Or, someone more familiar with the code than I am may see a more elegant solution. Hoping that sharing exactly why this is happening will help.

[jordantomax]

This bug is also affecting my app in production. Would you recommend forking the library temporarily, or do you think there will be a release containing the bug fix shortly? Thank you!

[greena13]

Hi @jordantomax, thanks for reaching out.

This issue will be among the first I will tackle when I can spare some time to work on this library again. However, if you are experiencing issues in production now, I would suggest you temporarily fork and return back in a couple of weeks.

[jordantomax]

@greena13 roger that, thanks for the quick reply!

[greena13]

No problem @jordantomax.

@StephenHaney, I really appreciate the work you did in investigating this!

This issue is the next one I will tackle. @mrlubos 's initial pull request will be the starting point, but I want to read through Stephen's notes a couple more times to ensure I understand what's going on before attempting to resolve the optimal solution.

[mrlubos]

@greena13 The investigation @StephenHaney posted is precisely what led me to the solution I created, sounds good to me!

[greena13]

Fix is now available in v2.0.0-pre6.

[mrlubos]

Awesome @greena13 !

[jordantomax]

Beautiful @greena13! I can confirm that pre6 fixes the issue. Thank you 🙏