sparksuite / react-accessible-dropdown-menu-hook

A simple Hook for creating fully accessible dropdown menus in React
http://sparksuite.github.io/react-accessible-dropdown-menu-hook
MIT License
112 stars 26 forks source link

If you hold down the escape key then click the dropdown, it doesn't open again #292

Closed canvaspixels closed 2 years ago

canvaspixels commented 2 years ago

Pretty edge case but worth noting. I have one of these Macs with the virtual escape key on the touch bar and accidentally had escape down when i opened the dropdown. After that it doesn't open again unless you refresh the page.

corymharper commented 2 years ago

@WesCossick this appears to be related to the changes we made here https://github.com/sparksuite/react-accessible-dropdown-menu-hook/commit/7c9a58355d2dababba3ea3438bd9b33078a7bff2

What's happening is the handleEveryClick click handler is being added multiple times without being removed, and firing when the button should be opening the menu leading to the menu being automatically closed. If you make the code synchronous you can't reproduce the bug.

This issue can be reproduced without the virtual escape button, just hold the physical escape button and spam click the menu button while doing so and it's pretty reliably reproduced... I suspect the key makes it easier to reproduce by triggering the item listener and the button listener at the same time, forcing the useEffect adding and removing the handleEveryClick function to fire more rapidly.

Must be some pretty strange race condition where the async tasks are getting backed up behind the callbacks that should remove them.

Thoughts about fixes? Maybe we could try something using closures to let each instance of the handleEveryClick function know if it was supposed to be removed at this point.

WesCossick commented 2 years ago

Must be some pretty strange race condition where the async tasks are getting backed up behind the callbacks that should remove them.

Are you able to confirm that the document.removeEventListener() function is being called before the document.addEventListener() function?

corymharper commented 2 years ago

Must be some pretty strange race condition where the async tasks are getting backed up behind the callbacks that should remove them.

Are you able to confirm that the document.removeEventListener() function is being called before the document.addEventListener() function?

Mmmm, I was able to confirm that the event listeners are sticking around and in some cases multiple of them are getting added, which would seem to passively suggest that, but I'll try to create a more concrete evidence.

corymharper commented 2 years ago

@WesCossick

An example where they were called in the wrong order:

Screen Shot 2022-01-18 at 3 12 08 PM

Compared to the normal behavior:

Screen Shot 2022-01-18 at 3 12 41 PM
WesCossick commented 2 years ago

Hmm, this might be a little hacky, but maybe we could remember whether an event listener has already been removed and not add it in the async code if so? Whatever value we store would need to be stored inside a property of an object, so that the arrow functions kept the reference to the object and not a copy of the value at the point when the function was defined...

corymharper commented 2 years ago

Hmm, this might be a little hacky, but maybe we could remember whether an event listener has already been removed and not add it in the async code if so? Whatever value we store would need to be stored inside a property of an object, so that the arrow functions kept the reference to the object and not a copy of the value at the point when the function was defined...

Something like this was also all I thought of.

WesCossick commented 2 years ago

Can you give it a try and see if it works?