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

Media-Modal: esc key closing not working as expected. #17534

Closed
spen opened this issue Aug 25, 2017 · 0 comments · Fixed by #19205
Closed

Media-Modal: esc key closing not working as expected. #17534

spen opened this issue Aug 25, 2017 · 0 comments · Fixed by #19205

Comments

@spen
Copy link
Contributor

spen commented Aug 25, 2017

Details

While working on #17150 (Replacing close-on-esc mixin) I found a (quite minor) potential issue with the behaviour of our implementation of react-modal in dialog-base.

There's logic in the existing close-on-esc that ignores the keypresses that happen while an input (or textarea) is focused.
ReactModal doesn't take this in to account and calls the close action passed to it via the onRequestClose prop regardless.
My assumption is that we'd want to extend the behaviour to our modals, allowing a user to hit escape while typing without closing their modal.

While testing this a second time I noticed some extra details too.
When opening the featured image modal and hitting esc before doing anything else, you'll notice that the modal closes - cool, as expected! ✅
However, when you open that modal and click through to an item detail view (select an item, click edit) and then attempt to hit esc you'll notice that the modal doesn't close.
Clicking back to the original list view via the <- Media Library button and then hitting esc still doesn't close the modal - uncool. ❌

I believe that this is a case of react-modal losing the focus it needs for these results - though I'm not 100% sure of those details.
I think this behaviour would make for a good case to either allow for a disableCloseOnEsc prop upstream (proposed & commented on) and handling the esc press ourselves - or just completely handling closing ourselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant