Skip to content

fix(Dialog): close on first Escape press when close button is focused#7915

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.

Sign up for GitHub

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

Merged
jonrohan merged 1 commit into from
Jun 2, 2026
Merged

fix(Dialog): close on first Escape press when close button is focused #7915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dialog-escape-close-button-tooltip.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Dialog: Fix `Escape` key not closing the dialog on the first keypress when the close button is focused
46 changes: 44 additions & 2 deletions packages/react/src/Dialog/Dialog.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -176,12 +176,54 @@ describe('Dialog', () => {

expect(onClose).not.toHaveBeenCalled()

await user.keyboard('{Escape}') // escape once to remove focus from the close button
await user.keyboard('{Escape}') // escape again to trigger the onClose
await user.keyboard('{Escape}')

expect(onClose).toHaveBeenCalledWith('escape')
})

it('calls `onClose` with a single "Escape" keypress when multiple dialogs can be opened', async () => {
const user = userEvent.setup()

function ButtonWithDialog({label, onClose}: {label: string; onClose: () => void}) {
const [isOpen, setIsOpen] = React.useState(false)
const buttonRef = React.useRef<HTMLButtonElement>(null)
return (
<>
<Button ref={buttonRef} onClick={() => setIsOpen(true)}>
{label}
</Button>
{isOpen && (
<Dialog
title={label}
onClose={() => {
onClose()
setIsOpen(false)
}}
returnFocusRef={buttonRef}
>
body
</Dialog>
)}
</>
)
}

const onCloseFirst = vi.fn()
const onCloseSecond = vi.fn()
const {getByText} = render(
<>
<ButtonWithDialog label="Dialog 1" onClose={onCloseFirst} />
<ButtonWithDialog label="Dialog 2" onClose={onCloseSecond} />
</>,
)

await user.click(getByText('Dialog 1'))
await user.keyboard('{Escape}')

expect(onCloseFirst).toHaveBeenCalled()
expect(onCloseSecond).not.toHaveBeenCalled()
})
Comment on lines +213 to +225

it('changes the <body> style for `overflow` if it is not set to "hidden"', () => {
document.body.style.overflow = 'scroll'

Expand Down
22 changes: 20 additions & 2 deletions packages/react/src/Dialog/Dialog.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -220,14 +220,28 @@ const DefaultHeader: React.FC<React.PropsWithChildren<DialogHeaderProps>> = ({
const onCloseClick = useCallback(() => {
onClose('close-button')
}, [onClose])
const onCloseKeyDown = useCallback<React.KeyboardEventHandler>(
event => {
if (event.key === 'Escape') {
// When the close button is focused its tooltip is open, and the
// tooltip's own Escape handler (registered on `document`) would
// otherwise swallow this keypress. Handle Escape here and stop it from
// reaching the document-level handler so the dialog closes on the first
// press while keeping the tooltip fully functional.
event.stopPropagation()
onClose('escape')
}
Comment on lines +231 to +233
},
[onClose],
)
return (
<Dialog.Header>
<div className={classes.HeaderInner}>
<div className={classes.HeaderContent}>
<Dialog.Title id={dialogLabelId}>{title ?? 'Dialog'}</Dialog.Title>
{subtitle && <Dialog.Subtitle id={dialogDescriptionId}>{subtitle}</Dialog.Subtitle>}
</div>
<Dialog.CloseButton onClose={onCloseClick} />
<Dialog.CloseButton onClose={onCloseClick} onKeyDown={onCloseKeyDown} />
</div>
</Dialog.Header>
)
Expand DownExpand Up@@ -506,12 +520,16 @@ const Buttons: React.FC<React.PropsWithChildren<{buttons: DialogButtonProps[]}>>
)
}

const CloseButton: React.FC<React.PropsWithChildren<{onClose: () => void}>> = ({onClose}) => {
const CloseButton: React.FC<React.PropsWithChildren<{onClose: () => void; onKeyDown?: React.KeyboardEventHandler}>> = ({
onClose,
onKeyDown,
}) => {
return (
<IconButton
icon={XIcon}
aria-label="Close"
onClick={onClose}
onKeyDown={onKeyDown}
variant="invisible"
data-component="Dialog.CloseButton"
/>
Expand Down
Loading