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

Cleanup function doesn't re-attach events that have been overwrited #16

Closed
zhouzi opened this issue Jul 4, 2022 · 3 comments · Fixed by #21
Closed

Cleanup function doesn't re-attach events that have been overwrited #16

zhouzi opened this issue Jul 4, 2022 · 3 comments · Fixed by #21
Labels
bug Something isn't working

Comments

@zhouzi
Copy link
Contributor

zhouzi commented Jul 4, 2022

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Other... Please describe:

Current behavior

  1. In component A, attach a listener to a key, e.g "m"
  2. In component B, attach a listener to the same key while A is still in the page
  3. Now unmount B while A is still in the page

A's key listener don't work anymore.

Expected behavior

I would expect A's listeners to not be overwritten anymore and thus work.

Minimal reproduction of the problem with instructions

https://stackblitz.com/edit/react-ts-3rlpgx?file=App.tsx

  1. In the above reproduction, press "m" to generate a random number.
  2. Click on the button to open the modal.
  3. In that modal, press "m" to close it.
  4. Now try to press "m" to pick a random number again: it doesn't work anymore.

What is the motivation / use case for changing the behavior?

We have a similar use case as the reproduction. A modal overwrites others shortcuts and when it's removed, the listeners are not re-attached (it's not re-rendered).

Environment


Libs:
- react version: latest
- reakeys version: latest


Browser:
- [x] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: XX  
- Platform:  

Others:

@amcdnl
Copy link
Member

amcdnl commented Jul 4, 2022

Sigh - I thought we fixed this. Any suggestions?

@amcdnl amcdnl added the bug Something isn't working label Jul 4, 2022
@zhouzi
Copy link
Contributor Author

zhouzi commented Jul 4, 2022

A solution could be to look for a listener with keys equal to the one that's unsubscribed in removeKeys and attach it. Something like:

const removeKeys = (nextKeys: HotkeyShortcuts[]) => {
  keys = keys.filter((k) => !nextKeys.includes(k));
  nextKeys.forEach((s) => {
    Mousetrap.unbind(s.keys, s.action);
    
    // can't use a === here but that's the idea
    const key = keys.findLast(k => k.keys === s.keys);
    if (key) {
      Mousetrap.bind(key.keys, key.callback, key.action);
    }
  });
};

That's a little tedious though. Another solution could be to unbind all listeners and re-bind all the remaining ones from keys. It has the advantage of letting Mousetrap handle priorities and overwrites.

const removeKeys = (nextKeys: HotkeyShortcuts[]) => {
  keys.forEach((s) => Mousetrap.unbind(s.keys, s.action));
  keys = keys.filter((k) => !nextKeys.includes(k));
  keys.forEach((k) => Mousetrap.bind(k.keys, k.callback, k.action));
};

@amcdnl
Copy link
Member

amcdnl commented Jul 4, 2022

If you wanna make a PR, I'd be happy to accept. There is a storybook story that tests the attaching/detaching, just need to make sure and test that w/ ur proposed change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants