Media editor: confirm before discarding unsaved changes#77730
Merged
Conversation
|
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. |
ramonjd
commented
Apr 28, 2026
Comment on lines
+158
to
+165
| <Button | ||
| size="compact" | ||
| icon={ close } | ||
| label={ __( 'Close' ) } | ||
| onClick={ onCancel } | ||
| disabled={ isSaving } | ||
| accessibleWhenDisabled | ||
| /> |
Member
Author
There was a problem hiding this comment.
Why a custom X button?
Because the modal's X button/ESC key/backdrop click all play the close animation before calling onRequestClose.
So by the time we open the confirm dialog, the modal has visually animated away. If the user picks “Keep editing”, the modal abruptly snaps back into place.
Kapture.2026-04-28.at.12.49.17.mp4
The downside here is that we have to disable shouldCloseOnClickOutside when there are pending changes. It suffers from the same cause as above, the there’s no prop to intercept it before the animation.
Contributor
There was a problem hiding this comment.
The downside here is that we have to disable shouldCloseOnClickOutside when there are pending changes. It suffers from the same cause as above, the there’s no prop to intercept it before the animation.
I actually don't mind making it slightly harder to close the modal if a user has pending changes, so having to disable shouldCloseOnClickOutside seems reasonable to me 👍
There was a problem hiding this comment.
From UX perspective, why the X when we have already Cancel? What's the difference for the user?
Member
Author
There was a problem hiding this comment.
Good question. It's mainly about consistency at the moment - most, if not all, editor models have this escape hatch.
This is an experimental feature so we can try things out. If it turns out that the X button is superfluous, we can ditch it later.
|
Size Change: +263 B (0%) Total Size: 7.77 MB 📦 View Changed
ℹ️ View Unchanged
|
ramonjd
commented
Apr 28, 2026
| onRequestClose={ handleCancel } | ||
| isDismissible={ false } | ||
| shouldCloseOnEsc={ ! hasChanges } | ||
| shouldCloseOnClickOutside={ ! hasChanges } |
Member
Author
There was a problem hiding this comment.
I created an issue here to change the component to change the order of animation/close callback:
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an “unsaved changes” discard confirmation flow to the media editor modal so users don’t accidentally lose in-progress cropper transforms or attachment field edits when closing.
Changes:
- Hoists cropper dirty state (
useCropper().isDirty) into a new inner component so close logic can consider both cropper and entity edits. - Introduces a controlled
__experimentalConfirmDialogto confirm discarding changes on close attempts. - Updates modal dismiss behavior and header actions (adds explicit Close icon button; disables some default dismissal paths when dirty).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramonjd
added a commit
that referenced
this pull request
Apr 28, 2026
Address review feedback on PR #77730: - Restore ESC-to-discard-dialog: keep shouldCloseOnEsc default (true) and intercept ESC via Modal's onKeyDown when there are pending changes, preventDefault() so Modal's own overlay handler skips the close. - Render ConfirmDialog inside Modal's children so it inherits the ModalContext dismisser tree. As a top-level sibling it would, on mount, request the parent Modal close. Backdrop click remains a no-op when dirty since Modal does not expose a hook to intercept it before the close animation.
|
Flaky tests detected in 4be0170. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25036414237
|
Contributor
andrewserong
left a comment
There was a problem hiding this comment.
Testing well for me manually, and I really like this addition, it gives greater confidence to clearing out the edited state (as we'll be doing in the "save" PR) as we're adding an explicit confirmation that the user needs to do here.
Just found a couple of very minor issues, hopefully should be a quick fix 🤞
Comment on lines
+471
to
488
| // `CropperProvider` is always mounted while the modal is open — it's | ||
| // just a `useReducer` with no side effects — so the inner component | ||
| // can read `isDirty` for images without forking on media type. The | ||
| // `key` remounts the provider when the edited attachment changes, | ||
| // discarding the previous cropper state. Today the modal always | ||
| // closes between edits so this is belt-and-braces, but it guards | ||
| // against future flows that swap `id` in the store without closing. | ||
| return ( | ||
| <CropperProvider key={ media?.id ?? 'none' }> | ||
| <MediaEditorModalContent | ||
| fields={ fields } | ||
| id={ id } | ||
| media={ media } | ||
| hasEdits={ hasEdits } | ||
| isModalOpen={ isModalOpen } | ||
| onUpdate={ onUpdate } | ||
| /> | ||
| </CropperProvider> |
Contributor
There was a problem hiding this comment.
Just a thought but the concept here is also good for if we ever want to implement the contents of the modal outside of the modal, too. I like the little refactor!
ramonjd
added a commit
that referenced
this pull request
Apr 28, 2026
Address review feedback on PR #77730: - Restore ESC-to-discard-dialog: keep shouldCloseOnEsc default (true) and intercept ESC via Modal's onKeyDown when there are pending changes, preventDefault() so Modal's own overlay handler skips the close. - Render ConfirmDialog inside Modal's children so it inherits the ModalContext dismisser tree. As a top-level sibling it would, on mount, request the parent Modal close. Backdrop click remains a no-op when dirty since Modal does not expose a hook to intercept it before the close animation.
cb7247c to
8ef1489
Compare
ramonjd
added a commit
that referenced
this pull request
Apr 28, 2026
Address review feedback on PR #77730: - Drop `isModalOpen` from `MediaEditorModalContent`. The outer component returns null when closed and remounts the inner via `CropperProvider`'s key when media changes, so the prop was always true and the snapshot-clear branch was dead. - Make ESC, the close button, and backdrop click no-ops while a save is in flight, so the in-progress request can settle without the modal closing under it.
andrewserong
approved these changes
Apr 28, 2026
Contributor
andrewserong
left a comment
There was a problem hiding this comment.
Updates look good to me, I reckon this'll be good to land after a rebase 👍
When the user closes the media editor modal with pending cropper edits or attachment field edits, prompt to confirm before discarding. Skip the prompt when there's nothing to lose.
The built-in Modal close button always plays its exit animation before firing onRequestClose. When pending changes prompted a confirmation dialog, the modal had already animated out — picking 'Keep editing' caused it to flicker back in. Render the X icon in headerActions so the close intent flows directly through our handler without animating the modal away. Disable shouldCloseOnEsc / shouldCloseOnClickOutside while there are pending changes so accidental dismissals don't lose work.
Address review feedback on PR #77730: - Restore ESC-to-discard-dialog: keep shouldCloseOnEsc default (true) and intercept ESC via Modal's onKeyDown when there are pending changes, preventDefault() so Modal's own overlay handler skips the close. - Render ConfirmDialog inside Modal's children so it inherits the ModalContext dismisser tree. As a top-level sibling it would, on mount, request the parent Modal close. Backdrop click remains a no-op when dirty since Modal does not expose a hook to intercept it before the close animation.
Address review feedback on PR #77730: - Drop `isModalOpen` from `MediaEditorModalContent`. The outer component returns null when closed and remounts the inner via `CropperProvider`'s key when media changes, so the prop was always true and the snapshot-clear branch was dead. - Make ESC, the close button, and backdrop click no-ops while a save is in flight, so the in-progress request can settle without the modal closing under it.
68eee39 to
4be0170
Compare
t-hamano
pushed a commit
that referenced
this pull request
Apr 29, 2026
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
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
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
Prompts the user to confirm before discarding pending changes when closing the media editor modal.
If either cropper transforms (
isDirty) or attachment field edits (hasEdits) are pending, clicking Cancel, the close button, ESC, or the backdrop now opens a confirmation dialog. If there's nothing to lose, the modal closes immediately as before.Kapture.2026-04-28.at.13.03.09.mp4
Why
Today the modal silently throws away any in-progress crop/rotate/flip work and quietly rolls back field edits when the user cancels. There's no warning, so an accidental ESC press loses everything.
How
useCropper().isDirtyfromHeaderActionsup into a newMediaEditorModalContentcomponent rendered insideCropperProvider. The close handler can now read both dirty signals (isDirty || hasEdits) before deciding whether to confirm.__experimentalConfirmDialogfrom@wordpress/components, following the existing pattern inglobal-styles-ui(the newer@wordpress/uiAlertDialogis not yet on the recommended-components allowlist).Test plan