ui/AlertDialog: better async confirm APIs, fully use base ui's AlertDialog#76937
Merged
Conversation
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
from
ae6126c to
4e79caa
Compare
|
Size Change: 0 B Total Size: 7.74 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in 6f9e590. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24130521432
|
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
3 times, most recently
from
04defc5 to
bed8d60
Compare
ciampo
changed the title
AlertDialog
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
from
7ded1d3 to
c6ff97a
Compare
ciampo
commented
Apr 2, 2026
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
5 times, most recently
from
7c25e6d to
43d04aa
Compare
|
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. |
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
from
43d04aa to
850a504
Compare
aduth
reviewed
Apr 2, 2026
Member
There was a problem hiding this comment.
I didn't quite get to review it fully before I had to wrap for the day, but generally I like the approach in using promises here as a common pattern for how someone would want to handle asynchronous actions, and letting the high-level component take care of all the minutia. 👍 I'll pick up on my review again tomorrow.
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
2 times, most recently
from
2c92543 to
de34ba9
Compare
aduth
approved these changes
Apr 3, 2026
Member
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
I feel like the implementation here is quite complex, though I appreciate what it's doing in letting AlertDialog absorb the complexity in service of developer ergonomics. And it's appropriately doing so at a higher level of abstraction.
Comment on lines
+256
to
+259
| setError( | ||
| 'Something went wrong. Please try again.' | ||
| ); | ||
| throw new Error( 'Task failed' ); |
Member
There was a problem hiding this comment.
I wonder about how a consumer has to both manage this state themselves and throw an error for the confirmation handler.
A thought: Since this is a high-level component, should it also be responsible for handling the error rendering? This also has some accessibility implications. For example, does this story actually effectively announce the error in any way to assistive technology? (I see role=alert but my understanding is that this doesn't work so well for dynamically-added content) Should the error message be linked with aria-describedby to the button? If these are things we'd expect, I'd also expect it'd be easy for a consumer to get them wrong. Now, on the other hand, I'm not sure exactly what it'd look like to handle this in the component. The thrown error's message is probably not a nice message to be showing to the user. Maybe one of these components could receive an errorMessage prop (string or callback), but that's a bit clunky too.
Contributor
Author
There was a problem hiding this comment.
That's a fair point and one feature that I had initially decided to explore as a follow-up. But following your comment, I decided to implement error handling directly in this PR.
- I tweaked
ConfirmResultto also accept anerrorstring prop alongside thecloseboolean prop; - when
erroris specified, the dialog will stay open and show that error next to the confirm button; - instead of
role="status", we usespeak()from@wordpress/a11y
Happy to iterate in follow-ups for any tweaks necessary to position and styling of the error message (cc @jameskoster )
Contributor
Author
There was a problem hiding this comment.
Should the error message be linked with aria-describedby to the button?
Specifically on this part, I opted for not making the error message part of the accessible description of the dialog, for a few reasons:
- the error already gets announced to an
aria-liveregion via thespeak()function, so we'd risk duplicating the announcement; - the error message is easily discoverable by sighted users and assistive technology, just after the confirm button;
aria-describedbyis only "read" when the dialog is either explicitly focused, or when the dialog is somehow re-announced, which I don't think would happen often;- we'd be having to work around base UI, since we're already using the
AlertDialog.Descriptionto pass an accessible description to the dialog
| render={ <p /> } | ||
| role="alert" | ||
| style={ { | ||
| color: 'var(--wpds-color-fg-content-error)', |
Member
There was a problem hiding this comment.
Is the color of red used here correct? It looks a bit dark.
This also makes me wonder again if this is the best API we expect people to use for customizing text color. Is this a standard way we'd expect people to show error texts? We had weighed a color prop before already but opted against it. Do you think an example like this is an example where a prop like that could be useful?
Contributor
Author
There was a problem hiding this comment.
Is the color of red used here correct? It looks a bit dark.
We currently have two fg colors for the error tone for non-interactive (ie. content) text:
We also have a separate fg color for the interactive text (resting state, active state):
Happy to give those colors a review together with @jameskoster when we find some time to pair on it.
This also makes me wonder again if this is the best API we expect people to use for customizing text color. Is this a standard way we'd expect people to show error texts? We had weighed a
colorprop before already but opted against it. Do you think an example like this is an example where a prop like that could be useful?
I understand that having a quick way to assign text color feels like a nice devx improvement, although IMO there are a few questions to answer first:
- Would this be something only for the
Textcomponent? If not, how do we design an API that scales well across the whole UI (and potentially beyond it)? For comparison, applying a CSS variable is a lower-level API than a direct prop, but is universal. - What colors would we allow? Any
--wpds-color-fg-contentvariables? Would we split the prop intotoneemphasisetc or would we keep it as one string prop? Again, apart from the TS validation/autocompletion (which partially overlaps with linters anyway), the API wouldn't look too different from using a CSS var directly?
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
from
de34ba9 to
49d586f
Compare
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
2 times, most recently
from
f631c4b to
fa5493b
Compare
…chine Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
ciampo
force-pushed
the
refactor/alert-dialog-use-base-ui
branch
from
fa5493b to
6f9e590
Compare
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?
Follow-up to #76847.
Refactors
AlertDialogto use Base UI'sAlertDialogprimitives directly (instead of wrappingDialog), and redesigns the component API for cleaner async workflows and better accessibility.Why?
For two reasons: extend the underlying base ui
AlertDialogmore consistently, and improve the UX and DevX for async confirm flows. The three main changes are:onConfirmnow accepts aPromise-returning callback. The component handles loading spinners, button disabling, and dismiss-blocking internally — no more manual state juggling.{ close: false, error: '...' }fromonConfirmto display an error message below the action buttons. The message is announced to screen readers viaspeak()and cleared automatically on the next confirm attempt or when the dialog reopens.descriptionprop for accessibility. Rendered via Base UI'sAlertDialog.Description, which wires uparia-describedbyautomatically.Dialogwrapper indirection by using Base UI'sAlertDialogprimitives directly.How?
Implementation notes
Internal state machine
Rootimplements a three-phase state machine (idle→pending→closing) to manage the dialog lifecycle:idle: default state, buttons are interactive.pending: entered whenonConfirmreturns a Promise. Buttons are disabled, spinner shown on the confirm button. Dismiss is blocked (Escape key and Cancel button are disabled).closing: entered when the dialog starts its exit animation. Buttons stay disabled to prevent interactions during the animation. Reset toidlevia Base UI'sonOpenChangeComplete.A safety-net
useEffectdetects the contradictory state of the dialog being open while phase is stuck atclosing(e.g. when a controlled consumer keepsopen={true}after confirm) and resets toidle.Spinner detection
The spinner is shown immediately when
onConfirmreturns a thenable (detected viaisThenable). Synchronous handlers skip the spinner entirely.Imperative close via
actionsRefWhen
onConfirmresolves successfully, the dialog is closed imperatively via Base UI'sactionsRef.current.close(). This generates realChangeEventDetailswith propercancel(),allowPropagation(), andpreventUnmountOnClose()methods — no synthetic event fabrication needed.onConfirmplacementonConfirmlives onRoot(the state machine owner).intentmoved toPopupThe
intentprop only affects the confirm button's styling, so it was moved fromRoottoPopupto eliminate unnecessary context forwarding.Error handling
Two error paths are supported:
{ close: false, error: '...' }fromonConfirm. The component renders the message below the action buttons, announces it to screen readers viaspeak(), and clears it on the next confirm attempt or when the dialog reopens.onConfirmthrows (or the promise rejects) without returning anerror, the dialog stays open and returns toidle. The error is logged toconsole.errorbut no visible message is shown — the component recovers gracefully.The recommended consumer pattern is to catch known errors and return
{ close: false, error }, letting unexpected errors fall through to the throw path (see theAsyncConfirmstory).API comparison: trunk vs this PR
AlertDialog.RootopenonOpenChangedefaultOpenintentPopuponConfirm{ close: false }, and{ error: '...' })AlertDialog.Popuptitlestring(required)string(required)childrenReactNode(required)ReactNode(optional)descriptionstring, wiresaria-describedby)intentRootonConfirmRootconfirmButtonTextstringstringcancelButtonTextstringstringloadinginitialFocusfinalFocusAsync confirm flow comparison
loadingstatePromisefromonConfirmloadingproploadingproponOpenChange{ error: '...' }→ built-in message +speak(). Throws → stays open,console.erroropen={false}{ close: false }to keep openTesting Instructions
npm run test:unit -- --testPathPattern="packages/ui/src/alert-dialog" --no-coverageAll 40 tests pass.
Storybook
npm run storybook:devTesting Instructions for Keyboard
Screen recording
Kapture.2026-04-02.at.13.32.58.mp4
Use of AI Tools
Cursor with Claude Opus 4.6