[QUESTION/BUG?] Duplicate focus tree IDs cause keypresses to be ignored

[Chris-Slade]

Describe the bug
This might or might not be a bug in react-hotkeys. I'm currently working to create a minimal example that reproduces this issue, but it's possible that it's caused by some complex interactions in my application that can't be trivially reproduced.

However, in lieu of finding the exact cause, I've managed to track down the effect, and a possible fix.

I have one parent HotKeys component near the root of the application and one child HotKeys nested in it. Certain interactions in my application result in a duplicate focus tree ID being added to the _focusTreeIds queue for the outer HotKeys. When this occurs, the first keypress after clicking on the inner hotkeys will be ignored with a message like

Ignored 'a' keydown event because it had an old focus tree id: 1.

I've included a log below showing this behavior. I added some additional logging statements in a fork to demonstrate what's happening; the output from these statements is included in the log below. (Note that the line numbers correspond to the forked versions of the files. The linked commit is the only change made in this fork.)

Modifying withHotKeys.js only to queue a new focus tree ID if it's different from the previous one (see the commented out lines here) prevents this "bug" from manifesting, but I'm not sure if this fix is correct.

If it is some form of misuse that causes it, would it make sense to protect against it in this way?

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

Expected behavior
Not sure.

Platform (please complete the following information):

• Version: Tried with 2.0.0-pre5. I also linked in development builds of the library when debugging this, testing with versions 2.0.0-pre5 and b866ec1.
• Browser: Electron ^4.1.4 (Chromium 69.0.3497.128)
• OS: macOS (10.14.4) and Windows 10.

Are you willing and able to create a PR request to fix this issue?
Yes, if the proposed fix makes sense. Otherwise, maybe.

Include the smallest log that includes your issue:

withHotKeys.js:114 Rendering HotKeys component 0
withHotKeys.js:114 Rendering HotKeys component 1
withHotKeys.js:227 Handling focus (component ID: 0, is focused: undefined)
FocusOnlyKeyEventStrategy.js:209 HotKeys (F0📕-C0🔺-P0🔺:) Focused. 
withHotKeys.js:175 Focus tree IDs (after push): [0]
withHotKeys.js:114 Rendering HotKeys component 0
FocusOnlyKeyEventStrategy.js:248 HotKeys (F0📕-C0🔺-P0🔺:) Received new props.
withHotKeys.js:259 Handling blur (component ID: 0, is focused: true)
FocusOnlyKeyEventStrategy.js:280 HotKeys (F0📕-C0🔺-P0🔺:) Lost focus.
withHotKeys.js:183 Focus tree IDs (after shift): []
withHotKeys.js:227 Handling focus (component ID: 0, is focused: false)
FocusOnlyKeyEventStrategy.js:209 HotKeys (F1📗-C0🔺-P0🔺:) Focused. 
withHotKeys.js:175 Focus tree IDs (after push): [1]
withHotKeys.js:227 Handling focus (component ID: 0, is focused: true)
FocusOnlyKeyEventStrategy.js:209 HotKeys (F1📗-C0🔺-P1⭐️:) Focused. 
withHotKeys.js:175 Focus tree IDs (after push): (2) [1, 1]
ActivityWindow.tsx:164 Focusing ActivityWindow
withHotKeys.js:259 Handling blur (component ID: 0, is focused: true)
FocusOnlyKeyEventStrategy.js:280 HotKeys (F1📗-C0🔺-P1⭐️:) Lost focus.
withHotKeys.js:183 Focus tree IDs (after shift): [1]
withHotKeys.js:227 Handling focus (component ID: 1, is focused: undefined)
FocusOnlyKeyEventStrategy.js:209 HotKeys (F2📘-C1⭐️-P0🔺:) Focused. 
withHotKeys.js:175 Focus tree IDs (after push): [2]
withHotKeys.js:227 Handling focus (component ID: 0, is focused: false)
FocusOnlyKeyEventStrategy.js:209 HotKeys (F2📘-C0🔺-P1⭐️:) Focused. 
withHotKeys.js:175 Focus tree IDs (after push): (2) [1, 2]
FocusOnlyKeyEventStrategy.js:320 HotKeys (F2📘-E0❤️-C0🔺-P1⭐️:) Ignored 'a' keydown event because it had an old focus tree id: 1.
FocusOnlyKeyEventStrategy.js:669 HotKeys (F2📘-E0❤️-C0🔺-P1⭐️:) Stopping further event propagation.
withHotKeys.js:183 Focus tree IDs (after shift): [2]
FocusOnlyKeyEventStrategy.js:377 HotKeys (F2📘-E1💚-C0🔺-P1⭐️:) New 'a' keyup event.

This is with log level debug. The verbose log is too long to include here.

What Configuration options are you using?

None, other than logLevel.

[greena13]

Hi @Chris-Slade, thank you for all work you've put into investigating and reporting this.

I have had a skim of your post and it does sound like a bit of an edge-case/strange behaviour, but I will have to sit down when I have more time and consider the full context of your use case into more thoroughly.

For the time being, I just wanted to confirm that this doesn't sound like intended behaviour.

[Chris-Slade]

Thanks for the response. Since it doesn't sound intended, I'll keep trying to narrow it down to a reproducible example I can post here, or submit a PR if I understand well enough what's happening to fix it correctly.

[greena13]

This is the bit that confuses me:

withHotKeys.js:227 Handling focus (component ID: 0, is focused: false)
FocusOnlyKeyEventStrategy.js:209 HotKeys (F1📗-C0🔺-P0🔺:) Focused. 
withHotKeys.js:175 Focus tree IDs (after push): [1]
withHotKeys.js:227 Handling focus (component ID: 0, is focused: true)
FocusOnlyKeyEventStrategy.js:209 HotKeys (F1📗-C0🔺-P1⭐️:) Focused. 
withHotKeys.js:175 Focus tree IDs (after push): (2) [1, 1]

I am not sure how Component 0 can be focused twice in a row (without being blurred inbetween). It's also occupying two positions (P0 and P1) simultaneously, which is confusing - and may point to the actual underlying issue.

Could you please describe the sequence of events that causes the focus to change between the two components? Are you using the cursor to click on different components, or is the first component calling a handler than focuses the second, or is something else entirely taking place?

[greena13]

I'm still not sure how this can occur, but I've crafted a fix that I believe should handle when it does (hopefully with no unintended side-effects).

I went in a different direction than the one you suggested with the fix, as the problem was larger than just the list of focus tree ids.

The fix will go out with the next pre-release. In the meantime, if you check out the master branch directly from github I'd be very interested if this solves your problem.

[Chris-Slade]

@greena13 I've tested it with the version from master and the issue does appear to be resolved. Cheers.

[greena13]

Awesome! Great to hear it.

[greena13]

Fix is now available in v2.0.0-pre6.