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] - Handler for / is getting called for ? #161

Closed
FutureKode opened this issue Apr 3, 2019 · 12 comments
Closed

[BUG] - Handler for / is getting called for ? #161

FutureKode opened this issue Apr 3, 2019 · 12 comments

Comments

@FutureKode
Copy link

FutureKode commented Apr 3, 2019

Describe the bug
Handler for / is getting called for ?

How are you using react hotkeys components? (HotKeys, GlobalHotKeys, IgnoreKeys etc)
One GlobalHotKeys to handle / to focus a search input
Another GlobalHotKeys to handle ? to show a hotkeys help modal.

Expected behavior
I would expect ? to not match /

Platform (please complete the following information):

  • Version: "^2.0.0-pre4"
  • Browser: Chrome
  • OS: MacOS

Are you willing and able to create a PR request to fix this issue?
Possibly

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

Include the smallest log that includes your issue:

HotKeys (GLOBAL-E3💛-C0🔺): Found action that matches '?+Shift' (sub-match: '/'): SHOW_SEARCH. Calling handler . . .
GlobalKeyEventStrategy.js?0da2:563 HotKeys (GLOBAL-E3💛-C0🔺): Stopping further event propagation.

What Configuration options are you using?

None

@FutureKode
Copy link
Author

Is there a way to turn off this sub-match feature?

@mrlubos
Copy link

mrlubos commented Apr 6, 2019

@FutureKode Not at the moment

@greena13
Copy link
Owner

greena13 commented Apr 8, 2019

Hi @FutureKode,

Apologies for the delay - I have been unable to carve out time to tend to this library as much as I would like, recently.

Thank you for your quality issue report. This is an interesting one. Could you please clarify whether you are using a keyboard with a layout where the / and ? are not on the same keys? Or is there some other way that you can trigger a ? keystroke without also pressing the shift key?

If not, do you still see the behaviour if you bind to / and shift+??

@greena13 greena13 reopened this Apr 8, 2019
@greena13
Copy link
Owner

greena13 commented Apr 8, 2019

And thanks for helping out with the support load, @mrlubos.

@mrlubos
Copy link

mrlubos commented Apr 8, 2019

@greena13 No problem Alex. I believe this is the same issue as attaching a handler on “2” and then pressing ⌘ + 2 to navigate away from a browser tab - the action gets called through submatch even though logically it seems it shouldn’t, right?

@Urbansson
Copy link

I'm having a similar problem with sub matching.

The problem is that a child have a handler for Enter and the parent have a handler for ⌘ + Enter.

So when you are pressing ⌘ + Enter in the child the handler for the parent never gets called because the child's handlers sub matches on Enter.

I feel like is more correct that the event checks the parents handlers before starting on finding sub matches or that we have some options to disable sub matching.

@FutureKode
Copy link
Author

I think that's the problem I was having too. Sub-matches should be checked after all match handlers have been checked.

@Chris-Slade
Copy link

I've encountered this issue as well. A handler for ctrl+s in a child HotKeys is preventing a parent HotKeys's ctrl+shift+s handler from handling that key sequence. It seems to me like the more specific handler should take precedence even if it's defined in a higher-up HotKeys.

  • Version: 2.0.0-pre5
  • Browser: Electron ^4.1.4 (Chromium 69.0.3497.128)
  • OS: macOS and Windows 10.

@mrlubos
Copy link

mrlubos commented May 8, 2019

@Chris-Slade yes, this is the approach I’d like to take!

@greena13
Copy link
Owner

Thanks for all your feedback on how useful react-hotkeys is proving for your applications, and where it's not matching your expectations.

One of react-hotkeys core features has always been (at least as I have understood it) the promise of nested key maps, so depending on what component of the page was in focus, different key maps and handlers would be given different precedence.

I believe it's the case that what you have requested (the longer key combinations and sequences take precedence) is indeed the case so long as they are defined at the same level (in the same component). That is, nesting takes precedence, and then combination/sequence length does.

However, version 2.0 of react-hotkeys separates out the definition of globally applicable key maps into a separate component (GlobalHotKeys) and really makes the developer's intent explicit (where it has previously only been implicitly achieved by setting a target of window or document). So there is perhaps less of a precedent for what the desired behaviour is when it comes to global hot keys.

My question to you all is, is the desired behaviour of key combinations' length taking precedence, irrespective or component nesting limited only to globally scoped keymaps? My hunch is yes.

The current behaviour of GlobalHotkeys was an effort to re-use the resolution algorithm used by regular HotKeys (keeping file size and maintenance costs down, and reducing surprise to users of react-hotkeys, as they would not have two resolution algorithms to reason through). However, this doesn't necessarily need to be the case.

@Chris-Slade
Copy link

@greena13 In my case I think the answer is yes. Currently I'm handling application-level shortcuts using a HotKeys component near the root of the app, but only because the docs recommend using HotKeys over GlobalHotKeys where possible — I could swap in a GlobalHotKeys pretty easily. My (current) use case is simple, however. YMMV.

I'm wondering if it would be desirable/feasible to continue re-using the resolution algorithm for "regular" and global hotkeys, but to extend the key-map API to allow some form of configuration for which handlers are invoked in the case of there being overlapping shortcut definitions in a tree of HotKeys components. Something like

const keyMap = {
    SEARCH: '/',
    HELP: { key: 'shift+/', overrideSubmatch: [ '/' ] },
};

@greena13
Copy link
Owner

greena13 commented Jun 2, 2019

Hey @FutureKode, I believe this should be fixed in v2.0.0-pre7. Please let me know if that's not the case.

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

5 participants