fix(block-editor): apply links to a selected image instead of replacing it

[rjvelazco]

Proposed Changes

Fixes #36361 — in the new block editor, selecting an image and clicking the toolbar Link button deleted the image and replaced it with a text link. The dotImage node already supports href/target end-to-end (it parses a wrapping <a> and re-wraps the <img> on render); the break was purely in the UI flow, which always routed through the text-link insert path.

What changed

• Toolbar Link button is image-aware — when a dotImage node is selected, the link popover opens in image-link mode (URL + "Open in new tab" only; Text/Advanced fields hidden) and applies the link via updateAttributes('dotImage', { href, target }), so the image is preserved instead of replaced.
• Remove link — a trash-can action appears when editing an image that already has a link, clearing href/target.
• Click behavior — clicking a linked image no longer opens the text-link editor; it selects the image node (the link is edited from the toolbar) and prevents the anchor from navigating.
• Layout fixes — eliminated the vertical jump/extra space when a link wraps the image: zero the inline-block baseline descender gap, and zero the Tailwind Typography img margin that reappears once the image is no longer a direct <figure> child.
• Active state — the toolbar Link button now highlights for linked images (isImageLinked), mirroring isLink for text links.

The text-link flow is unchanged. No dotImage node name or stored-content shape changed — links round-trip through the existing href/target attributes, so no data migration is required.

Files

• link-popover.component.{ts,html} — image-link mode, Remove action
• toolbar.component.{ts,html} + editor-toolbar.store.ts — image detection, active state
• editor-chrome-click.ts — don't open the dialog on linked-image clicks
• editor.component.css — layout/margin fixes
• editor-popover.service.ts — isImageLink payload flag
• Language.properties — dot.block.editor.dialog.link.remove

Testing

• pnpm nx lint new-block-editor ✓
• tsc --noEmit on the lib ✓
• Manual: insert image → select → Link button opens URL-only popover → Save keeps the image wrapped in <a> (not replaced); re-select shows active Link button + trash to remove; no layout jump toggling the link; text-link flow unchanged.

Video

video.mov

🤖 Generated with Claude Code

[claude]

Claude finished @rjvelazco's task in 1m 19s —— View job

---

Rollback Safety Analysis

• Read rollback-unsafe categories reference
• Get full PR diff
• Analyze diff against all unsafe categories
• Apply appropriate label (AI: Safe To Rollback ✅)

---

Result: ✅ Safe to Roll Back

All 10 changed files are frontend-only (Angular TypeScript/HTML/CSS) and one localization properties file. None of the changes fall into any rollback-unsafe category:

Category	Check	Result
C-1 Structural Data Model Change	No runonce tasks, no DB schema changes	✅ SAFE
C-2 Elasticsearch Mapping Change	No ES mapping code touched	✅ SAFE
C-3 Content JSON Model Version Bump	No CURRENT_MODEL_VERSION changes	✅ SAFE
C-4 DROP TABLE / DROP COLUMN	No SQL DDL	✅ SAFE
H-1 One-Way Data Migration	No data transformations	✅ SAFE
H-2 RENAME TABLE / COLUMN	No renames	✅ SAFE
H-3 PK Restructuring	No constraint changes	✅ SAFE
H-4 New Field Type	No new FieldTypeAPI registrations	✅ SAFE
H-5 Storage Provider Change	No storage config changes	✅ SAFE
H-6 DROP PROCEDURE / FUNCTION	None	✅ SAFE
H-7 NOT NULL Column Without Default	No DDL	✅ SAFE
H-8 VTL Viewtool Contract Change	No viewtool Java changes	✅ SAFE
M-1 Column Type Change	No DDL	✅ SAFE
M-2 Push Publishing Bundle Format	No bundle XML changes	✅ SAFE
M-3 REST/GraphQL API Contract	No REST endpoint changes	✅ SAFE
M-4 OSGi Plugin API	No OSGi interface changes	✅ SAFE

Summary: The PR exclusively modifies Angular component templates, TypeScript logic, CSS, and a Java Language.properties file. The dotImage node's stored attributes (href, target) already existed — only the UI path to set them is fixed. No database, Elasticsearch, REST API, or content model changes were made. Rolling back to N-1 removes the UI fix but leaves all stored data and server-side contracts intact.

[github-actions]

🤖 dotBot Review (Bedrock)

Reviewed 10 file(s); 7 candidate(s) → 5 confirmed, 1 uncertain (unverified, kept for review).

Confirmed findings

• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/components/link-popover/link-popover.component.html:108 — Advanced section visibility not reset when switching link modes
  The advancedOpen() state persists when switching between image and text link modes. When in image mode (isImageLink=true), the Advanced section is hidden, but if the user opens it in text mode, switches to image mode, then back to text mode, the Advanced section remains open. The component should reset advancedOpen to false when isImageLink changes to ensure proper state.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/components/link-popover/link-popover.component.ts:390 — Potential XSS in image href handling
  The code directly uses form input for image href without validation. The dotImage node's render logic (from documentation.json) shows href is rendered as an attribute without sanitization, allowing javascript: URLs. This could enable XSS when the linked image is clicked.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/components/link-popover/link-popover.component.ts:295 — Missing unit tests for image link attribute updates
  The link-popover.component.ts changes introduce image link handling in onInsert/onRemoveLink methods but lack corresponding test cases in the spec file. Manual testing was documented but automated verification of attribute persistence/removal for dotImage nodes is absent, creating risk of regression.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/extensions/nodes/image.extension.ts:185 — Right-click context menu blocked on images
  The mousedown handler in image.extension.ts calls event.preventDefault() unconditionally, suppressing the browser's default context menu on right-click (button 2). This interferes with native behavior and user expectations for image interactions.
• 🟡 Medium dotCMS/src/main/webapp/WEB-INF/messages/Language.properties:6126 — Missing translation keys in other language files
  The key 'dot.block.editor.dialog.link.remove' was added to Language.properties but not to other language files (e.g. Language_es.properties). This will cause missing translations for non-English locales. All new UI strings should be added to all supported language files, even if initially untranslated.

🔎 Uncertain (could not confirm or disprove — review manually)

• 🔵 Low core-web/libs/new-block-editor/src/lib/editor/extensions/nodes/image.extension.ts:185 — Missing event target check
  The mousedown handler triggers even when clicking child elements (like the image itself), but may need to verify event target to avoid interfering with nested interactions.

---

_{us.deepseek.r1-v1:0 · Run: #28457758185 · tokens: in: 60982 · out: 14949 · total: 75931 · calls: 22 · est. ~$0.163}

[github-actions]

🟠 [High] Missing event handler implementation

The template references onRemoveImageLink() click handler but this method is not implemented in link-popover.component.ts, causing runtime errors when attempting to remove image links.

[rjvelazco]

@claude review this changes, pretty please

[claude]

Claude finished @rjvelazco's task in 1m 21s —— View job

---

Code Review

• Gather context and understand the request
• Read all 10 changed files
• Analyze for bugs, correctness, and best practices
• Post review findings

---

Overall Assessment

This is a clean, well-scoped fix. The core approach is correct: routing the link toolbar button through updateAttributes('dotImage', ...) instead of the text-link insert path is exactly the right TipTap idiom, and the round-trip through href/target on the node's renderHTML/addNodeView is already proven. The layout fixes for the inline-block baseline gap and the Tailwind Typography margin regression are also correct. No data-migration is needed.

Below are the issues I found, from most to least significant.

---

🔴 Bug: renderHTML writes href="" when href is null

File: extensions/nodes/image.extension.ts:143

const anchorAttrs: Record<string, string> = { href };

When href is null (the default), this emits <a href="null">…</a> in the serialized HTML. The existing guard on line 145 (const inner = href ? ['a', anchorAttrs, img] : img) prevents the anchor being rendered at all, so this doesn't cause a visible bug today. But if renderHTML is ever called on a node with href: null directly (e.g. from a custom serializer or copy/paste), the wrong attribute object gets passed into TipTap's merge. Worth guarding:

const anchorAttrs: Record<string, string> = {};
if (href) anchorAttrs['href'] = href;
if (target) anchorAttrs['target'] = target;
const inner = href ? ['a', anchorAttrs, img] : img;

---

🟡 Missing test coverage: onRemoveImageLink

The bot flagged this incorrectly as "missing implementation" — onRemoveImageLink is implemented at link-popover.component.ts:433. But there are zero tests for it. The method does the right thing (updateAttributes + close()), but given this is the first time the remove-link path exists, a minimal spec covering: (a) updateAttributes called with { href: null, target: null }, and (b) manager.close() called, would prevent regressions.

---

🟡 Image-link edit state doesn't pre-populate openInNewTab

File: toolbar.component.ts:421–425

this.popovers.openLink(() => btn.getBoundingClientRect(), {
    isImageLink: true,
    initialValues: attrs['href']
        ? { href: attrs['href'], target: attrs['target'] ?? null }
        : undefined
});

The initialValues includes target, but the form's openInNewTab control is only set from initialValues.target in the pre-populate effect (values.target === '_blank'). Trace the path:

1. Image has target="_blank" → attrs.target = '_blank'
2. initialValues.target = '_blank' is passed correctly
3. The pre-populate effect reads values.target === '_blank' → sets openInNewTab: true ✅

This actually works. But if attrs['target'] is null (no target attribute at all), the ?? null fallback is fine. No bug — just worth a comment clarifying that target flows into openInNewTab via the existing effect.

---

🟡 ::ng-deep in new CSS (pre-existing pattern, but worth flagging)

File: editor.component.css:244

The PR adds two new ::ng-deep blocks for the linked-image layout fix. ::ng-deep is deprecated in Angular but this file already uses it pervasively (it's the established pattern for styling ProseMirror's DOM from the Angular component). The new rules follow the existing pattern correctly — no change needed, but see Angular deprecation note.

---

🟡 mousedown + event.preventDefault() on the remove button blocks keyboard access

File: link-popover.component.html:187

(mousedown)="$event.preventDefault(); onRemoveImageLink()"

Using mousedown + preventDefault is the correct pattern for toolbar buttons that must not blur the editor (consistent with every other button in this file). However, the <button> does not have a (click) handler as a fallback, so keyboard users who Tab to this button and press Enter/Space will activate the click event, not mousedown, and nothing will happen. The other destructive buttons in this file (Cancel, Insert) use mousedown on buttons where preventing editor blur is the purpose — but onRemoveImageLink closes the popover and returns focus to the editor anyway, so a (click) handler would actually be fine here:

(click)="onRemoveImageLink()"

Or add both:

(mousedown)="$event.preventDefault()" (click)="onRemoveImageLink()"

---

🟢 Minor: target attribute not cleared on renderHTML when null

File: extensions/nodes/image.extension.ts:144

const anchorAttrs: Record<string, string> = { href };
if (target) anchorAttrs['target'] = target;

(Fixing the href: null issue above would also naturally express this more clearly.) The current code already conditionally sets target — this part is fine.

---

🟢 No confirmation before removing the link

The bot flagged this as a concern — I don't share it. A trash icon for a single attribute (not content deletion) is a well-established pattern and the action is undoable via Ctrl+Z. No confirmation dialog needed here.

---

Summary

Severity	Issue	File	Line
🔴 Bug (latent)	{ href } spreads null into anchorAttrs	image.extension.ts	143
🟡 Missing tests	onRemoveImageLink has no spec	link-popover.component.ts	433
🟡 A11y	Remove button only responds to mousedown, not keyboard click	link-popover.component.html	187

The href: null spread is latent (current guard prevents it rendering), but it should be fixed before it causes confusion. Fix this →

[rjvelazco]

Review dispositions

Thanks for the automated reviews. Summary of how each finding was handled:

Addressed

• Overly broad click prevention (editor-chrome-click.ts) — tightened in 814ba6c to gate the link-navigation preventDefault() on the clicked element being the image (event.target.closest('img') within the anchor) rather than any descendant img, so a text click in a mixed anchor still routes to the text-link editor.
• href: null → href="null" in renderHTML (image.extension.ts) — anchorAttrs now built conditionally so a null/absent href never serializes.
• Remove-link button keyboard a11y (link-popover.component.html) — split into (mousedown)="$event.preventDefault()" + (click)="onRemoveImageLink()" so Enter/Space activate it without blurring the editor or double-firing on mouse.

False positives (no change)

• Missing onRemoveImageLink() implementation — the method is implemented at link-popover.component.ts:433; the handler exists and is wired. (The Bedrock/Claude review also corrected this.)
• Image href lacks protocol validation / XSS — image and text links share the same validation. onInsert() returns early on this.form.controls.href.invalid before the image branch, and href carries [Validators.required, linkHrefValidator]. isValidLinkHref allows only http/https/mailto/tel + anchor/relative paths; javascript: parses to protocol === 'javascript:', fails the allowlist, and the form is invalid → nothing is applied. There is no path that sets an image href outside this validated form.

Acknowledged, no change

• ::ng-deep without justification (editor.component.css) — ::ng-deep is the established pattern throughout this file for styling ProseMirror's DOM from the Angular component; the new rules follow it and carry comments explaining the layout rationale.

Note on tests

No spec was added for onRemoveImageLink because libs/new-block-editor/ intentionally has no *.spec.ts files yet (project convention); changes here are verified via nx lint + tsc.