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

fix: close overlay stack with Escape key [WIP] #3385

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

DavideMininni-Fincons
Copy link
Contributor

@DavideMininni-Fincons DavideMininni-Fincons commented Feb 5, 2025

The proposed implementation revolves around a new base class which maintains a registry of overlays;
it can be improved in a couple of points.

  1. If a user opens an overlay (eg. a dialog) and then changes story in Storybook, the overlay is not removed from the registry and it's still considered opened (isOpen=true). The usage of the WeakRef API could (in theory) solve this issue; however I tried to force the garbage collector with the Chrome Devtools console and nothing happened.
  2. Related to previous point, since it's possible that the stack is filled with old overlay's instances, hitting ESC with no visible overlays will call the close method on old instances. Could it lead to issues?
  3. The cleanStack method will work if the overlays are not reused (e.g element with the same id in different pages) and will not work for overlays in the ShadowDOM

A proposal discussed with @TomMenga could be to not maintain a registry and to simply use event.preventDefault combined with the check on event.defaultPrevented in all the keydown function on ESC hit.

@github-actions github-actions bot temporarily deployed to pr3385 February 6, 2025 09:36 Inactive
@github-actions github-actions bot temporarily deployed to pr3385-diff February 6, 2025 09:36 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Esc closes Dialog, when Autocomplete drop-down is focused
1 participant