gaucho-labs / leptos-hotkeys

a declarative way of using keyboard shortcuts + callbacks in leptos applications
https://leptos-hotkeys.vercel.app
MIT License
43 stars 8 forks source link

Key is re-triggered when other key is let go #119

Closed maxbergmark closed 1 week ago

maxbergmark commented 1 month ago

Hey! I've been using this crate in a new project, and everything is working great except for one small issue. This issue can be reproduced on the demo page.

image

Steps to reproduce

  1. Press and hold down arrow up (Count goes up by 1)
  2. Quickly press and release arrow right (Count goes up by 2)
  3. Press arrow right (Count goes up by 1)
  4. Release arrow right (Count goes up by 1)
  5. Release arrow up (Count is unchanged)

It seems as if the up arrow event is triggered when the right arrow is released, as long as the up arrow is held down. I have also noticed that the effect is quite inconsistent depending on which arrow key is held down, and which is pressed after.

Another way to visualize the behavior can be seen on my Leptos Sudoku game/solver. If you click a square there, you are able to change your selected square using the arrow keys. But if you accidentally press two arrow keys at the same time, the results are not as expected.

friendlymatthew commented 1 month ago

Thanks for creating an issue @maxbergmark! FYI -- work's been ramping up and it'll take me some time to get to this...

Feel free to open a PR and try attacking this issue! (The entire codebase isn't that big either!)

It seems as if the up arrow event is triggered when the right arrow is released, as long as the up arrow is held down. I have also noticed that the effect is quite inconsistent depending on which arrow key is held down, and which is pressed after.

The demo actually binds ArrowUp or ArrowRight keys to increment the count. Same behavior using ArrowDown or ArrowLeft to decrement the count. I just never updated the demo to include that. https://github.com/gaucho-labs/leptos-hotkeys/blob/4b977cf92b5b3801d7610c723f83239ef6daf804/examples/demo/src/app.rs#L78-L88



Another way to visualize the behavior can be seen on my Leptos Sudoku game/solver. If you click a square there, you are able to change your selected square using the arrow keys. But if you accidentally press two arrow keys at the same time, the results are not as expected.

Ah -- this is bad (the undesired behavior). I'll have to spend more time investigating this and see where it went wrong. But once again, I'm pretty busy atm.

Also, the web app looks very sleek!

maxbergmark commented 1 month ago

I did some additional testing by enabling the "debug" option for the crate. From there, it is quite apparent what's happening. Here, I started by pressing arrow up, and then quickly pressed and released arrow right while arrow up was held. image

The culprit seems to be this code in use_hotkeys.rs:

let mut pressed_keyset = pressed_keys.get();
if let Some(matching_hotkey) = parsed_keys
    .iter()
    .find(|hotkey| is_hotkey_match(hotkey, &mut pressed_keyset))

With how the signal pressed_keys is set up, it will fire events both on keyup and keydown events (both of course modify the underlying hashset). With the key presses described above, the following events are triggered:

  1. KeyDown {"ArrowUp"}
  2. KeyDown {"ArrowUp", "ArrowRight"}
  3. KeyUp {"ArrowUp"}
  4. KeyUp {}

Here, it is clear why "ArrowUp" is triggered three times, even if the button is only pressed once. It would also explain why the counter only increments twice (instead of thrice) on the demo page. This is because it is a single hotkey matching both "ArrowUp" and "ArrowRight", as opposed to two separate hotkeys. So step 2 above wouldn't trigger two separate events, but only one.

The easiest fix I could think of would be to also track the last pressed key separately from the hashset, and only trigger hotkey events that include the last pressed key.

maxbergmark commented 1 month ago

Quick update: I think I have a fix for the problem. I just replaced the existing hashmap containing all keypresses with a struct:

#[derive(Debug, Default, Clone)]
#[cfg_attr(feature = "ssr", allow(dead_code))]
pub struct KeyPresses {
    pub keys: std::collections::HashMap<String, web_sys::KeyboardEvent>,
    pub last_key: Option<String>,
}
maxbergmark commented 1 month ago

I created a PR, and tested the functionality locally using the sudoku app I linked above. From my testing, all of these things work as expected:

  1. Single key presses
  2. Pressing two separate key presses simultaneously
  3. Pressing a key while another key is held down
  4. Using a multi-key hotkey while one or more other keys are held down
  5. Pressing a key while some multi-key hotkey is held down

I would really like to avoid breaking any existing applications using Leptos Hotkeys, so I would appreciate an extra set of eyes on the code changes. And if there's some refactoring needed before merging this, let me know!