Skip to content

[6.x] Confirm image editor discards in a CP dialog instead of window.confirm() - #20466

Open
brandonkelly wants to merge 2 commits into
6.xfrom
bugfix/image-editor-confirm-dialog
Open

brandonkelly wants to merge 2 commits into
6.xfrom
bugfix/image-editor-confirm-dialog

Conversation

@brandonkelly

Copy link
Copy Markdown
Member

Description

Closing the image editor after a change, or clicking “Reset to original”, asked with the browser’s native window.confirm(). That blocks the page (and freezes browser automation), can’t be themed, and doesn’t match the rest of the control panel. Both now ask in a craft-dialog, the way other in-page CP confirmations do (e.g. WorkflowEditLock.vue): a label, the existing message, a focused Cancel, and a danger action.

 onBeforeHide(event)                  // close button, Escape, backdrop
   if editor.isDirty
     event.preventDefault()
-    if window.confirm(…) → close
+    askToConfirm('close')            // opens <craft-dialog> “Discard changes”

 onConfirmationHidden()               // the confirmation's craft-after-hide
+  if confirmed
+    'close' → opened = false         // focus has already gone back to the editor,
+                                     // so the editor's own return still works
+    'reset' → editor.reset(), then focus the selected tab
+                                     // the Reset button disables itself after a reset
  • The confirmation is a sibling of the editor’s dialog, not a child: craft-dialog’s lifecycle events bubble, and the editor listens for craft-after-show/craft-hide on its own dialog.
  • openImageEditorDialog() tore its host down on the first craft-after-hide inside it. It now only tears down once the editor itself is closed, so canceling the confirmation doesn’t unmount the editor.
  • The editor’s scoped ::part() padding overrides now target its own dialog, so they don’t strip the confirmation’s padding.
  • Undo/redo shortcuts are paused while the confirmation is open.

Evidence

New open-image-editor-dialog.browser.test.ts opens the editor through openImageEditorDialog() with an edit pending, then:

request close        → window.confirm not called; dialogs open: [Edit Image, Discard changes]
Cancel               → dialogs open: [Edit Image]
request close, Discard changes → no dialogs left; editor removed
  • Before (both source files reverted): fails, expected "vi.fn()" to not be called at all, but actually been called 1 times with "Any changes will be lost if you close the image editor."
  • Before (only the host guard reverted): fails, expected [] to deeply equal [ 'Edit Image' ]. Canceling unmounted the editor.
  • After: passes.

Checked by hand in the workbench (Chromium): from the asset edit page and from the Assets index “Edit Image” action, the confirmation opens with focus on Cancel. Tab stays inside it. Escape or Cancel returns focus to the control that asked (the editor’s Close button, or Reset). “Discard changes” closes the editor and returns focus to the index’s Actions button. “Reset to original” resets and focuses the selected tab. Closing an unchanged editor doesn’t ask. Screenshots are in the Amp thread; this environment can’t upload images to GitHub.

vp lint, stylelint, vue-tsc, and the image editor unit tests pass. In composer ci, pint, rector, phpstan, and the core tests pass. The adapter suite had 3 unrelated failures (ConfigCompatibilityTest, LegacyMarkdownTest deprecation-log counts). Those tests pass when run alone, and this PR changes no PHP.

Not in this PR: about 35 other confirm() calls across the CP (slideout and cp-modal dirty checks, delete buttons, @craftcms/ui actions, legacy modules). They use a synchronous boolean return, so replacing them needs a shared promise-based confirmation helper. That’s a separate design decision.

brandonkelly and others added 2 commits November 7, 2026 22:16
Closing the image editor with changes, or resetting it to the original,
asked with the browser's native confirm(), which blocks the page and
can't be themed. Both now ask in a craft-dialog with Cancel focused.

The confirmation sits beside the editor's dialog, since craft-dialog's
lifecycle events bubble, and openImageEditorDialog() only tears its host
down once the editor itself has closed.

Amp-Thread-ID: https://ampcode.com/threads/T-01a1b7ff-c89a-7546-b6a9-cdb8ba41cc70
Co-authored-by: Amp <amp@ampcode.com>
@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

📚 Storybook previews

@craftcms/ui — open Storybook

No changed components detected in this Storybook.

resources/js — open Storybook

No changed components detected in this Storybook.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant