fix(attachments): a deleted inline image is inert, like the chip beside it

Final review round 5, asked as "what would you most regret merging". Making the file chip inert on deletion left its sibling behind: the inline image's placeholder kept role=button and tabindex=0 after a CONFIRMED deletion, while `retryLoad` refuses from that point on. A keyboard or screen-reader user got a focus stop that announces itself as a button and does nothing — the exact dead stop DR-12 names, and the one I had just closed on the surface next to it. Two surfaces, one object, and they disagreed. The semantics now follow the cause rather than being set once at construction: a transient failure IS retryable and keeps the button, a confirmed deletion drops role and tabindex and blurs the element first, so focus is not stranded somewhere no further keystroke can reach. Adds the image NodeView's first test file — through a real Tiptap editor, like the chip's, since these are properties of imperative NodeView DOM. All three cases fail without the fix.
2026-09-25 03:42:06 +00:00 · 2026-08-04 20:43:24 +00:00
parent 74ccada739
commit 95b6e1b3e8
2 changed files with 147 additions and 2 deletions
@@ -302,6 +302,21 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
 					? 'This attachment has been deleted'
 					: 'This attachment could not be loaded — it may have been deleted. Click to retry.';
 				missing.style.cursor = deleted ? 'default' : 'pointer';
+				// And the INTERACTIVE SEMANTICS go with the copy, not just the
+				// cursor (DR-12; final review round 5). A confirmed deletion
+				// makes `retryLoad` a no-op, so leaving role=button + tabindex
+				// hands a keyboard or screen-reader user a focus stop that
+				// announces itself as a button and does nothing — the same dead
+				// stop the file chip's `disabled` closes, on the surface next to
+				// it. A transient failure IS retryable and keeps both.
+				if (deleted) {
+					if (document.activeElement === missing) missing.blur();
+					missing.removeAttribute('role');
+					missing.removeAttribute('tabindex');
+				} else {
+					missing.setAttribute('role', 'button');
+					missing.setAttribute('tabindex', '0');
+				}
 				if (currentUuid) missing.setAttribute('data-attachment-id', currentUuid);
 				missing.style.display = '';
 				img.style.display = 'none';
@@ -403,8 +418,8 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
 				showMissing();
 			});

-			missing.setAttribute('role', 'button');
-			missing.setAttribute('tabindex', '0');
+			// role/tabindex are set by showMissing(), which knows whether this is
+			// a retryable failure or a confirmed deletion. Hidden and inert here.
 			missing.style.cursor = 'pointer';

 			function retryLoad() {
@@ -0,0 +1,130 @@
+// The inline image's missing-attachment placeholder (PLAN-2392 DR-12 / DR-17).
+//
+// Two causes land on the same element and must NOT look the same to a keyboard
+// or screen-reader user:
+//
+//   - a transient load failure — retryable, so the placeholder is a button;
+//   - a CONFIRMED deletion — `retryLoad` refuses, so a button that announces
+//     itself and does nothing is a dead focus stop. That is the same failure
+//     the file chip's `disabled` closes, on the surface right next to it.
+//
+// Driven through a REAL Tiptap editor, like the chip spec: the placeholder is
+// imperative NodeView DOM and its accessibility semantics are properties of
+// that DOM, which a hand-built element would not pin.
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { Editor } from '@tiptap/core';
+import StarterKit from '@tiptap/starter-kit';
+
+const deletionListeners = new Set<(uuid: string) => void>();
+vi.mock('$lib/attachments/events', () => ({
+	notifyAttachmentPanelOpen: () => {},
+	registerAttachmentDeletionListener: (fn: (uuid: string) => void) => {
+		deletionListeners.add(fn);
+		return () => deletionListeners.delete(fn);
+	},
+}));
+
+// No probe: these tests are about what the placeholder IS, not how it is
+// discovered, and a real HEAD would make them asynchronous for no gain.
+const probeMock = vi.fn(async () => ({ status: 'transient' as const }));
+vi.mock('./attachment-metadata', () => ({
+	fetchAttachmentMetadata: () => probeMock(),
+	revalidateAttachmentMetadata: () => probeMock(),
+	invalidateAttachmentMetadata: () => {},
+	mimeToFormat: () => null,
+}));
+
+const { AttachmentImage } = await import('./attachment-image');
+
+function makeEditor(element: HTMLElement): Editor {
+	return new Editor({
+		element,
+		extensions: [
+			StarterKit,
+			AttachmentImage.configure({
+				workspaceSlug: '',
+				getDownloadUrl: (uuid: string) => `/api/v1/workspaces/ws/attachments/${uuid}`,
+				address: () => ({ workspaceSlug: '', itemId: 'item-A', hostToken: 'apanel-1' }),
+				supportedFormats: [],
+				transform: async () => {
+					throw new Error('not used');
+				},
+			}),
+		],
+		content: '<p><img data-attachment-id="uuid-1" src="/api/v1/x" alt="A diagram"></p>',
+		editable: true,
+	});
+}
+
+describe('inline image missing placeholder', () => {
+	let target: HTMLElement;
+	let editor: Editor | undefined;
+
+	beforeEach(() => {
+		deletionListeners.clear();
+		probeMock.mockClear();
+		target = document.body.appendChild(document.createElement('div'));
+	});
+
+	afterEach(() => {
+		editor?.destroy();
+		editor = undefined;
+		target.remove();
+	});
+
+	function placeholder(): HTMLElement {
+		editor ??= makeEditor(target);
+		const el = target.querySelector<HTMLElement>('.attachment-missing');
+		if (!el) throw new Error('placeholder did not render');
+		return el;
+	}
+
+	function failLoad() {
+		const img = target.querySelector<HTMLImageElement>('img[data-attachment-id]');
+		if (!img) throw new Error('image NodeView did not render');
+		img.dispatchEvent(new Event('error'));
+	}
+
+	it('is a focusable button while the failure is merely transient', () => {
+		editor = makeEditor(target);
+		failLoad();
+
+		const el = placeholder();
+		expect(el.style.display).not.toBe('none');
+		// Retryable, so it invites the retry and can be reached to perform it.
+		expect(el.getAttribute('role')).toBe('button');
+		expect(el.getAttribute('tabindex')).toBe('0');
+		expect(el.title).toContain('retry');
+	});
+
+	it('drops its interactive semantics once the deletion is confirmed', async () => {
+		editor = makeEditor(target);
+		failLoad();
+		expect(placeholder().getAttribute('role')).toBe('button');
+
+		// Another surface deleted the row: authoritative, and `retryLoad`
+		// refuses from here on.
+		for (const fn of deletionListeners) fn('uuid-1');
+
+		const el = placeholder();
+		expect(el.getAttribute('role')).toBeNull();
+		expect(el.getAttribute('tabindex')).toBeNull();
+		// The copy stops inviting a retry that cannot happen, too.
+		expect(el.title).toBe('This attachment has been deleted');
+		expect(el.title).not.toContain('retry');
+	});
+
+	it('does not leave focus stranded on a placeholder that just went inert', () => {
+		editor = makeEditor(target);
+		failLoad();
+		const el = placeholder();
+		el.focus();
+		expect(document.activeElement).toBe(el);
+
+		for (const fn of deletionListeners) fn('uuid-1');
+
+		// Removing tabindex from the focused element would otherwise leave
+		// focus on something unreachable by any further keystroke.
+		expect(document.activeElement).not.toBe(el);
+	});
+});