Skip to content

Commit b59c091

Browse files
RobinMalfaitRobinMalfait
authored
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
1 parent d60d2a5 commit b59c091

File tree

5 files changed

+212
-23
lines changed

5 files changed

+212
-23
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1010
### Fixes
1111

1212
- Only add `type=button` to real buttons ([#709](https://github.com/tailwindlabs/headlessui/pull/709))
13+
- Fix `escape` bug not closing Dialog after clicking in Dialog ([#754](https://github.com/tailwindlabs/headlessui/pull/754))
1314

1415
## [Unreleased - Vue]
1516

1617
### Fixes
1718

1819
- Only add `type=button` to real buttons ([#709](https://github.com/tailwindlabs/headlessui/pull/709))
1920
- Add Vue emit types ([#679](https://github.com/tailwindlabs/headlessui/pull/679), [#712](https://github.com/tailwindlabs/headlessui/pull/712))
21+
- Fix `escape` bug not closing Dialog after clicking in Dialog ([#754](https://github.com/tailwindlabs/headlessui/pull/754))
2022

2123
## [@headlessui/react@v1.4.0] - 2021-07-29
2224

‎packages/@headlessui-react/src/components/dialog/dialog.test.tsx‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,90 @@ describe('Keyboard interactions', () => {
430430
assertDialog({ state: DialogState.InvisibleUnmounted })
431431
})
432432
)
433+
434+
it(
435+
'should be possible to close the dialog with Escape, when a field is focused',
436+
suppressConsoleLogs(async () => {
437+
function Example() {
438+
let [isOpen, setIsOpen] = useState(false)
439+
return (
440+
<>
441+
<button id="trigger" onClick={() => setIsOpen(v => !v)}>
442+
Trigger
443+
</button>
444+
<Dialog open={isOpen} onClose={setIsOpen}>
445+
Contents
446+
<input id="name" />
447+
<TabSentinel />
448+
</Dialog>
449+
</>
450+
)
451+
}
452+
render(<Example />)
453+
454+
assertDialog({ state: DialogState.InvisibleUnmounted })
455+
456+
// Open dialog
457+
await click(document.getElementById('trigger'))
458+
459+
// Verify it is open
460+
assertDialog({
461+
state: DialogState.Visible,
462+
attributes: { id: 'headlessui-dialog-1' },
463+
})
464+
465+
// Close dialog
466+
await press(Keys.Escape)
467+
468+
// Verify it is close
469+
assertDialog({ state: DialogState.InvisibleUnmounted })
470+
})
471+
)
472+
473+
it(
474+
'should not be possible to close the dialog with Escape, when a field is focused but cancels the event',
475+
suppressConsoleLogs(async () => {
476+
function Example() {
477+
let [isOpen, setIsOpen] = useState(false)
478+
return (
479+
<>
480+
<button id="trigger" onClick={() => setIsOpen(v => !v)}>
481+
Trigger
482+
</button>
483+
<Dialog open={isOpen} onClose={setIsOpen}>
484+
Contents
485+
<input
486+
id="name"
487+
onKeyDown={event => {
488+
event.preventDefault()
489+
event.stopPropagation()
490+
}}
491+
/>
492+
<TabSentinel />
493+
</Dialog>
494+
</>
495+
)
496+
}
497+
render(<Example />)
498+
499+
assertDialog({ state: DialogState.InvisibleUnmounted })
500+
501+
// Open dialog
502+
await click(document.getElementById('trigger'))
503+
504+
// Verify it is open
505+
assertDialog({
506+
state: DialogState.Visible,
507+
attributes: { id: 'headlessui-dialog-1' },
508+
})
509+
510+
// Try to close the dialog
511+
await press(Keys.Escape)
512+
513+
// Verify it is still open
514+
assertDialog({ state: DialogState.Visible })
515+
})
516+
)
433517
})
434518
})
435519

‎packages/@headlessui-react/src/components/dialog/dialog.tsx‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,14 @@ import React, {
77
useMemo,
88
useReducer,
99
useRef,
10+
useState,
1011

1112
// Types
1213
ContextType,
1314
ElementType,
1415
MouseEvent as ReactMouseEvent,
15-
KeyboardEvent as ReactKeyboardEvent,
1616
MutableRefObject,
1717
Ref,
18-
useState,
1918
} from 'react'
2019

2120
import { Props } from '../../types'
@@ -217,6 +216,16 @@ let DialogRoot = forwardRefWithAs(function Dialog<
217216
close()
218217
})
219218

219+
// Handle `Escape` to close
220+
useWindowEvent('keydown', event => {
221+
if (event.key !== Keys.Escape) return
222+
if (dialogState !== DialogStates.Open) return
223+
if (hasNestedDialogs) return
224+
event.preventDefault()
225+
event.stopPropagation()
226+
close()
227+
})
228+
220229
// Scroll lock
221230
useEffect(() => {
222231
if (dialogState !== DialogStates.Open) return
@@ -282,16 +291,6 @@ let DialogRoot = forwardRefWithAs(function Dialog<
282291
onClick(event: ReactMouseEvent) {
283292
event.stopPropagation()
284293
},
285-
286-
// Handle `Escape` to close
287-
onKeyDown(event: ReactKeyboardEvent) {
288-
if (event.key !== Keys.Escape) return
289-
if (dialogState !== DialogStates.Open) return
290-
if (hasNestedDialogs) return
291-
event.preventDefault()
292-
event.stopPropagation()
293-
close()
294-
},
295294
}
296295
let passthroughProps = rest
297296

‎packages/@headlessui-vue/src/components/dialog/dialog.test.ts‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,111 @@ describe('Keyboard interactions', () => {
526526
assertDialog({ state: DialogState.InvisibleUnmounted })
527527
})
528528
)
529+
530+
it(
531+
'should be possible to close the dialog with Escape, when a field is focused',
532+
suppressConsoleLogs(async () => {
533+
renderTemplate({
534+
template: `
535+
<div>
536+
<button id="trigger" @click="toggleOpen">
537+
Trigger
538+
</button>
539+
<Dialog :open="isOpen" @close="setIsOpen">
540+
Contents
541+
<input id="name" />
542+
<TabSentinel />
543+
</Dialog>
544+
</div>
545+
`,
546+
setup() {
547+
let isOpen = ref(false)
548+
return {
549+
isOpen,
550+
setIsOpen(value: boolean) {
551+
isOpen.value = value
552+
},
553+
toggleOpen() {
554+
isOpen.value = !isOpen.value
555+
},
556+
}
557+
},
558+
})
559+
560+
assertDialog({ state: DialogState.InvisibleUnmounted })
561+
562+
// Open dialog
563+
await click(document.getElementById('trigger'))
564+
565+
// Verify it is open
566+
assertDialog({
567+
state: DialogState.Visible,
568+
attributes: { id: 'headlessui-dialog-1' },
569+
})
570+
571+
// Close dialog
572+
await press(Keys.Escape)
573+
574+
// Verify it is close
575+
assertDialog({ state: DialogState.InvisibleUnmounted })
576+
})
577+
)
578+
579+
it(
580+
'should not be possible to close the dialog with Escape, when a field is focused but cancels the event',
581+
suppressConsoleLogs(async () => {
582+
renderTemplate({
583+
template: `
584+
<div>
585+
<button id="trigger" @click="toggleOpen">
586+
Trigger
587+
</button>
588+
<Dialog :open="isOpen" @close="setIsOpen">
589+
Contents
590+
<input
591+
id="name"
592+
@keydown="cancel"
593+
/>
594+
<TabSentinel />
595+
</Dialog>
596+
</div>
597+
`,
598+
setup() {
599+
let isOpen = ref(false)
600+
return {
601+
isOpen,
602+
setIsOpen(value: boolean) {
603+
isOpen.value = value
604+
},
605+
toggleOpen() {
606+
isOpen.value = !isOpen.value
607+
},
608+
cancel(event: KeyboardEvent) {
609+
event.preventDefault()
610+
event.stopPropagation()
611+
},
612+
}
613+
},
614+
})
615+
616+
assertDialog({ state: DialogState.InvisibleUnmounted })
617+
618+
// Open dialog
619+
await click(document.getElementById('trigger'))
620+
621+
// Verify it is open
622+
assertDialog({
623+
state: DialogState.Visible,
624+
attributes: { id: 'headlessui-dialog-1' },
625+
})
626+
627+
// Try to close the dialog
628+
await press(Keys.Escape)
629+
630+
// Verify it is still open
631+
assertDialog({ state: DialogState.Visible })
632+
})
633+
)
529634
})
530635
})
531636

‎packages/@headlessui-vue/src/components/dialog/dialog.ts‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,6 @@ export let Dialog = defineComponent({
8787
'aria-labelledby': this.titleId,
8888
'aria-describedby': this.describedby,
8989
onClick: this.handleClick,
90-
onKeydown: this.handleKeyDown,
9190
}
9291
let { open: _, initialFocus, ...passThroughProps } = this.$props
9392

@@ -205,6 +204,16 @@ export let Dialog = defineComponent({
205204
nextTick(() => target?.focus())
206205
})
207206

207+
// Handle `Escape` to close
208+
useWindowEvent('keydown', event => {
209+
if (event.key !== Keys.Escape) return
210+
if (dialogState.value !== DialogStates.Open) return
211+
if (containers.value.size > 1) return // 1 is myself, otherwise other elements in the Stack
212+
event.preventDefault()
213+
event.stopPropagation()
214+
api.close()
215+
})
216+
208217
// Scroll lock
209218
watchEffect(onInvalidate => {
210219
if (dialogState.value !== DialogStates.Open) return
@@ -260,16 +269,6 @@ export let Dialog = defineComponent({
260269
handleClick(event: MouseEvent) {
261270
event.stopPropagation()
262271
},
263-
264-
// Handle `Escape` to close
265-
handleKeyDown(event: KeyboardEvent) {
266-
if (event.key !== Keys.Escape) return
267-
if (dialogState.value !== DialogStates.Open) return
268-
if (containers.value.size > 1) return // 1 is myself, otherwise other elements in the Stack
269-
event.preventDefault()
270-
event.stopPropagation()
271-
api.close()
272-
},
273272
}
274273
},
275274
})

0 commit comments

Comments
 (0)