fix(transcription): remove annotation canvas delete button that obscured text (#722) #723
Reference in New Issue
Block a user
Delete Branch "fix/issue-722-remove-annotation-canvas-delete-button"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #722.
What
Removes the per-annotation delete button from the document canvas — a 44 px circular control pinned to each box's top-right corner. On single-line annotations it was taller than the box itself, overlapping the box below and obscuring the underlying document text.
Why removal (not "improve in place")
The button was redundant in every normal flow: drawing a box atomically creates an annotation and its transcription block, and the right-hand panel already provides a non-overlapping, labelled delete per block that cascades to the annotation. A 44 px touch target (required for tablet transcribers) can't be shrunk enough to stop overlapping small boxes, so the geometry was unwinnable.
What stays
The hidden keyboard Delete shortcut is kept — it obscures nothing and remains a power-user path (and the only cleanup route for orphan annotations, which only arise from backend data corruption). Its full wiring (
showDelete→onDeleteRequest→deleteAnnotation) is untouched, so the confirm-dialog + cascade behaviour is unchanged.Changes
AnnotationShape.svelte— remove the delete<button>and the now-unuseddeleteVisiblederived; keep theDelete-key branch.AnnotationShape.svelte.spec.ts— replace the 6 button-render/click tests with a single contract test asserting no delete button ever renders; keep the keyboard tests.AnnotationLayer.svelte.spec.ts— consolidate the two now-vacuous absence tests into one contract test.e2e/documents.spec.ts— repointadmin can delete an annotationto the kept path (focus annotation →Delete→ confirm "Bestätigen").Verification
npm run check, Prettier, ESLint — clean for all touched files (pre-commit lint hook passed).🤖 Generated with Claude Code
🏛️ Markus Keller — Application Architect
⚠️ Approved with concerns (one minor, non-blocking)
A clean subtraction. This is a pure UI-control removal with no impact on layer boundaries, transport, schema, or module ownership — nothing in my architectural triggers fires (no Flyway migration, no new package/route/controller/service, no
ErrorCode/Permission, no infra). The structurally interesting decision — keep the prop contract, remove only the visible affordance — was made correctly.What I checked
documents/[id]/+page.svelte:263(onDeleteAnnotationRequest={handleAnnotationDeleteRequest}) →PdfViewer.svelte:297(onDeleteRequest) →AnnotationLayer.svelte:117-118(showDelete={canDraw},onDeleteRequest) →AnnotationShape.svelte:90keyboardDeletebranch →useTranscriptionBlocks.deleteAnnotation. No segment is orphaned.deleteVisible) was removed. The retained propsshowDelete/onDeleteRequestare not dead —showDeletestill gates the keyboard branch (AnnotationShape.svelte:90) andonDeleteRequestis still invoked there. Keeping them is the right call: collapsing them now would couple the keyboard path's removal to this change for no benefit. Good restraint.AnnotationShapekeeps its one job (render a box + emit interaction intents); the parent still owns the delete policy. Removing the in-shape<button>actually tightens the separation — the component no longer renders its own action chrome.isHoveredremains load-bearing (bgAlpha/boxShadow), so it wasn't left as a vestigial prop after the button that consumed it went away.Suggestion (non-blocking)
AnnotationLayer.svelte.test.ts:243. This is a different file from the.spec.tsthe PR touched, and it still carries the parenthetical'renders without throwing when canDraw is true (delete button visible)'. The body is fine (no-throw assertion only), but the label now describes behaviour that no longer exists — exactly the kind of stale breadcrumb that misleads a future reader into thinking a delete button renders atcanDraw. The issue's scope note said stale delete references were "folded into scope during implementation," but this.test.tsslipped through because the scope table only enumerated the.spec.tsvariants. Drop the parenthetical (or retitle to(annotate mode)). Pure hygiene, not a merge blocker.No architectural objection. Ship after the title tidy if convenient.
⚙️ Tobias Wendt — DevOps & Platform Engineer
✅ Approved
Frontend-only change, no infra/CI/Compose/observability surface touched. I reviewed it purely for CI gate behaviour and e2e reliability — that's the only thing in my lane here, and it holds up.
What I checked
No CI pipeline impact
docker-compose*.yml, Caddyfile, env vars, or any infra config. Diff is 3 spec files + 1 component. Nothing here changes how the build/lint/test jobs provision or run.e2e flow is correctly wired (not a silent no-op)
The repointed
admin can delete an annotationtest depends on the kept keyboard path actually firing in CI. I traced the wiring to confirm it's not a dead branch:AnnotationShape.svelte:90—if (e.key === 'Delete' && showDelete) onDeleteRequest?.()AnnotationLayer.svelte:117—showDelete={canDraw}documents.spec.ts:387), socanDraw(and thusshowDelete) istruebeforeDeleteis pressed. The<div>carriestabindex="0", solocator.press('Delete')lands focus correctly. Confirm label/^bestätigen$/imatchesbtn_confirminmessages/de.json:38. Flow is sound end-to-end.Ordering / flake assessment
playwright.config.ts—fullyParallel: false,workers: 1,retries: 2in CI. Tests in this file run sequentially in declaration order, so thedrawtest seeds an annotation before thedeletetest runs.countBeforeat runtime and assertscountBefore - 1rather than a hardcoded count, and gates on a visible annotation first (documents.spec.ts:380). Robust to how many annotations prior tests left behind. The cross-test data dependency (delete needs a prior draw) is pre-existing and unchanged by this PR.timeout: 8000and the canvas hydration wait pattern is preserved. No new arbitrary sleeps introduced.Suggestion (non-blocking)
The
admin can delete an annotationtest silently relies onadmin can draw an annotationhaving run first. Withretries: 2a flaky draw could cascade into a confusing delete failure on a fresh worker retry. Not introduced by this PR, but if you ever touch this block again, consider making the delete test self-seed its own annotation inbeforeEach(or wrapping the block intest.describe.serial) to make the dependency explicit. Today's setup works; this is just hardening.LGTM from the platform side — merge when QA/dev sign off.
👨💻 Felix Brandt — Fullstack Developer (SvelteKit · Spring Boot · Python)
⚠️ Approved with concerns
Clean, focused removal. The on-canvas
<button>and the now-orphaneddeleteVisiblederived are both gone, and I confirmed the retained wiring is genuinely live, not dead:PdfViewer→AnnotationLayer(onDeleteRequest,showDelete={canDraw}) →AnnotationShapekeyboard branch (if (e.key === 'Delete' && showDelete) onDeleteRequest?.(), line 90) → parenthandleAnnotationDeleteRequest→transcription.deleteAnnotation. SoshowDelete/onDeleteRequest/deleteAnnotationall earn their keep via the keyboard path. Good call keeping the negative-path test (does not call onDeleteRequest on Delete key when showDelete is false) — that branch is real.Two things I'd want addressed before merge.
Blockers
None.
Suggestions
1. Stale test that now lies —
AnnotationLayer.svelte.test.ts:243(untouched by this PR)There is a second test file for this component that the PR didn't catch:
The
(delete button visible)description is now factually wrong —canDraw=trueno longer renders any delete button. The test still passes (it only assertsnot.toThrow()), so CI stays green, but the description actively misleads the next reader. Per my "dead code is deleted, comments explain why not what" rule, this needs a one-word fix: drop the parenthetical, e.g.renders without throwing when canDraw is true. While here — having bothAnnotationLayer.svelte.spec.tsandAnnotationLayer.svelte.test.tscovering the same component is a pre-existing smell, not yours to fix in this PR, but worth a follow-up issue.2. The rewritten absence assertions are near-vacuous
The three rewritten tests all hang on:
Since the
annotation-delete-{id}testid no longer exists anywhere in the component, this assertion is structurally incapable of failing for any prop combination — it's a tautology. It only guards against someone re-introducing that exact testid. That's a thin contract. It doesn't catch the real regression risk: a future dev re-adding an overlapping on-canvas control under a different selector/testid. Consider asserting against the shape of the regression instead — e.g. that the annotation element renders no descendant<button>(element().querySelector('button')is null) in annotate+active mode. That tests the actual invariant the issue cares about ("nothing obscures the text"), not a string that you yourself just deleted. The keyboard tests (Deletefires / doesn't fire onshowDelete) are meaningful — keep those as-is.3. e2e repoint reads correctly —
e2e/documents.spec.tsfocus→Delete→Bestätigenexercises the kept path and assertscountBefore - 1, so it's not vacuous. No concern there; the updated comment matches behavior.Net: the production change is exactly what I'd write. The test rewrite is the soft spot — one stale description in an overlooked file, and absence assertions that are technically true but barely defend the invariant.
🔒 Nora "NullX" Steiner — Application Security Engineer
✅ Approved
Frontend-only change. Through the security lens, this PR removes presentation (a UI button) without touching a single authorization boundary. Nothing here changes who can delete what, or how the destructive action is gated. Clean.
What I checked
No authorization logic lived in the removed control. The deleted
<button>inAnnotationShape.svelteonly callede.stopPropagation()+onDeleteRequest?.(). There was never a client-side permission check on this button to begin with — removing it removes zero security enforcement. Good: the UI was correctly not the trust boundary.Server-side authz on delete is fully intact and untouched.
DELETE /api/documents/{documentId}/annotations/{annotationId}inAnnotationController.java:59-68carries@RequirePermission({Permission.ANNOTATE_ALL, Permission.WRITE_ALL}). The PR diff touches no backend file (PR metadata confirmschanged_files: 4, all underfrontend/). The real authorization gate is exactly where it should be — declarative AOP at the controller, not in Svelte. This is the property that matters, and it is preserved.Destructive-action confirmation is retained. The kept keyboard path is unchanged:
AnnotationShape.svelte:90(if (e.key === 'Delete' && showDelete) onDeleteRequest?.()) →AnnotationLayer.svelte:118→handleAnnotationDeleteRequestinroutes/documents/[id]/+page.svelte:60-67, which still awaitsconfirm({ title: m.transcription_block_delete_confirm(), destructive: true })and bails on!confirmedbefore callingtranscription.deleteAnnotation(). No accidental-deletion regression.No new data exposure / no client-trust assumption introduced. The change is a pure deletion of a render branch and one unused
$derived. No new fields surfaced, no new endpoint reachable, no input handling added. TheshowDeletegate on the kept Delete-key handler is a UX affordance, not a security control — and correctly so, since the backend@RequirePermissionis the enforcement layer regardless of what the client sends.Suggestions (non-blocking)
e2e/documents.spec.ts— focus annotation →Delete→ confirm "Bestätigen"). Good practice: the permission/confirm path still has end-to-end coverage. No action needed; just noting it as the right call.DELETE …/annotations/{id}returns 403 for aREAD_ALL-only user and 401 unauthenticated. The annotation could be deleted by any authenticated client hitting the API directly (the UI is irrelevant to that). The@RequirePermissionannotation is correct, but a@WithMockUserboundary test would lock the contract in permanently. Not a blocker for this PR — it changes no backend behaviour.LGTM. Deletion remains authz-gated server-side via
@RequirePermission({ANNOTATE_ALL, WRITE_ALL}), the destructive-action confirm dialog is retained, and no authorization logic ever lived in the removed UI button.📋 Elicit — Requirements Engineer & Business Analyst
⚠️ Approved with concerns
The change is faithful to issue #722: the redundant on-canvas control is gone, the keyboard path and its full wiring survive, and the three acceptance criteria are demonstrably met. My concerns are not blockers for this fix — they are unresolved requirements/NFR items the issue itself flags but leaves with no home in the backlog. I want them logged, not silently absorbed.
Acceptance criteria traceability
<button>block fully removed fromAnnotationShape.svelte; unuseddeleteVisiblederived removed; component + layer contract tests now assert the button never renders, even hovered+active incanDrawmode.Delete⇒ confirm dialog, cascade delete unchangedif (e.key === 'Delete' && showDelete) onDeleteRequest?.()is intact; both the positive test and the negativeshowDelete:falseguard test are retained; e2e repointed to focus →Delete→ "Bestätigen".TranscriptionBlock.sveltedelete (labelled, with confirm dialog) untouched; redundancy premise verified.The "why removal, not improve-in-place" argument is sound and well-evidenced: the 44 px target is a hard WCAG 2.2 floor that can't coexist with single-line box geometry, and the draw flow guarantees every user-created annotation has a panel-deletable block. No scope creep — the diff matches the declared file list, including the two folded-in spec/e2e items called out in the scope note.
Concerns (Suggestions — none blocking)
1. NFR-A11y — the retained
Deleteshortcut is a fully hidden affordance (Minor discoverability gap, but worth tracking).I verified the shortcut is documented nowhere: no mention in
routes/hilfe/transkription/+page.svelte, no tooltip, no on-screen cue anywhere in the transcription UI. Worse for the recognition-over-recall heuristic, the box'saria-labelis"Block anzeigen"("show block") — so a keyboard or screen-reader user gets an active contradiction: the element announces itself as "show", while it also silently destroys onDelete. The issue accepts this and defers a help-page line as out of scope. That deferral is reasonable for a bug fix, but it should become a tracked Should-have, not an inline footnote that evaporates on merge. Suggested follow-up issue:aria-keyshortcuts="Delete"on the box is a near-zero-cost partial mitigation that would at least make the binding machine-discoverable.2. Requirements debt — the orphan-annotation cleanup path is now removed with no tracking owner.
The PR/issue correctly classify orphan annotations (annotation with no block) as a backend-corruption-only edge case and accept losing the in-app cleanup route. I agree it shouldn't gate this UI fix. But "accepted trade-off" ≠ "requirement closed." Right now the residual risk has no home: if a partial backend failure ever produces an orphan, there is now zero in-app recovery and no documented operational alternative. Recommend a one-line Technical Debt register entry so the decision is auditable rather than buried in a closed PR:
A pointer to how an orphan would be remediated (manual SQL? admin endpoint?) in that entry would close the loop.
3. Minor —
showDelete/onDeleteRequestnow have a single consumer.With the visible button gone, the entire
showDelete → onDeleteRequest → deleteAnnotationchain exists solely to feed one keydown line. That's intentional and correct per the issue (it feeds the shortcut), so no change requested — just flagging that the prop nameshowDeletenow reads as a misnomer (it gates a key handler, shows nothing). A future rename to something likedeleteEnabledwould aid the next reader. Pure readability nit; out of scope here.Summary
Clean, well-scoped, well-tested bug fix that meets every acceptance criterion. Approving with the request that the two acknowledged-but-untracked items — the hidden-shortcut discoverability gap (#1) and the orphan-cleanup requirements debt (#2) — be captured as a follow-up issue and a tech-debt entry respectively, so accepted trade-offs stay visible in the backlog instead of disappearing on merge.
🧪 Sara Holt — QA Engineer & Test Strategist
⚠️ Approved with concerns
The behaviour change (removing the on-canvas delete button) is correctly captured: the deleted markup matches the removed assertions, the retained
Delete-key path is genuinely exercised at the unit layer, and the e2e was honestly repointed to the surviving deletion route. The concerns are about a vacuously-passing risk in the e2e and a focus-vs-synthetic-event mismatch between the unit test and the real DOM path. None block, but two are worth tightening before this is the only regression net for deletion.What's good (credit where due)
AnnotationShape.svelte.spec.ts:40renders with the strongest possible positive props (isHovered: true, isActive: true, showDelete: true) and asserts the button never appears — plus a positive control (annotation-ann-1is in the document). That positive control matters: it proves the testid query mechanism works, so the.not.toBeInTheDocument()isn't passing because the whole render silently failed. Same pattern inAnnotationLayer.svelte.spec.ts:103. Good defensive testing.if (e.key === 'Delete' && showDelete)(AnnotationShape.svelte:90) are tested: fires whenshowDelete: true(spec line 56) and does not fire whenshowDelete: false(line 76). That's the retained power-user path and it has real branch coverage — the PR's central claim ("keyboard Delete kept and unchanged") is test-backed.canDraw = transcribeMode && canAnnotate→showDelete(PdfViewer.svelte:289/AnnotationLayer.svelte:117) →onDeleteRequest(annotation.id)→handleAnnotationDeleteRequest(+page.svelte:60) →confirm({ destructive: true })→ ConfirmDialog renders a button labelledm.btn_confirm()= "Bestätigen" (ConfirmDialog.svelte:57,messages/de.json:38) →transcription.deleteAnnotation. So the e2e'sgetByRole('button', { name: /^bestätigen$/i })selector is correct and resolves to the real confirm button. The repointed flow does exercise actual deletion, not a no-op.Blockers
None.
Suggestions
Unit test dispatches a synthetic event; the e2e relies on real focus — neither proves the focus contract.
AnnotationShape.svelte.spec.ts:70-71doesannotationEl.dispatchEvent(new KeyboardEvent('keydown', { key: 'Delete', bubbles: true }))— this fires the handler regardless of focus, so it does not verify that the element is focusable/focused when Delete is pressed. The e2e (documents.spec.ts:392-393) doesannotation.click()thenannotation.press('Delete'), which does depend on the click leaving focus on therole="button"div. The div hastabindex="0"(AnnotationShape.svelte:85) so this should hold — but the only thing guarding that "click focuses, then Delete deletes" contract is now a single e2e. If a future refactor moves focus on click (e.g. opens the panel and focuses an input), the unit test stays green and only the e2e catches it. Consider one browser-mode unit test that does a real.click()then.press('Delete')on the rendered shape to lock the focus contract at the fast layer.The e2e is order-dependent and the count assertion can mask a wrong-element delete.
countBefore - 1(documents.spec.ts:397) only proves the count dropped by one — it passes even if the wrong annotation were deleted, and it silently depends on the earlierdraw/persiststests having created at least one annotation. If those tests fail or are run in isolation (--grep "delete"),countBeforeis 0 and the assertion becomestoHaveCount(-1)which never settles → an 8s timeout, reported as a delete bug rather than a fixture-ordering bug. Two cheap hardenings: (a) assertcountBefore >= 1up front with a clear message so the failure mode is legible; (b) capture the deleted annotation'sdata-testidbefore pressing Delete and additionally assert that specific testid is gone — turns a count check into an identity check.No coverage that confirm-cancel aborts the delete. The
if (!confirmed) return;branch (+page.svelte:65) is now the only thing standing between a stray Delete keypress and data loss, and it has no test. A unit/integration test ofhandleAnnotationDeleteRequestwith the confirm stub returningfalseassertingdeleteAnnotationis not called would protect the destructive-action guard. Worth adding since the canvas button (which had its own click-isolation test,stopPropagation) is gone and the keyboard path is faster to trigger accidentally.Minor — dead prop surface.
onDeleteRequest/showDeleteremain onAnnotationShapepurely for the keyboard branch, which is fine, but there's now no longer any test assertingonclickis not fired alongside a delete (the olddoes not call onclick when delete button is clickedtest was correctly dropped with the button). Pressing Delete also runs throughonkeydownonly, soonclickcan't fire — no action needed, just noting the isolation guarantee that test used to provide is no longer relevant.Net: the test changes faithfully follow the behaviour change and the contract tests are well-constructed (positive controls, both keyboard branches). My only real worry is concentration of risk — deletion now has one e2e doing the real focus→Delete→confirm dance, and that e2e's count assertion is both order-fragile and identity-blind. Suggestions 1–3 move that risk back down the pyramid where it's cheap.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate
🚫 Changes requested
Removing the overlapping on-canvas delete button is the right call — a 44px circle pinned over single-line boxes obscured the very document text people are reading, and that geometry was genuinely unwinnable. I fully support the removal. But the PR leaves the replacement deletion paths in a state that fails my hardest-constraint test: a 67-year-old transcribing on a tablet, no keyboard attached. After this change, can they still delete a mis-drawn box, and can they discover how? Right now the honest answer is "barely, and not discoverably."
Blockers
1. The only touch-reachable delete path is a 16px target — fails WCAG 2.2 (2.5.8 Target Size).
frontend/src/lib/document/transcription/TranscriptionBlock.svelte:255-274With the canvas button gone, the panel delete button becomes the sole deletion route for any touch user. But it is icon-only at
h-4 w-4(16×16px) with no padding-driven hit area:16px is roughly a third of the 44×44px CSS minimum WCAG 2.2 mandates, and well below the 48px I want for the 60+ audience. The PR description leans on "the right-hand panel already provides a non-overlapping, labelled delete" as the justification for removal — but that fallback is itself not tablet-usable. We're trading an overlapping-but-tappable control for a non-overlapping-but-untappable one.
Fix: give the panel delete button a real target before this lands:
(The
-mr-2keeps the visual icon position while the hit area grows outward. Same treatment applies to the review and comment buttons beside it — they share the same undersized footprint — but the delete button is the blocker because it is now load-bearing.)2. The kept Delete-key path has no discoverable affordance — invisible to everyone who isn't already a power user.
frontend/src/lib/document/annotation/AnnotationShape.svelte:88-91This is a genuinely good, non-overlapping path — but it is completely hidden. There is no tooltip, no hint text, no
aria-keyshortcuts, and a grep ofmessages/de.jsonconfirms no "Entf/Delete" hint string exists anywhere. A keyboard-only user (and our 60+ laptop transcribers are exactly that cohort) has no way to learn this exists. An invisible shortcut is not an affordance; it's a secret. The PR body calls it a "power-user path," which is fine as a supplement — but it cannot be the design's answer to "how does a keyboard user delete," because nothing tells them it's there.Fix (pick at least one, ideally both):
aria-keyshortcuts="Delete"to the annotation<div>so assistive tech announces it on focus.de/en/esi18n key for it.Suggestions
3. Confirm dialog focus + dual-audience copy. The e2e test now drives focus →
Delete→ click "Bestätigen". Please verify the confirm dialog moves focus to itself and traps it, and that "Bestätigen"/"Abbrechen" buttons themselves meet 44px — otherwise we've moved the touch-target problem one dialog downstream.4. Add the touch path to the e2e, not just the keyboard path.
frontend/e2e/documents.spec.ts:383+now exercises only theDeletekey. Since the panel button is the only touch route, add a test that taps the panel delete button at a mobile viewport (375px) to lock in that this path keeps working — otherwise the regression that matters most for tablet transcribers is untested.Net: the removal is correct and I want it merged — but not until the surviving paths actually serve the audience the canvas button was protecting. Fix the 16px panel target (Blocker 1) and give the Delete key a visible/announced cue (Blocker 2), and this becomes a clean approve.
— Leonie
🔧 Review fixes pushed (
73237b95,de804a45)Addressed the review findings:
🎨 Leonie — Blocker 1 (touch target): fixed.
TranscriptionBlock.svelte— both icon-only footer actions (Delete and the Review toggle) now have a ≥44×44 px hit area (min-h-[44px] min-w-[44px] inline-flex items-center justify-centerwith negative margins to preserve row layout); icon glyphs unchanged. Added a browser test asserting both buttons render ≥44 px in each dimension.🎨 Leonie — Blocker 2 (visible keyboard-shortcut hint): deferred to #327, not fixed here. By decision, the discoverability work for the kept
Deleteshortcut (visible hint +aria-keyshortcuts+ i18n + cheatsheet row) belongs with the transcribe keyboard-shortcuts feature, not this button-removal PR — adding a visible hint here would risk re-introducing the on-canvas overlay this PR set out to remove. Folded into #327 (added aDeleterow to its shortcut table and an addendum section).👨💻 Felix / 🏛️ Architect — stale test title:
AnnotationLayer.svelte.test.ts:243retitled(delete button visible)→(no delete button).👨💻 Felix / 🧪 Sara — near-vacuous absence assertions: both "never renders a delete button" contract tests now also assert the annotation element has zero descendant
<button>in annotate+active mode (the removed-testid check stays as a positive control).🧪 Sara / ⚙️ Tobias — e2e fragility:
admin can delete an annotationnow guardsexpect(countBefore).toBeGreaterThan(0)and adds an identity check (the specific deleted annotation'sdata-testidis gone after deletion), alongside the count assertion.Out of scope (acknowledged, not done here):
.spec.ts/.test.tsduplication cleanup;showDeleterename; backend authz boundary test; orphan-annotation tech-debt register entry.Verified clean on touched files:
npm run check, Prettier, ESLint. Browser/e2e specs run in CI.