fix(dialog): fire onCancel when AlertDialog is dismissed via Escape key#10307
fix(dialog): fire onCancel when AlertDialog is dismissed via Escape key#10307arham766 wants to merge 2 commits into
Conversation
The Escape key is handled by the overlay, which closes the dialog directly and bypasses the cancel button, so onCancel was never called. Provide an onKeyDown handler on the dialog element so onCancel is also fired when the Escape key dismisses the dialog, in both the v3 and S2 AlertDialogs. Closes adobe#1773
| ...dialogContext, | ||
| onKeyDown: chain(dialogContext.onKeyDown, (e: React.KeyboardEvent) => { | ||
| if (e.key === 'Escape' && !e.nativeEvent.isComposing) { | ||
| onCancel(); |
There was a problem hiding this comment.
| onCancel(); | |
| onCancel(); | |
| e.stopPropagation(); | |
| e.preventDefault(); |
There was a problem hiding this comment.
Applied — with the consequence that consuming the event means the overlay no longer closes the dialog, so the handler now runs the full cancel-button path (onClose() then onCancel()). Escape is now literally equivalent to pressing the cancel button, which I think is the right semantic anyway.
One behavior change to rule on: with isKeyboardDismissDisabled on the trigger, Escape previously kept the dialog open (while leaking onCancel — flagged in the PR description); now it closes it, because the flag isn't visible from DialogContext. Arguably isKeyboardDismissDisabled + AlertDialog is contradictory once Escape ≡ cancel, but happy to thread the flag through the context instead if you'd rather preserve it.
| // RAC Dialog filters out keyboard events, so attach onKeyDown to the dialog | ||
| // element with a custom render function. This allows components like AlertDialog | ||
| // to respond to the Escape key dismissing the dialog. | ||
| render={ |
There was a problem hiding this comment.
I think it's probably better to update RACDialog->useDialog to accept onKeyDown?
This wasn't in the original PR, we didn't have the render prop back then, how'd you arrive at this solution?
There was a problem hiding this comment.
Done — useDialog now accepts onKeyDown (doc-commented on AriaDialogProps, so the docs table picks it up), RAC Dialog inherits it with zero code change since keyboard events aren't in GlobalDOMEvents, and the S2 render function is deleted; onKeyDown just flows through props. Added a RAC-level test that the handler reaches the section.
How I got to render: I traced why props/context didn't work — RAC filters with filterDOMProps(props, {global: true}) and GlobalDOMEvents deliberately excludes keyboard events — and reached for the narrowest hook that didn't touch public types. First-class support is clearly better now that you've blessed the API addition.
One S2 detail: in a DialogContainer (no trigger state at AlertDialog's position in the tree), the handler falls back to not consuming the event so the container's overlay still closes the dialog; onCancel fires exactly once either way.
| // is also called when the Escape key dismisses the dialog. | ||
| let context: DialogContextValue = { | ||
| ...dialogContext, | ||
| onKeyDown: chain(dialogContext.onKeyDown, (e: React.KeyboardEvent) => { |
There was a problem hiding this comment.
Would this be easier if onClose was expanded to give the reason?
#2192
There was a problem hiding this comment.
Yes — a close reason on onClose would make this whole dance trivial (map escape/click-outside → cancel at the AlertDialog level) and would also solve the isKeyboardDismissDisabled visibility problem cleanly, since the overlay would stay the single owner of dismissal. That's a wider API change across DialogContext/overlay state though, so I kept this PR targeted. I'd be glad to take #2192 on as a follow-up if you want it.
Closes #1773
Re-implements the approach from #10136 (which was approved but closed over an unsigned CLA — CLA is signed on my end), and covers the Spectrum 2 AlertDialog as requested in that review.
Problem
Pressing Escape closes an AlertDialog via the overlay (
state.close()directly), so theonCancelcallback — wired only to the cancel button'sonPress— never fires. Consumers can't distinguish a keyboard dismissal from a confirmation.Fix
V3 (
@adobe/react-spectrum):AlertDialogprovides aDialogContextwith an addedonKeyDownthat callsonCancelon Escape (chained with any existing context handler, guarded against IME composition like the overlay's own keyboard handling).Dialogmerges the context'sonKeyDownintodialogPropsafteruseDialog— required becauseuseDialog/filterDOMPropsstrips event handlers, so the handler can't ride through the existingmergeProps(contextProps, props)path. The overlay still closes the dialog exactly as before; nothing is prevented or stopped.S2 (
@react-spectrum/s2): keyboard events can't reach the RAC Dialog's section through props or context (filterDOMProps(props, {global: true})deliberately excludes them), so S2Dialogaccepts a/** @private */ onKeyDown(same precedent asisStickyon S2 TableView'sCellProps) and attaches it via RAC Dialog's publicrenderprop —sectionPropscarries ref/role/aria through, so it's a plain React handler on the real<section role="alertdialog">. Therenderpath is only used when the handler is present. Nested-overlay Escapes stop propagation in react-aria, so they won't misfireonCancel.One behavior note for review: if a V3
DialogTriggerhasisKeyboardDismissDisabled, Escape won't close the dialog butonCancelstill fires (the flag isn't visible throughDialogContext). #10136 had the same behavior. Happy to thread the flag through the context if you'd rather suppress it. (Not reachable in S2 — AlertDialog doesn't expose the prop.)✅ Pull Request Checklist:
📝 Test Instructions:
onCancelhandler (e.g. the AlertDialog story, or the S2 stackblitz from onCancel should fire in AlertDialog if Escape is pressed #1773 (comment)) and press Escape —onCancelfires once and the dialog closes.onCancelstill fires exactly once (no double-fire).onCancel— Escape still closes the dialog with no errors.Unit tests: 4 new cases in
packages/@adobe/react-spectrum/test/dialog/AlertDialog.test.js, 3 inpackages/@react-spectrum/s2/test/AlertDialog.test.tsx(Escape fires once, no-onCancel safety, button no-double-fire, and trigger-integration close). The Escape tests fail without the source change.🧢 Your Project:
Personal contribution