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

[Bug]: Escape key doesn't close dialogs after a mouse click inside it #524

Closed
kieranm opened this issue May 14, 2021 · 6 comments
Closed

Comments

@kieranm
Copy link

kieranm commented May 14, 2021

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v1.2.0

What browser are you using?

Chrome

Reproduction repository

https://headlessui.dev/react/dialog

Describe your issue

Setup

  • Open the dialog (such as the one at the top of the example page).
  • Click the text inside the dialog to remove focus from the button.
  • Press "escape".

Expected behaviour:
The dialog captures the escape key and closes. Even though clicking moves focus off child elements, the dialog is still open and I think users would reasonably still expect to be able to close it with the keyboard.

Actual behaviour:
The dialog doesn't capture the escape key and doesn't close.

This issue is reproducible using the example on the Headless UI site so I haven't created a codesandbox specifically for it, but please let me know if one is required.

Apologies if I'm missing some important context!

Thanks!

@crnkovic
Copy link

Same issue is happening with a Vue version.

@kieranm
Copy link
Author

kieranm commented Jun 6, 2021

I'm not sure if this is the optimal solution, but as a workaround I've added a global event listener to the window...

export const Modal: React.FC<ModalProps> = (props: ModalProps) => {
  useEffect(() => {
    const handler = (e: KeyboardEvent) => {
      if (e.key === "Escape") {
        props.onClose && props.onClose(false)
      }
    }
    window.addEventListener("keydown", handler)
    return () => window.removeEventListener("keydown", handler)
  }, [])

 ...
}

@Lueton
Copy link

Lueton commented Jun 6, 2021

A possible (working) solution would be adding tabindex to the dialog. For example in dialog.tsx:

let propsWeControl = {
    ref: dialogRef,
    id,
    role: 'dialog',
    'aria-modal': dialogState === DialogStates.Open ? true : undefined,
    'aria-labelledby': state.titleId,
    'aria-describedby': describedby,


    tabIndex: 0, // <---- Possible fix


    onClick(event: ReactMouseEvent) {
      event.stopPropagation()
    },

    // Handle `Escape` to close
    onKeyDown(event: ReactKeyboardEvent) {
      if (event.key !== Keys.Escape) return
      if (dialogState !== DialogStates.Open) return
      if (hasNestedDialogs) return
      event.preventDefault()
      event.stopPropagation()
      close()
    },
  }

@darr1s
Copy link

darr1s commented Jun 9, 2021

My issue is similar, ESC key wont work anywhere unless I implement @kieranm solution, then

  1. Once modal appear
  2. I had to click anywhere, outside or inside
  3. Then ESC key would work

@cubecleveland
Copy link

I am still 1 month later can not understand the solution to this issue, do i need to fork the repo and add the props to dialog....?

RobinMalfait added a commit that referenced this issue Aug 24, 2021
We've "fixed" an issue when we had nested Dialogs ([#430](#430)).
The `escape` would not close the correct Dialog. The issue here was with
the logic to know whether we were the last Dialog or not. The issue was
_not_ how we implemented the `close` functionality.

To make things easier, we moved the global window event to a scoped div
(the Dialog itself). While that fixed the nested Dialog issue, it
introduced this bug where `escape` would not close if you click on a
non-focusable element like a span in the Dialog.

Since that PR we did a bunch of improvements on how the underlying
"stacking" system worked.
This PR reverts to the "global" window event listener so that we can
still catch all of the `escape` keydown events.

Fixes: #524
Fixes: #693
RobinMalfait added a commit that referenced this issue Aug 24, 2021
* fix broken `escape` key behaviour

We've "fixed" an issue when we had nested Dialogs ([#430](#430)).
The `escape` would not close the correct Dialog. The issue here was with
the logic to know whether we were the last Dialog or not. The issue was
_not_ how we implemented the `close` functionality.

To make things easier, we moved the global window event to a scoped div
(the Dialog itself). While that fixed the nested Dialog issue, it
introduced this bug where `escape` would not close if you click on a
non-focusable element like a span in the Dialog.

Since that PR we did a bunch of improvements on how the underlying
"stacking" system worked.
This PR reverts to the "global" window event listener so that we can
still catch all of the `escape` keydown events.

Fixes: #524
Fixes: #693

* update changelog
@RobinMalfait
Copy link
Member

RobinMalfait commented Aug 24, 2021

Hey! Thank you for your bug report!
Much appreciated! 🙏

This should be fixed, and will be available in the next release.

You can already try it using:

  • npm install @headlessui/react@dev or yarn add @headlessui/react@dev.
  • npm install @headlessui/vue@dev or yarn add @headlessui/vue@dev.

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

No branches or pull requests

6 participants