Conversation
dc7aef3 to
e746200
Compare
|
Size Change: -5 B (0%) Total Size: 7.73 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I realized that when our Button is disabled while focusable, clicking it still triggers the :active pseudo-state, which causes the focus ring to become transparent, which could give the false impression that the button is interactive.
If we agree with this change, I can move it to a separate PR (and investigate whether the same fix should be applied to @wordpress/components)
There was a problem hiding this comment.
I think the change is consistent with how we're currently using these styles (i.e. to indicate an active interaction). Although... I'm not really sure I fully understand why we do that in the first place?
There was a problem hiding this comment.
Removing the focus ring when the button is :active (ie. it's being pressed down during a click) is an additional visual confirmation (especially for keyboard users) that the "click" happened.
With the recent async worklow-related fixes, it actually looks like we don't need the fix anymore, since the :active state is (correcly) not triggered anymore while the Button is loading.
|
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. |
|
Flaky tests detected in 3256925. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23656209594
|
|
Regarding whether Escape should be blocked in the irreversible intent: I wonder if it could be reasonably inferred the "Escape" should be equated as "Cancel". My understanding of blocking something like a click on the backdrop is that it's easy to do that accidentally and not reasonably associated with confirming or canceling. Edit: I also see this behavior described in the WAI Alert Dialog pattern example (emphasis mine)
|
aduth
left a comment
There was a problem hiding this comment.
It's looking good 👍 I think the main things for me at this point are figuring out how we want to proceed on handling Escape, and making sure the async behavior is working correctly.
There was a problem hiding this comment.
I think the change is consistent with how we're currently using these styles (i.e. to indicate an active interaction). Although... I'm not really sure I fully understand why we do that in the first place?
e27765c to
ca56e56
Compare
|
@aduth all feedback should have been addressed:
|
aduth
left a comment
There was a problem hiding this comment.
Couple minor comments, but LGTM overall 👍
| onConfirm={ () => { | ||
| setIsLoading( true ); | ||
| new Promise< void >( ( resolve ) => | ||
| setTimeout( resolve, 2000 ) | ||
| ).then( () => { | ||
| setIsLoading( false ); | ||
| setIsOpen( false ); | ||
| } ); | ||
| } } |
There was a problem hiding this comment.
For the story example, we never call args.onOpenChange when confirming. Not sure it really matters except for completeness or if someone were following this example in a component abstraction of an async confirmation dialog that exposes an onOpenChange. Maybe we'd call it somewhere in this promise flow? Or as a useEffect after initial mount with open as its dependency?
There was a problem hiding this comment.
onOpenChange should fire for dialog-initiated closes (Escape, backdrop click, cancel button), and is not expected to fire when confirming since the consumer (ie. the storybook example) is controlling the open state (setIsOpen(false)).
Calling onOpenChange in the promise (or in a useEffect) would, IMO, set the wrong expectation about how onOpenChange is expected to work.
I did add an additional log of the onConfirm callback, though
| return ( | ||
| <Dialog.Popup | ||
| ref={ ref } | ||
| role={ intent === 'irreversible' ? 'alertdialog' : 'dialog' } |
There was a problem hiding this comment.
Trying to follow the spec and related examples, I don't fully understand why we wouldn't just want to apply alertdialog statically for all cases? I gather we may be associating something about "urgency" or how much it "requires a response", though all of these dialogs expect a response. And nothing I can see in the alertdialog examples or spec say anything about severity of an action or click-outside behaviors.
There was a problem hiding this comment.
I guess we started with the legacy ConfirmDialog from @wordpress/components in mind, and we saw ConfirmDialog as a convenient higher-level version of Dialog.
Then we started adding the differentiation for the irreversible variant:
- we made it harder to dismiss, hence the logic around the
Escapekey (recently reverted) and the pointer clicks on the backrdop (still applied) - we made it an official
alertdialog, while keeping the default variant a simpledialog
I remember also explicitly discussing these differences as the reason for having a custom implementation on top of our Dialog, rather than building on top of base ui's AlertDialog.
If we're ok to always (regardless of the variant):
- assign the
alertdialogrole; - allow the Escape key dismissal;
- disallow the click outside dismissal;
then we should just rename our component AlertDialog and build it on top of base ui's AlertDialog — which I'd be totally fine with.
@aduth should I go ahead with the refactor?
There was a problem hiding this comment.
That's a bit more extensive rethink than I expected to prompt with my question 😅 Though it's definitely an interesting proposition. I guess the main deviation there is standardizing the behavior of clicking outside. But the alignment to BaseUI is appealing.
I'm not sure I fully understand the relation between AlertDialog and Dialog in BaseUI, how much it builds on each other, how much that leaves for us to reimplement (i.e. how big a refactor).
If it's a big refactor, we could optionally do a few steps to move in that direction, like renaming ConfirmDialog to AlertDialog and/or always assigning the role.
Personally, I think I'd be okay with this refactor, including the implications on disallowing click outside dismissal regardless of intent. But it could be good to get some more buy-in from other folks as well.
There was a problem hiding this comment.
I tried the refactor (can be always undone), it was quite simple and didn't involve any public API changes (apart from the component name): 2d37d04
The ConfirmDialog was authored against an older Dialog version that had
a `title` prop on `Dialog.Root` and a `Dialog.Heading` subcomponent.
The current Dialog uses `Dialog.Title` (with children) and has no
`title` prop on Root.
- Thread `title` through `ConfirmDialogContext` instead of passing it
to `Dialog.Root`.
- Replace `<Dialog.Heading />` with `<Dialog.Title>{ title }</Dialog.Title>`.
- Define `title` directly on `RootProps` instead of picking it from
`DialogRootProps`.
- Fix invalid `wpds` i18n text domain.
Made-with: Cursor
- Swap runtime `react` imports for `@wordpress/element`. - Sort imports (externals, then @wordpress/*, then internal). - Use `import type` for type-only imports. - Move Storybook file to `stories/index.story.tsx`. - Update story title to `Design System/Components/ConfirmDialog`. - Fix `waitFor` blocks to use single assertions (lint rule). - Add JSDoc to Trigger component. - Export `ConfirmDialog` namespace from the package index. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
- Add `role="alertdialog"` to the popup for `irreversible` intent, `role="dialog"` for `default`. - Block Escape key dismissal for `irreversible` intent (previously only backdrop click was blocked). - Block backdrop click for `default` intent too — confirm dialogs should always require an explicit action. - Extract `getIntentConfig()` helper in context.tsx to centralize intent-dependent behavior (role, blocked dismiss reasons). - Update JSDoc and story descriptions to reflect the new behavior. Made-with: Cursor
Add a `loading` prop to `ConfirmDialog.Popup` that shows a spinner on the confirm button and disables both buttons. All async flow logic (controlled open state, preventing close during loading, closing on completion) is handled by the consumer. Pass `disabled` alongside `loading` to `Dialog.Action` to align `_Dialog.Close`'s internal `useButton` with the desired disabled state. Made-with: Cursor
Made-with: Cursor
The title is only consumed by Popup (to render Dialog.Title), so it belongs there as a direct prop rather than traversing through context. This simplifies the data flow and removes title from ConfirmDialogContext. Made-with: Cursor
The default (non-irreversible) variant should allow dismissing via backdrop click, matching the original behavior. Only the irreversible intent blocks all implicit dismissals. Made-with: Cursor
Dialog.Action wraps Base UI's Dialog.Close, which defaults `disabled` to `false` internally. This caused its `useButton` to set `aria-disabled="false"`, overriding Button's own `aria-disabled` derived from the `loading` prop. Extract `disabled` and `loading`, resolve them the same way Button does (`disabled ?? loading`), and pass the result to Dialog.Close. Forward `loading` directly to Button for the spinner CSS class. Made-with: Cursor
Now that Dialog.Action properly resolves disabled from loading,
the explicit `disabled={loading || undefined}` workaround and its
explanatory comment are no longer needed.
Made-with: Cursor
Expand the `loading` JSDoc to explicitly state that passing the prop (even as `false`) opts into manual-close mode. Add a regression test encoding this contract so future contributors don't accidentally change the `loading !== undefined` check. Made-with: Cursor
943524a to
2d5026b
Compare
aduth
left a comment
There was a problem hiding this comment.
LGTM 👍 I think I like the AlertDialog name a lot more. Not only in how it aligns to BaseUI, but also in the alertdialog role semantics.
* initlal version
* ConfirmDialog: Fix Dialog API incompatibilities
The ConfirmDialog was authored against an older Dialog version that had
a `title` prop on `Dialog.Root` and a `Dialog.Heading` subcomponent.
The current Dialog uses `Dialog.Title` (with children) and has no
`title` prop on Root.
- Thread `title` through `ConfirmDialogContext` instead of passing it
to `Dialog.Root`.
- Replace `<Dialog.Heading />` with `<Dialog.Title>{ title }</Dialog.Title>`.
- Define `title` directly on `RootProps` instead of picking it from
`DialogRootProps`.
- Fix invalid `wpds` i18n text domain.
Made-with: Cursor
* ConfirmDialog: Adapt to @wordpress/ui package conventions
- Swap runtime `react` imports for `@wordpress/element`.
- Sort imports (externals, then @wordpress/*, then internal).
- Use `import type` for type-only imports.
- Move Storybook file to `stories/index.story.tsx`.
- Update story title to `Design System/Components/ConfirmDialog`.
- Fix `waitFor` blocks to use single assertions (lint rule).
- Add JSDoc to Trigger component.
- Export `ConfirmDialog` namespace from the package index.
Made-with: Cursor
* ConfirmDialog: Add CHANGELOG entry
Made-with: Cursor
* Dialog: Remove ConfirmDialog story now covered by dedicated component
Made-with: Cursor
* ConfirmDialog: Tighten irreversible intent behavior
- Add `role="alertdialog"` to the popup for `irreversible` intent,
`role="dialog"` for `default`.
- Block Escape key dismissal for `irreversible` intent (previously
only backdrop click was blocked).
- Block backdrop click for `default` intent too — confirm dialogs
should always require an explicit action.
- Extract `getIntentConfig()` helper in context.tsx to centralize
intent-dependent behavior (role, blocked dismiss reasons).
- Update JSDoc and story descriptions to reflect the new behavior.
Made-with: Cursor
* ConfirmDialog: Add `loading` prop for async confirm flows
Add a `loading` prop to `ConfirmDialog.Popup` that shows a spinner on
the confirm button and disables both buttons. All async flow logic
(controlled open state, preventing close during loading, closing on
completion) is handled by the consumer.
Pass `disabled` alongside `loading` to `Dialog.Action` to align
`_Dialog.Close`'s internal `useButton` with the desired disabled state.
Made-with: Cursor
* ConfirmDialog: Add CHANGELOG entries for enhancements
Made-with: Cursor
* ConfirmDialog: Move title prop from Root to Popup
The title is only consumed by Popup (to render Dialog.Title), so it
belongs there as a direct prop rather than traversing through context.
This simplifies the data flow and removes title from
ConfirmDialogContext.
Made-with: Cursor
* format css
* ConfirmDialog: Restore backdrop click dismissal for default intent
The default (non-irreversible) variant should allow dismissing via
backdrop click, matching the original behavior. Only the irreversible
intent blocks all implicit dismissals.
Made-with: Cursor
* Dialog: Fix Action disabled/loading forwarding to Button
Dialog.Action wraps Base UI's Dialog.Close, which defaults
`disabled` to `false` internally. This caused its `useButton` to
set `aria-disabled="false"`, overriding Button's own `aria-disabled`
derived from the `loading` prop.
Extract `disabled` and `loading`, resolve them the same way Button
does (`disabled ?? loading`), and pass the result to Dialog.Close.
Forward `loading` directly to Button for the spinner CSS class.
Made-with: Cursor
* ConfirmDialog: Remove disabled workaround on confirm action
Now that Dialog.Action properly resolves disabled from loading,
the explicit `disabled={loading || undefined}` workaround and its
explanatory comment are no longer needed.
Made-with: Cursor
* Remove superflous CHANGELOG entries
* Update CHANGELOG PR reference
* Allow Escape key to dismiss the irreversible variant, cleanup getIntentConfig
* Remove hardcoded domain from Storybook docs URL
* CHANGELOG
* Try to fix async confirm workflow
* Better approach to async confirm work
* Add async flow unit test
* ConfirmDialog: Clarify loading prop manual-close contract
Expand the `loading` JSDoc to explicitly state that passing the prop
(even as `false`) opts into manual-close mode. Add a regression test
encoding this contract so future contributors don't accidentally
change the `loading !== undefined` check.
Made-with: Cursor
* Log "onConfirm" action
* Refactor to use base ui's AlertDialog, rename to AlertDialog
* format
Yay, I like this too 🎉 |
* initlal version
* ConfirmDialog: Fix Dialog API incompatibilities
The ConfirmDialog was authored against an older Dialog version that had
a `title` prop on `Dialog.Root` and a `Dialog.Heading` subcomponent.
The current Dialog uses `Dialog.Title` (with children) and has no
`title` prop on Root.
- Thread `title` through `ConfirmDialogContext` instead of passing it
to `Dialog.Root`.
- Replace `<Dialog.Heading />` with `<Dialog.Title>{ title }</Dialog.Title>`.
- Define `title` directly on `RootProps` instead of picking it from
`DialogRootProps`.
- Fix invalid `wpds` i18n text domain.
Made-with: Cursor
* ConfirmDialog: Adapt to @wordpress/ui package conventions
- Swap runtime `react` imports for `@wordpress/element`.
- Sort imports (externals, then @wordpress/*, then internal).
- Use `import type` for type-only imports.
- Move Storybook file to `stories/index.story.tsx`.
- Update story title to `Design System/Components/ConfirmDialog`.
- Fix `waitFor` blocks to use single assertions (lint rule).
- Add JSDoc to Trigger component.
- Export `ConfirmDialog` namespace from the package index.
Made-with: Cursor
* ConfirmDialog: Add CHANGELOG entry
Made-with: Cursor
* Dialog: Remove ConfirmDialog story now covered by dedicated component
Made-with: Cursor
* ConfirmDialog: Tighten irreversible intent behavior
- Add `role="alertdialog"` to the popup for `irreversible` intent,
`role="dialog"` for `default`.
- Block Escape key dismissal for `irreversible` intent (previously
only backdrop click was blocked).
- Block backdrop click for `default` intent too — confirm dialogs
should always require an explicit action.
- Extract `getIntentConfig()` helper in context.tsx to centralize
intent-dependent behavior (role, blocked dismiss reasons).
- Update JSDoc and story descriptions to reflect the new behavior.
Made-with: Cursor
* ConfirmDialog: Add `loading` prop for async confirm flows
Add a `loading` prop to `ConfirmDialog.Popup` that shows a spinner on
the confirm button and disables both buttons. All async flow logic
(controlled open state, preventing close during loading, closing on
completion) is handled by the consumer.
Pass `disabled` alongside `loading` to `Dialog.Action` to align
`_Dialog.Close`'s internal `useButton` with the desired disabled state.
Made-with: Cursor
* ConfirmDialog: Add CHANGELOG entries for enhancements
Made-with: Cursor
* ConfirmDialog: Move title prop from Root to Popup
The title is only consumed by Popup (to render Dialog.Title), so it
belongs there as a direct prop rather than traversing through context.
This simplifies the data flow and removes title from
ConfirmDialogContext.
Made-with: Cursor
* format css
* ConfirmDialog: Restore backdrop click dismissal for default intent
The default (non-irreversible) variant should allow dismissing via
backdrop click, matching the original behavior. Only the irreversible
intent blocks all implicit dismissals.
Made-with: Cursor
* Dialog: Fix Action disabled/loading forwarding to Button
Dialog.Action wraps Base UI's Dialog.Close, which defaults
`disabled` to `false` internally. This caused its `useButton` to
set `aria-disabled="false"`, overriding Button's own `aria-disabled`
derived from the `loading` prop.
Extract `disabled` and `loading`, resolve them the same way Button
does (`disabled ?? loading`), and pass the result to Dialog.Close.
Forward `loading` directly to Button for the spinner CSS class.
Made-with: Cursor
* ConfirmDialog: Remove disabled workaround on confirm action
Now that Dialog.Action properly resolves disabled from loading,
the explicit `disabled={loading || undefined}` workaround and its
explanatory comment are no longer needed.
Made-with: Cursor
* Remove superflous CHANGELOG entries
* Update CHANGELOG PR reference
* Allow Escape key to dismiss the irreversible variant, cleanup getIntentConfig
* Remove hardcoded domain from Storybook docs URL
* CHANGELOG
* Try to fix async confirm workflow
* Better approach to async confirm work
* Add async flow unit test
* ConfirmDialog: Clarify loading prop manual-close contract
Expand the `loading` JSDoc to explicitly state that passing the prop
(even as `false`) opts into manual-close mode. Add a regression test
encoding this contract so future contributors don't accidentally
change the `loading !== undefined` check.
Made-with: Cursor
* Log "onConfirm" action
* Refactor to use base ui's AlertDialog, rename to AlertDialog
* format
* initlal version
* ConfirmDialog: Fix Dialog API incompatibilities
The ConfirmDialog was authored against an older Dialog version that had
a `title` prop on `Dialog.Root` and a `Dialog.Heading` subcomponent.
The current Dialog uses `Dialog.Title` (with children) and has no
`title` prop on Root.
- Thread `title` through `ConfirmDialogContext` instead of passing it
to `Dialog.Root`.
- Replace `<Dialog.Heading />` with `<Dialog.Title>{ title }</Dialog.Title>`.
- Define `title` directly on `RootProps` instead of picking it from
`DialogRootProps`.
- Fix invalid `wpds` i18n text domain.
Made-with: Cursor
* ConfirmDialog: Adapt to @wordpress/ui package conventions
- Swap runtime `react` imports for `@wordpress/element`.
- Sort imports (externals, then @wordpress/*, then internal).
- Use `import type` for type-only imports.
- Move Storybook file to `stories/index.story.tsx`.
- Update story title to `Design System/Components/ConfirmDialog`.
- Fix `waitFor` blocks to use single assertions (lint rule).
- Add JSDoc to Trigger component.
- Export `ConfirmDialog` namespace from the package index.
Made-with: Cursor
* ConfirmDialog: Add CHANGELOG entry
Made-with: Cursor
* Dialog: Remove ConfirmDialog story now covered by dedicated component
Made-with: Cursor
* ConfirmDialog: Tighten irreversible intent behavior
- Add `role="alertdialog"` to the popup for `irreversible` intent,
`role="dialog"` for `default`.
- Block Escape key dismissal for `irreversible` intent (previously
only backdrop click was blocked).
- Block backdrop click for `default` intent too — confirm dialogs
should always require an explicit action.
- Extract `getIntentConfig()` helper in context.tsx to centralize
intent-dependent behavior (role, blocked dismiss reasons).
- Update JSDoc and story descriptions to reflect the new behavior.
Made-with: Cursor
* ConfirmDialog: Add `loading` prop for async confirm flows
Add a `loading` prop to `ConfirmDialog.Popup` that shows a spinner on
the confirm button and disables both buttons. All async flow logic
(controlled open state, preventing close during loading, closing on
completion) is handled by the consumer.
Pass `disabled` alongside `loading` to `Dialog.Action` to align
`_Dialog.Close`'s internal `useButton` with the desired disabled state.
Made-with: Cursor
* ConfirmDialog: Add CHANGELOG entries for enhancements
Made-with: Cursor
* ConfirmDialog: Move title prop from Root to Popup
The title is only consumed by Popup (to render Dialog.Title), so it
belongs there as a direct prop rather than traversing through context.
This simplifies the data flow and removes title from
ConfirmDialogContext.
Made-with: Cursor
* format css
* ConfirmDialog: Restore backdrop click dismissal for default intent
The default (non-irreversible) variant should allow dismissing via
backdrop click, matching the original behavior. Only the irreversible
intent blocks all implicit dismissals.
Made-with: Cursor
* Dialog: Fix Action disabled/loading forwarding to Button
Dialog.Action wraps Base UI's Dialog.Close, which defaults
`disabled` to `false` internally. This caused its `useButton` to
set `aria-disabled="false"`, overriding Button's own `aria-disabled`
derived from the `loading` prop.
Extract `disabled` and `loading`, resolve them the same way Button
does (`disabled ?? loading`), and pass the result to Dialog.Close.
Forward `loading` directly to Button for the spinner CSS class.
Made-with: Cursor
* ConfirmDialog: Remove disabled workaround on confirm action
Now that Dialog.Action properly resolves disabled from loading,
the explicit `disabled={loading || undefined}` workaround and its
explanatory comment are no longer needed.
Made-with: Cursor
* Remove superflous CHANGELOG entries
* Update CHANGELOG PR reference
* Allow Escape key to dismiss the irreversible variant, cleanup getIntentConfig
* Remove hardcoded domain from Storybook docs URL
* CHANGELOG
* Try to fix async confirm workflow
* Better approach to async confirm work
* Add async flow unit test
* ConfirmDialog: Clarify loading prop manual-close contract
Expand the `loading` JSDoc to explicitly state that passing the prop
(even as `false`) opts into manual-close mode. Add a regression test
encoding this contract so future contributors don't accidentally
change the `loading !== undefined` check.
Made-with: Cursor
* Log "onConfirm" action
* Refactor to use base ui's AlertDialog, rename to AlertDialog
* format
Prerequisite for #76837
What?
Add a new
AlertDialogcompound component to@wordpress/ui, built on top of Base UI'sAlertDialog. Also fixesDialog.Actiondisabled/loading prop forwarding.Why?
Provides a standardized alert dialog pattern for common confirm/cancel flows — including destructive and irreversible actions — so consumers don't need to compose
Dialogfrom scratch for these use cases.An alert dialog requires a user response to proceed. It uses
role="alertdialog", is always modal, and blocks backdrop click dismissal to ensure the user makes an explicit choice.How?
AlertDialogwraps Base UI'sAlertDialog.Rootand exposes a compound API:AlertDialog.Root,AlertDialog.Trigger, andAlertDialog.Popup.Implementation details
Component structure
Root— wraps Base UI'sAlertDialog.Root, managesAlertDialogContext(intent).Popup— rendersDialog.Popupwith a header (title), body (children), and footer (cancel + confirm actions).Trigger— thin wrapper overDialog.Trigger.context.tsx— providesAlertDialogContextwith the currentintent.Intent system
The
intentprop onRootcontrols styling:defaultirreversiblealertdialogalertdialogloadingprop (async confirm flows)AlertDialog.Popupaccepts aloadingprop that shows a spinner on the confirm button and disables both buttons. Whenloadingis provided, the confirm button renders as a plainButton(instead ofDialog.Action) so clicking it doesn't auto-close — this avoids a race condition where_Dialog.Close's close handler fires before the consumer'sonClick.Important: Passing
loading— even asfalse— opts into manual-close mode. The consumer is responsible for closing the dialog viaopen={false}. Omit the prop entirely for the default auto-close-on-confirm behavior. This contract is encoded in tests and documented in the prop's JSDoc.All async flow logic is consumer-side:
open/onOpenChange)onOpenChangewhile loadingloadingtoAlertDialog.Popupopen={false}when the operation completesDialog.Action disabled/loading fix
Dialog.Actionwraps Base UI'sDialog.Close, which defaultsdisabledtofalseinternally. This caused itsuseButtonto setaria-disabled="false", overriding Button's ownaria-disabledderived fromloading. Fixed by extractingdisabledandloading, resolving them (disabled ?? loading), and forwarding correctly to bothDialog.CloseandButton.Testing Instructions
Storybook
npm run storybook:devUnit tests
Testing Instructions for Keyboard
Use of AI Tools
Cursor + Claude Opus 4.6. All code was reviewed and validated by the author.