From 95b6e1b3e80d2c60841bdec6c53e6345e1e02c21 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 4 Aug 2026 20:43:24 +0000 Subject: [PATCH] fix(attachments): a deleted inline image is inert, like the chip beside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../lib/components/editor/attachment-image.ts | 19 ++- .../attachmentImageMissing.svelte.test.ts | 130 ++++++++++++++++++ 2 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts diff --git a/web/src/lib/components/editor/attachment-image.ts b/web/src/lib/components/editor/attachment-image.ts index c4f88b16..005c4baf 100644 --- a/web/src/lib/components/editor/attachment-image.ts +++ b/web/src/lib/components/editor/attachment-image.ts @@ -302,6 +302,21 @@ export const AttachmentImage = Node.create({ ? '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({ 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() { diff --git a/web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts b/web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts new file mode 100644 index 00000000..f62ef4d1 --- /dev/null +++ b/web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts @@ -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: '

A diagram

', + 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('.attachment-missing'); + if (!el) throw new Error('placeholder did not render'); + return el; + } + + function failLoad() { + const img = target.querySelector('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); + }); +});