Skip to content

fix(edit-content): make WYSIWYG Insert Image dialog dismissable (#35898)#35900

Merged
erickgonzalez merged 3 commits into
mainfrom
issue-35898-insert-image-dialog-in-wysiwyg-field-cannot-be-clo
Jun 3, 2026
Merged

fix(edit-content): make WYSIWYG Insert Image dialog dismissable (#35898)#35900
erickgonzalez merged 3 commits into
mainfrom
issue-35898-insert-image-dialog-in-wysiwyg-field-cannot-be-clo

Conversation

@oidacra

@oidacra oidacra commented Jun 2, 2026

Copy link
Copy Markdown
Member

Summary

The Insert Image dialog opened from a WYSIWYG field in the New Edit Content experience had no way to be dismissed — no close (X) icon, no Esc-key support, and clicking the overlay mask did nothing. Users were trapped in the dialog and could only escape by selecting an image (then deleting it).

This adds the three PrimeNG DynamicDialog dismissal flags to the dialogService.open(DotAssetSearchDialogComponent, …) call in DotWysiwygPluginService.dotImageDialog():

closable: true,        // renders the X icon in the header
closeOnEscape: true,   // Esc dismisses the dialog
dismissableMask: true, // overlay-mask click dismisses the dialog

In PrimeNG 21.1.3 these DynamicDialogConfig options all default to false, so the dialog previously rendered with no close affordances. This follows the project convention in core-web/CLAUDE.md ("All dialogs must have closable: true and closeOnEscape: true") and mirrors the sibling block-editor asset picker (new-block-editor/src/lib/editor/config.utils.ts), which already sets all three flags. No changes were needed to DotAssetSearchDialogComponent itself.

The onClose subscription was also reworked: instead of the previous filter((asset) => !!asset) (which produced no callback at all on dismissal), it now subscribes unconditionally — inserting content only when an asset is selected, and calling editor.focus() on every close path so focus reliably returns to the editor (AC5) without depending on PrimeNG/TinyMCE default focus behavior.

Additionally, a small UI tweak adds host: { class: 'px-4' } (Tailwind, 1rem left/right) to the shared DotAssetCardListComponent, giving the asset list horizontal breathing room. Note this component is also consumed by the block-editor asset picker, so the gutter change applies there too.

Closes #35898

Acceptance Criteria

  • AC1 — Insert Image dialog displays a close (X) icon in the header (closable: true; verified against PrimeNG 21.1.3 source where the default is false)
  • AC2 — Clicking the X dismisses the dialog without inserting an image (onClose only inserts when an asset is selected; covered by the negative-path unit test)
  • AC3 — Pressing Esc dismisses the dialog without inserting an image (closeOnEscape: true)
  • AC4 — Clicking outside (overlay mask) dismisses the dialog without inserting an image (dismissableMask: true) — intentional scope addition beyond the original issue ACs, justified by the reported behavior in the issue video
  • AC5 — After dismissal, keyboard focus returns to the WYSIWYG editor (editor.focus() is now called on every close path; asserted in both the insert and dismiss unit tests)
  • AC6 — After dismissal, the editor content is unchanged (insertion is gated on a selected asset; covered by the negative-path unit test)

AC7 from the original issue (cancel in-progress upload on close) was determined out of scope: this dialog is browse-only. Disk uploads use a separate drag-and-drop path (handleImageDrop).

Test Plan

  • yarn nx lint edit-content → 0 errors (pre-existing warnings only, in unrelated files)
  • yarn nx test edit-content → 1867 passed, 0 failures
  • yarn nx lint ui / yarn nx test ui → 0 errors, all green (covers the dot-asset-card-list change)
  • Unit test should open the dialog when the button is clicked — asserts the full dialogConfig (including the three new flags) and that editor.focus() is called after insertion.
  • Unit test should NOT insert content when the dialog is closed without selecting an image — mocks onClose emitting undefined, asserts editor.insertContent is not called and editor.focus() is called (AC5 on the dismiss path).

Changed Files

  • core-web/libs/edit-content/src/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.ts
  • core-web/libs/edit-content/src/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.spec.ts
  • core-web/libs/ui/src/lib/components/dot-asset-search/components/dot-asset-card-list/dot-asset-card-list.component.ts

Demo

CleanShot.2026-06-02.at.11.26.58.mp4

This PR fixes: #35898

Add closable, closeOnEscape and dismissableMask flags to the
DotAssetSearchDialogComponent opened from the WYSIWYG field, so the
Insert Image dialog can be closed via the X icon, the Esc key, or an
overlay-mask click without inserting an image. The existing
filter((asset) => !!asset) guard on onClose already prevents content
insertion on a no-selection close. Adds a negative-path unit test
covering dismissal without selection.

Closes #35898
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @oidacra's task in 1m 36s —— View job


Review

The fix is correct and the tests cover both paths. A few things worth a second look:

1. PR body is out of sync with the code (minor)

AC5 is marked unchecked / "needs manual QA" in the description, but commit dc8f4522 now calls editor.focus() on every close path and the new test asserts it on the dismiss path. The description and AC checklist should be updated — AC5 is implemented and unit-tested, only the visual focus-ring behaviour still needs eyes-on QA.

2. Out-of-scope styling change

The host: { class: 'px-4' } added to dot-asset-card-list.component.ts:28 is unrelated to making the dialog dismissable and is not mentioned in the PR description (Summary / ACs / Changed Files). It lives in libs/ui and the component is consumed by dot-asset-search.component.html, so any other consumer of DotAssetCardListComponent (and the block-editor asset picker indirectly, since the dialog reuses the same search component) will pick up the new padding. Either:

  • call it out explicitly in the PR description (what it fixes, screenshot before/after), or
  • split it into its own PR.

Right now it's a silent shared-component change riding in a fix PR.

3. Replacing the filter operator with an if (intentional, but worth confirming)

dot-wysiwyg-plugin.service.ts:91-99 — the .pipe(filter(asset => !!asset)) was deliberately removed so editor.focus() runs on every close. That's correct for AC5. Just be aware: any future contributor reading the diff might think the filter was lost by accident. The inline comment helps, but moving the focus call into a finalize/tap could make the intent clearer (or leaving as-is is fine — just flagging).

4. dismissableMask: true is a UX choice beyond the issue

Called out in the PR body, but worth a second thought: clicking the mask is a "tap anywhere" dismiss, which can fire on accidental drags out of the search input. The sibling block-editor picker uses it, so consistency is the right call — just noting that this is the easiest path to an accidental dismiss with no undo (in-progress search state is lost).

5. Subscription not torn down

ref.onClose.subscribe(...) at dot-wysiwyg-plugin.service.ts:91 has no takeUntilDestroyed / explicit unsubscribe. PrimeNG completes onClose after the dialog closes so it's not a real leak today, but the rest of this service uses takeUntilDestroyed(this.destroyRef$) (line 37). Adding it here would match the file's existing pattern and survive any future PrimeNG change to the onClose semantics. Not blocking.

6. Nits

  • dot-wysiwyg-plugin.service.spec.ts:168-170// AC5: … ties the comment to PR-review context. Per the repo guideline ("don't reference the current task / fix / issue") this should be reworded to describe the behaviour without the AC tag.
  • Test name "should NOT insert content when the dialog is closed without selecting an image" reads well; could also assert the result is the same on null (the other "no selection" sentinel some PrimeNG dialogs emit), but undefined is what PrimeNG actually emits here so it's fine.

Summary

No bugs in the dialog-dismissal logic itself. Main asks: update the PR description (AC5 + the asset-card-list padding change), and consider whether the padding change should be its own PR.


  • Read changed files
  • Check git history / diff
  • Cross-check against AC and CLAUDE.md conventions
  • Post review
    • Branch: issue-35898-insert-image-dialog-in-wysiwyg-field-cannot-be-clo

Add Tailwind px-4 (1rem left/right) to the dot-asset-card-list host
so the asset list in the Insert Image dialog has horizontal breathing
room.
@oidacra oidacra marked this pull request as ready for review June 2, 2026 15:25
…log close

Subscribe to the dialog onClose without the truthy filter so the editor
regains focus on every close path (image insert, X, Esc or overlay mask),
satisfying AC5 without relying on PrimeNG/TinyMCE default focus behavior.
Insertion still only happens when an asset is selected. Unit tests assert
editor.focus() is called on both the insert and dismiss branches.
@oidacra oidacra enabled auto-merge June 2, 2026 15:41
@oidacra oidacra added this pull request to the merge queue Jun 2, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 2, 2026
@erickgonzalez erickgonzalez added this pull request to the merge queue Jun 3, 2026
Merged via the queue into main with commit 881330b Jun 3, 2026
34 checks passed
@erickgonzalez erickgonzalez deleted the issue-35898-insert-image-dialog-in-wysiwyg-field-cannot-be-clo branch June 3, 2026 15:05
fmontes pushed a commit that referenced this pull request Jun 4, 2026
…) (#35900)

## Summary

The **Insert Image** dialog opened from a **WYSIWYG field** in the New
Edit Content experience had no way to be dismissed — no close (X) icon,
no Esc-key support, and clicking the overlay mask did nothing. Users
were trapped in the dialog and could only escape by selecting an image
(then deleting it).

This adds the three PrimeNG `DynamicDialog` dismissal flags to the
`dialogService.open(DotAssetSearchDialogComponent, …)` call in
`DotWysiwygPluginService.dotImageDialog()`:

```ts
closable: true,        // renders the X icon in the header
closeOnEscape: true,   // Esc dismisses the dialog
dismissableMask: true, // overlay-mask click dismisses the dialog
```

In PrimeNG 21.1.3 these `DynamicDialogConfig` options all default to
`false`, so the dialog previously rendered with no close affordances.
This follows the project convention in `core-web/CLAUDE.md` ("All
dialogs must have `closable: true` and `closeOnEscape: true`") and
mirrors the sibling block-editor asset picker
(`new-block-editor/src/lib/editor/config.utils.ts`), which already sets
all three flags. No changes were needed to
`DotAssetSearchDialogComponent` itself.

The `onClose` subscription was also reworked: instead of the previous
`filter((asset) => !!asset)` (which produced no callback at all on
dismissal), it now subscribes unconditionally — inserting content only
when an asset is selected, and calling `editor.focus()` on **every**
close path so focus reliably returns to the editor (AC5) without
depending on PrimeNG/TinyMCE default focus behavior.

Additionally, a small UI tweak adds `host: { class: 'px-4' }` (Tailwind,
1rem left/right) to the shared `DotAssetCardListComponent`, giving the
asset list horizontal breathing room. Note this component is also
consumed by the block-editor asset picker, so the gutter change applies
there too.

Closes #35898

## Acceptance Criteria

- [x] AC1 — Insert Image dialog displays a close (X) icon in the header
(`closable: true`; verified against PrimeNG 21.1.3 source where the
default is `false`)
- [x] AC2 — Clicking the X dismisses the dialog without inserting an
image (`onClose` only inserts when an asset is selected; covered by the
negative-path unit test)
- [x] AC3 — Pressing Esc dismisses the dialog without inserting an image
(`closeOnEscape: true`)
- [x] AC4 — Clicking outside (overlay mask) dismisses the dialog without
inserting an image (`dismissableMask: true`) — *intentional scope
addition beyond the original issue ACs, justified by the reported
behavior in the issue video*
- [x] AC5 — After dismissal, keyboard focus returns to the WYSIWYG
editor (`editor.focus()` is now called on every close path; asserted in
both the insert and dismiss unit tests)
- [x] AC6 — After dismissal, the editor content is unchanged (insertion
is gated on a selected asset; covered by the negative-path unit test)

> AC7 from the original issue (cancel in-progress upload on close) was
determined **out of scope**: this dialog is browse-only. Disk uploads
use a separate drag-and-drop path (`handleImageDrop`).

## Test Plan

- `yarn nx lint edit-content` → 0 errors (pre-existing warnings only, in
unrelated files)
- `yarn nx test edit-content` → 1867 passed, 0 failures
- `yarn nx lint ui` / `yarn nx test ui` → 0 errors, all green (covers
the `dot-asset-card-list` change)
- Unit test `should open the dialog when the button is clicked` —
asserts the full `dialogConfig` (including the three new flags) **and**
that `editor.focus()` is called after insertion.
- Unit test `should NOT insert content when the dialog is closed without
selecting an image` — mocks `onClose` emitting `undefined`, asserts
`editor.insertContent` is **not** called **and** `editor.focus()` **is**
called (AC5 on the dismiss path).

## Changed Files

-
`core-web/libs/edit-content/src/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.ts`
-
`core-web/libs/edit-content/src/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.spec.ts`
-
`core-web/libs/ui/src/lib/components/dot-asset-search/components/dot-asset-card-list/dot-asset-card-list.component.ts`

## Demo


https://github.com/user-attachments/assets/da2813c3-625c-482f-88c9-1be01ee2bcf9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Insert Image dialog in WYSIWYG field cannot be closed (add X button and Esc key)

2 participants