Skip to content
This repository has been archived by the owner on Aug 14, 2022. It is now read-only.

Cannot press Escape to close Dialog #249

Open
lqt93 opened this issue Jul 3, 2021 · 5 comments
Open

Cannot press Escape to close Dialog #249

lqt93 opened this issue Jul 3, 2021 · 5 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@lqt93
Copy link
Contributor

lqt93 commented Jul 3, 2021

Can not close the Dialog by pressing Escape. It is easy to see in the Basic example of Dialog in Moai docs:
https://docs.moaijs.com/?path=/docs/components-dialog--primary

@lqt93 lqt93 added bug Something isn't working help wanted Extra attention is needed labels Jul 3, 2021
@thien-do
Copy link
Collaborator

thien-do commented Jul 3, 2021

Yeah... the problem is we are binding the "onEscape" event at the wrapper div of the dialog:

onKeyDown={(event) => {
if (event.key === "Escape") props.onEsc?.();
}}

This makes the code real simple, but it won't work if the user is not focusing on the dialog (technically: document.activeElement is inside the wrapper div).

At the moment I'm not really sure what should be done here:

  • We could bind the event to the document element, which will work but I don't feel right...,
  • Or, we should keep the listener at the wrapper, and "trap" the user's focus (technical) inside the dialog (e.g. they can't even "tab" outside of it")

Anyway, should look at other leading dialog to see how they are handling this.

@thien-do
Copy link
Collaborator

thien-do commented Jul 3, 2021

@nhducit @ZeroX-DG @monodyle We're more than happy to hear any opinion from you guys

@monodyle
Copy link
Contributor

monodyle commented Jul 3, 2021

The problem is the dialog is not focused when the dialog shows up. So you can't close dialog via keydown event until you got focused on them. I have to quickly look on rc-dialog, which used by ant.design, and see that they focus on the dialog content, which inside the wrapper.

https://github.com/react-component/dialog/blob/cb6f8f0dbad9656b03d18c036ba4f7525b31d3d0/src/Dialog/index.tsx#L64-L70

Understandable because when you show up the dialog, which has a z-index over all others, then the tabindex focusing should be on this. See this example from w3 for better visualization.

@lqt93
Copy link
Contributor Author

lqt93 commented Jul 3, 2021

I totally agree with focusing on the content of dialog whenever it is opened.

@thien-do
Copy link
Collaborator

thien-do commented Jul 5, 2021

oh thanks @monodyle ! Didn't know w3 has an example for dialog!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants