From 4abe048e32e15d41cca32538edd1b572489852bf Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 24 Aug 2021 11:13:06 +0200 Subject: [PATCH] fix broken `escape` key behaviour We've "fixed" an issue when we had nested Dialogs ([#430](https://github.com/tailwindlabs/headlessui/pull/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 --- .../src/components/dialog/dialog.test.tsx | 84 ++++++++++++++ .../src/components/dialog/dialog.tsx | 23 ++-- .../src/components/dialog/dialog.test.ts | 105 ++++++++++++++++++ .../src/components/dialog/dialog.ts | 21 ++-- 4 files changed, 210 insertions(+), 23 deletions(-) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx index 6fa61270ef..fcdc88f0f2 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx @@ -430,6 +430,90 @@ describe('Keyboard interactions', () => { assertDialog({ state: DialogState.InvisibleUnmounted }) }) ) + + it( + 'should be possible to close the dialog with Escape, when a field is focused', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + return ( + <> + + + Contents + + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Close dialog + await press(Keys.Escape) + + // Verify it is close + assertDialog({ state: DialogState.InvisibleUnmounted }) + }) + ) + + it( + 'should not be possible to close the dialog with Escape, when a field is focused but cancels the event', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + return ( + <> + + + Contents + { + event.preventDefault() + event.stopPropagation() + }} + /> + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Try to close the dialog + await press(Keys.Escape) + + // Verify it is still open + assertDialog({ state: DialogState.Visible }) + }) + ) }) }) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx index 2055bd4ac6..839ca69087 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.tsx @@ -7,15 +7,14 @@ import React, { useMemo, useReducer, useRef, + useState, // Types ContextType, ElementType, MouseEvent as ReactMouseEvent, - KeyboardEvent as ReactKeyboardEvent, MutableRefObject, Ref, - useState, } from 'react' import { Props } from '../../types' @@ -217,6 +216,16 @@ let DialogRoot = forwardRefWithAs(function Dialog< close() }) + // Handle `Escape` to close + useWindowEvent('keydown', event => { + if (event.key !== Keys.Escape) return + if (dialogState !== DialogStates.Open) return + if (hasNestedDialogs) return + event.preventDefault() + event.stopPropagation() + close() + }) + // Scroll lock useEffect(() => { if (dialogState !== DialogStates.Open) return @@ -282,16 +291,6 @@ let DialogRoot = forwardRefWithAs(function Dialog< 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() - }, } let passthroughProps = rest diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts index e01d98ee9c..e706b1d5fa 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts @@ -526,6 +526,111 @@ describe('Keyboard interactions', () => { assertDialog({ state: DialogState.InvisibleUnmounted }) }) ) + + it( + 'should be possible to close the dialog with Escape, when a field is focused', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + Contents + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + toggleOpen() { + isOpen.value = !isOpen.value + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Close dialog + await press(Keys.Escape) + + // Verify it is close + assertDialog({ state: DialogState.InvisibleUnmounted }) + }) + ) + + it( + 'should not be possible to close the dialog with Escape, when a field is focused but cancels the event', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + Contents + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + toggleOpen() { + isOpen.value = !isOpen.value + }, + cancel(event: KeyboardEvent) { + event.preventDefault() + event.stopPropagation() + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Try to close the dialog + await press(Keys.Escape) + + // Verify it is still open + assertDialog({ state: DialogState.Visible }) + }) + ) }) }) diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.ts b/packages/@headlessui-vue/src/components/dialog/dialog.ts index 346e7deaa3..d2a7f84ad3 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.ts @@ -87,7 +87,6 @@ export let Dialog = defineComponent({ 'aria-labelledby': this.titleId, 'aria-describedby': this.describedby, onClick: this.handleClick, - onKeydown: this.handleKeyDown, } let { open: _, initialFocus, ...passThroughProps } = this.$props @@ -205,6 +204,16 @@ export let Dialog = defineComponent({ nextTick(() => target?.focus()) }) + // Handle `Escape` to close + useWindowEvent('keydown', event => { + if (event.key !== Keys.Escape) return + if (dialogState.value !== DialogStates.Open) return + if (containers.value.size > 1) return // 1 is myself, otherwise other elements in the Stack + event.preventDefault() + event.stopPropagation() + api.close() + }) + // Scroll lock watchEffect(onInvalidate => { if (dialogState.value !== DialogStates.Open) return @@ -260,16 +269,6 @@ export let Dialog = defineComponent({ handleClick(event: MouseEvent) { event.stopPropagation() }, - - // Handle `Escape` to close - handleKeyDown(event: KeyboardEvent) { - if (event.key !== Keys.Escape) return - if (dialogState.value !== DialogStates.Open) return - if (containers.value.size > 1) return // 1 is myself, otherwise other elements in the Stack - event.preventDefault() - event.stopPropagation() - api.close() - }, } }, })