fix(Dialog): close on first Escape press when close button is focused#7915
Conversation
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.
🦋 Changeset detectedLatest commit: daa8dbe The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
staff
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where Dialog required two Escape key presses to close when the close button is focused, due to the close button tooltip’s document-level Escape handler preventing the dialog’s own Escape handler from running.
Changes:
- Add an
onKeyDownhandler toDialog.CloseButtonto interceptEscape, stop propagation, and close the dialog immediately. - Update Dialog Escape tests and add a regression test for the multiple-dialogs scenario.
- Add a patch changeset documenting the behavior change.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Dialog/Dialog.tsx | Intercepts Escape on the close button to avoid the tooltip swallowing the first keypress and closes the dialog immediately. |
| packages/react/src/Dialog/Dialog.test.tsx | Updates Escape-close expectations and adds a multi-dialog regression test. |
| .changeset/dialog-escape-close-button-tooltip.md | Patch changeset describing the fixed Escape-close behavior. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 2
| event.stopPropagation() | ||
| onClose('escape') | ||
| } |
| 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() | ||
| }) |
jonrohan
added
the
Canary Release
|
Integration test results from github/github-ui PR:
All checks passed! |
llastflowers
left a comment
llastflowers
left a comment
There was a problem hiding this comment.
Tested in Storybook, looks great! ✨
Closes https://github.com/github/primer/issues/2604
When a
Dialogopens, focus moves to its close button. Because icon buttons now render a tooltip by default, the tooltip's ownEscapehandler (registered ondocumentviauseOnEscapePress) runs first and callsstopImmediatePropagation()+preventDefault()to dismiss the tooltip — swallowing the firstEscapekeypress so the dialog doesn't close. Users had to pressEscapetwice (the previous test even worked around this), and in a multi-dialog setup the firstEscapecould shift focus to the wrong return-focus target instead of closing the dialog.Fix
Intercept the
Escapekeydown on the close button itself. React dispatchesonKeyDownat the root container, which sits belowdocumentin the DOM, so callingstopPropagation()there prevents the event from reaching the tooltip's document-level handler. We close the dialog directly with the'escape'gesture, and the tooltip stays fully functional.Considered but rejected: disabling the tooltip
The simplest fix was to set
unsafeDisableTooltipon the close button. We chose not to do this because the tooltip contributes to the button's discoverability/affordance and we didn't want to regress the accessibility/UX of the close button just to work around the event handling. Intercepting the event keeps the tooltip — and its behavior — intact while fixing the bug.Changelog
Changed
Dialognow closes on the firstEscapekeypress when the close button is focused, instead of requiring a second press.Rollout strategy
Testing & Reviewing
Dialog(each with areturnFocusRef).Escapeonce.Added a regression test in
Dialog.test.tsxcovering the single-Escapeclose and the multi-dialog scenario, and updated the existing Escape test to expect a single keypress. All 40 Dialog tests pass; type-check is clean.Merge checklist