useDialog: handle Escape via React onKeyDown so cascade works through portals#78433
Merged
Conversation
|
Size Change: +7 B (0%) Total Size: 7.97 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in e7475e3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/26125403997
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
mirka
approved these changes
May 19, 2026
Member
mirka
left a comment
There was a problem hiding this comment.
Works well, aside from the potential prop-merging issues.
ciampo
added a commit
that referenced
this pull request
May 19, 2026
…ropped Now that `useDialog` returns an `onKeyDown` handler on its `props` object, consumers (like `Popover`) that already spread an `onKeyDown` from their own props onto the dialog wrapper would silently lose one of the two handlers depending on prop ordering. Take a clean approach instead: have `useDialog` accept an optional `onKeyDown` option, run it before the built-in close-on-Escape, and let it call `event.preventDefault()` to opt out of closing. Forward the Popover wrapper's user-provided `onKeyDown` through this option so both handlers are guaranteed to run. Addresses #78433 (comment).
mirka
approved these changes
May 19, 2026
Comment on lines
+50
to
+58
| /** | ||
| * Optional consumer-provided `onKeyDown` handler. It runs before the | ||
| * built-in close-on-Escape behavior so the consumer can call | ||
| * `event.preventDefault()` to opt out of closing for a given key event. | ||
| * | ||
| * Provided as an option (rather than letting consumers attach their own | ||
| * `onKeyDown` to the dialog wrapper) so consumers don't have to manually | ||
| * merge their handler with the one returned by `useDialog`. | ||
| */ |
Member
There was a problem hiding this comment.
Seems too verbose and internal-facing for a public-facing description? I think all that consumers need to know is that their handler will be merged with the internal handler, as long as it just "works" (which it will).
Also the readme seems to be missing documentation for all the parameters… maybe fine because it's experimental, and possibly will never be recommended for use after @wordpress/ui.
Contributor
Author
There was a problem hiding this comment.
Comment updated as suggested.
Also updated the README while I was at it
eaeffde to
b5ae576
Compare
… portals useDialog attached its Escape handler as a native `keydown` listener on the dialog node, which ran while the native event bubbled — before React dispatched synthetic events through the React tree. Portaled descendants that handled Escape via React `onKeyDown` (e.g. base-ui's `useDismiss` inside a `@wordpress/components` Popover) could never prevent the dialog from closing, because their `event.stopPropagation()` came too late. Replace the ref-attached native listener with a React `onKeyDown` returned alongside `focusOutsideProps`. This mirrors how Modal already handles Escape and makes the cascade work for any React `onKeyDown` descendant. Other clean-ups bundled in: - Use `event.key === 'Escape'` instead of the deprecated `event.keyCode`. - Remove the leaking `addEventListener` (the ref callback registered a fresh listener on every mount without removing it on unmount). Fixes #78432.
…ropped Now that `useDialog` returns an `onKeyDown` handler on its `props` object, consumers (like `Popover`) that already spread an `onKeyDown` from their own props onto the dialog wrapper would silently lose one of the two handlers depending on prop ordering. Take a clean approach instead: have `useDialog` accept an optional `onKeyDown` option, run it before the built-in close-on-Escape, and let it call `event.preventDefault()` to opt out of closing. Forward the Popover wrapper's user-provided `onKeyDown` through this option so both handlers are guaranteed to run. Addresses #78433 (comment).
da87014 to
e7475e3
Compare
peterwilsoncc
pushed a commit
to peterwilsoncc/gutenberg-build
that referenced
this pull request
May 20, 2026
…gh portals (#78433) * useDialog: handle Escape via React onKeyDown so cascade works through portals useDialog attached its Escape handler as a native `keydown` listener on the dialog node, which ran while the native event bubbled — before React dispatched synthetic events through the React tree. Portaled descendants that handled Escape via React `onKeyDown` (e.g. base-ui's `useDismiss` inside a `@wordpress/components` Popover) could never prevent the dialog from closing, because their `event.stopPropagation()` came too late. Replace the ref-attached native listener with a React `onKeyDown` returned alongside `focusOutsideProps`. This mirrors how Modal already handles Escape and makes the cascade work for any React `onKeyDown` descendant. Other clean-ups bundled in: - Use `event.key === 'Escape'` instead of the deprecated `event.keyCode`. - Remove the leaking `addEventListener` (the ref callback registered a fresh listener on every mount without removing it on unmount). Fixes #78432. * useDialog: Add PR number to CHANGELOG entry * useDialog: accept an `onKeyDown` option so consumer handlers aren't dropped Now that `useDialog` returns an `onKeyDown` handler on its `props` object, consumers (like `Popover`) that already spread an `onKeyDown` from their own props onto the dialog wrapper would silently lose one of the two handlers depending on prop ordering. Take a clean approach instead: have `useDialog` accept an optional `onKeyDown` option, run it before the built-in close-on-Escape, and let it call `event.preventDefault()` to opt out of closing. Forward the Popover wrapper's user-provided `onKeyDown` through this option so both handlers are guaranteed to run. Addresses WordPress/gutenberg#78433 (comment). * useDialog: tighten CHANGELOG (split breaking change, shorten fix entry) * useDialog: tighten onKeyDown JSDoc to be consumer-facing * useDialog: flesh out README with all options --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Source: WordPress/gutenberg@200f6f8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Fixes #78432.
useDialog(used byPopover,EntitiesSavedStates, the dataviews panel dropdown, …) attached its Escape handler as a nativekeydownlistener on the dialog node, which ran before React dispatched synthetic events through the React tree. Portaled descendants that handled Escape via ReactonKeyDown(e.g. base-ui'suseDismissinside a@wordpress/componentsPopover) could never prevent the dialog from closing.Why?
A
@wordpress/uiAutocompleteinside a@wordpress/componentsPopovercloses the hostPopoveron Escape regardless of Autocomplete state. With the sameAutocompleteinside a@wordpress/componentsModal(which handles Escape via a ReactonKeyDownprop), the cascade works correctly: first Escape closes the listbox; only a subsequent Escape closes the host.How?
Replace the ref-attached native listener with a React
onKeyDownreturned alongsidefocusOutsideProps, mirroring howModalalready handles Escape.Because that handler is now part of the returned
propsobject,useDialogalso accepts an optionalonKeyDownoption that runs before the built-in close-on-Escape (and can callevent.preventDefault()to opt out).Popoverforwards its user-providedonKeyDownthrough this option so a consumer's<Popover onKeyDown={…}>keeps working without manual prop merging.Also bundled in (small clean-ups inside the same callback)
event.key === 'Escape'instead of the deprecatedevent.keyCode.addEventListener(the previous ref callback registered a fresh listener on every mount without removing it on unmount).Backward compatibility / potential breaking change
Existing public behavior is preserved: the existing unit tests continue to pass after being rewritten to use
userEventfor realistic key simulation, plus two new tests covering the consumer-onKeyDownmerging.All three existing in-tree
useDialogconsumers —Popover,EntitiesSavedStates, the dataviews panel dropdown — keep working without consumer-side changes (onlyPopoveractually accepts an externalonKeyDownand has been updated to forward it).Out-of-tree consumers that spread
useDialog's returned props onto a wrapper that also has its ownonKeyDownfrom elsewhere should either pass that handler touseDialogvia the newonKeyDownoption or merge the two themselves. Flagged in the CHANGELOG.Testing Instructions
npm run storybook:devPlayground/WP Compat Overlay Slot→Inside Components Popover.Autocompleteinput and type a few characters to open its listbox.Popovershould stay open.Popovershould close.Before this PR: any Escape press closes the host
Popoverimmediately, regardless ofAutocompletestate.Unit tests:
Testing Instructions for Keyboard
Same as above — this is keyboard-only behavior.
Screenshots or screencast
N/A — behavior change only.
Use of AI Tools
Drafted with AI assistance; reviewed and verified manually (storybook reproduction + new unit tests covering the portaled-descendant cascade and the consumer-
onKeyDownmerge).