From daa8dbe67e7a96e961d9ee15c891803767e319c0 Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Mon, 1 Jun 2026 21:42:35 +0000 Subject: [PATCH] fix(Dialog): close on first Escape when close button is focused The close button's tooltip registered a document-level Escape handler that swallowed the first keypress. Intercept Escape on the close button and stop propagation so the dialog closes immediately while keeping the tooltip functional. --- .../dialog-escape-close-button-tooltip.md | 5 ++ packages/react/src/Dialog/Dialog.test.tsx | 46 ++++++++++++++++++- packages/react/src/Dialog/Dialog.tsx | 22 ++++++++- 3 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 .changeset/dialog-escape-close-button-tooltip.md diff --git a/.changeset/dialog-escape-close-button-tooltip.md b/.changeset/dialog-escape-close-button-tooltip.md new file mode 100644 index 00000000000..ba84ed77970 --- /dev/null +++ b/.changeset/dialog-escape-close-button-tooltip.md @@ -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 diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index b013a7399fc..0cdbac53645 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -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(null) + return ( + <> + + {isOpen && ( + { + onClose() + setIsOpen(false) + }} + returnFocusRef={buttonRef} + > + body + + )} + + ) + } + + const onCloseFirst = vi.fn() + const onCloseSecond = vi.fn() + const {getByText} = render( + <> + + + , + ) + + await user.click(getByText('Dialog 1')) + await user.keyboard('{Escape}') + + expect(onCloseFirst).toHaveBeenCalled() + expect(onCloseSecond).not.toHaveBeenCalled() + }) + it('changes the style for `overflow` if it is not set to "hidden"', () => { document.body.style.overflow = 'scroll' diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 7015367bff9..a7634ab1739 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -220,6 +220,20 @@ const DefaultHeader: React.FC> = ({ const onCloseClick = useCallback(() => { onClose('close-button') }, [onClose]) + const onCloseKeyDown = useCallback( + 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') + } + }, + [onClose], + ) return (
@@ -227,7 +241,7 @@ const DefaultHeader: React.FC> = ({ {title ?? 'Dialog'} {subtitle && {subtitle}}
- + ) @@ -506,12 +520,16 @@ const Buttons: React.FC> ) } -const CloseButton: React.FC void}>> = ({onClose}) => { +const CloseButton: React.FC void; onKeyDown?: React.KeyboardEventHandler}>> = ({ + onClose, + onKeyDown, +}) => { return (