Fix broken escape key behaviour (#754)

* 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
tailwindlabs · Aug 24, 2021 · b59c091 · b59c091
1 parent d60d2a5
commit b59c091
Show file tree

Hide file tree

Showing 5 changed files with 212 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,13 +10,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixes
 
 - Only add `type=button` to real buttons ([#709](https://github.com/tailwindlabs/headlessui/pull/709))
+- Fix `escape` bug not closing Dialog after clicking in Dialog ([#754](https://github.com/tailwindlabs/headlessui/pull/754))
 
 ## [Unreleased - Vue]
 
 ### Fixes
 
 - Only add `type=button` to real buttons ([#709](https://github.com/tailwindlabs/headlessui/pull/709))
 - Add Vue emit types ([#679](https://github.com/tailwindlabs/headlessui/pull/679), [#712](https://github.com/tailwindlabs/headlessui/pull/712))
+- Fix `escape` bug not closing Dialog after clicking in Dialog ([#754](https://github.com/tailwindlabs/headlessui/pull/754))
 
 ## [@headlessui/react@v1.4.0] - 2021-07-29
 

diff --git 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 (
+            <>
+              <button id="trigger" onClick={() => setIsOpen(v => !v)}>
+                Trigger
+              </button>
+              <Dialog open={isOpen} onClose={setIsOpen}>
+                Contents
+                <input id="name" />
+                <TabSentinel />
+              </Dialog>
+            </>
+          )
+        }
+        render(<Example />)
+
+        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 (
+            <>
+              <button id="trigger" onClick={() => setIsOpen(v => !v)}>
+                Trigger
+              </button>
+              <Dialog open={isOpen} onClose={setIsOpen}>
+                Contents
+                <input
+                  id="name"
+                  onKeyDown={event => {
+                    event.preventDefault()
+                    event.stopPropagation()
+                  }}
+                />
+                <TabSentinel />
+              </Dialog>
+            </>
+          )
+        }
+        render(<Example />)
+
+        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
@@ -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
@@ -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: `
+            <div>
+              <button id="trigger" @click="toggleOpen">
+                Trigger
+              </button>
+              <Dialog :open="isOpen" @close="setIsOpen">
+                Contents
+                <input id="name" />
+                <TabSentinel />
+              </Dialog>
+            </div>
+          `,
+          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: `
+            <div>
+              <button id="trigger" @click="toggleOpen">
+                Trigger
+              </button>
+              <Dialog :open="isOpen" @close="setIsOpen">
+                Contents
+                <input
+                  id="name"
+                  @keydown="cancel"
+                />
+                <TabSentinel />
+              </Dialog>
+            </div>
+          `,
+          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
@@ -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()
-      },
     }
   },
 })