fix(stammbaum): order keyboard tab stops by visual layout, not DB order (#718) #720

marcel · 2026-06-02T20:52:32+02:00

marcel commented

2026-06-02 20:52:32 +02:00

Closes #718.

Problem

On /stammbaum, pressing Tab between person nodes jumped around the canvas with no relation to on-screen position — focus walked the nodes array order (backend/DB row order) while the eye walks the visual grid. This failed WCAG 2.1 — 2.4.3 Focus Order (Level A) (and co-failed 1.3.2 Meaningful Sequence for AT reading order).

Root cause: tab order is DOM-source-order driven, and the node {#each} iterated the unsorted nodes array. The spatial layout from buildLayout() was only ever fed into each node's SVG transform, never back into iteration order.

Fix

Render the node loop in reading order. Added a nodesInReadingOrder $derived.by in StammbaumTree.svelte that sorts a copy of nodes by layout y (top generation first), then x (left-to-right within a row):

A missing layout position sorts last (mirrors the existing {#if pos} render guard) — the comparator never dereferences undefined.
Final tie-break on node.id makes the comparator total and deterministic.
The node {#each} iterates this derived list.

Shift+Tab and reload-stability fall out for free: reversed render order reverses backward tabbing, and x/y are independent of backend row order (buildLayout sorts each generation alphabetically before packing).

A load-bearing comment documents the dependency on uniform per-generation y (y = g * (NODE_H + ROW_GAP)), so a future vertical-stagger change won't silently regress tab order.

Acceptance criteria

✅ Canvas → leftmost node of the topmost generation, then nodes in reading order
✅ Tab moves left-to-right within a generation
✅ Tab drops to the next generation after finishing a row
✅ Shift+Tab reverses the same order
✅ Focus order stable across reloads

Untouched (as the issue notes): canvas/controls/side-panel tab stops, the {#if pos} guard, the connector paint block, the generation rail.

Tests

Two focused tests in StammbaumTree.svelte.test.ts, scoped to in-SVG role="button" elements (canvas + controls + side panel are also focusable, so whole-page Tab counts would be brittle):

renders node tab stops in reading order, not backend array order — scrambled input, asserts explicit sequence Eugenie, Walter, Clara, Hans.
orders tab stops by rendered position regardless of input order — self-validating: DOM order equals the same nodes sorted by their rendered (y, x) transform.

Written red first (38 other tests passing), then green: 40 passed (40) on --project=client. Prettier + ESLint clean. svelte-check shows only the pre-existing project-wide baseline; none in changed files.

No docs needed (no DB/route/package/error-code/permission change).

🤖 Generated with Claude Code

Closes #718. ## Problem On `/stammbaum`, pressing `Tab` between person nodes jumped around the canvas with no relation to on-screen position — focus walked the `nodes` array order (backend/DB row order) while the eye walks the visual grid. This failed **WCAG 2.1 — 2.4.3 Focus Order (Level A)** (and co-failed 1.3.2 Meaningful Sequence for AT reading order). Root cause: tab order is DOM-source-order driven, and the node `{#each}` iterated the unsorted `nodes` array. The spatial layout from `buildLayout()` was only ever fed into each node's SVG transform, never back into iteration order. ## Fix Render the node loop in reading order. Added a `nodesInReadingOrder` `$derived.by` in `StammbaumTree.svelte` that sorts a copy of `nodes` by layout **y** (top generation first), then **x** (left-to-right within a row): - A missing layout position sorts **last** (mirrors the existing `{#if pos}` render guard) — the comparator never dereferences `undefined`. - Final tie-break on `node.id` makes the comparator total and deterministic. - The node `{#each}` iterates this derived list. `Shift+Tab` and reload-stability fall out for free: reversed render order reverses backward tabbing, and x/y are independent of backend row order (`buildLayout` sorts each generation alphabetically before packing). A **load-bearing comment** documents the dependency on uniform per-generation `y` (`y = g * (NODE_H + ROW_GAP)`), so a future vertical-stagger change won't silently regress tab order. ## Acceptance criteria - ✅ Canvas → leftmost node of the topmost generation, then nodes in reading order - ✅ Tab moves left-to-right within a generation - ✅ Tab drops to the next generation after finishing a row - ✅ Shift+Tab reverses the same order - ✅ Focus order stable across reloads Untouched (as the issue notes): canvas/controls/side-panel tab stops, the `{#if pos}` guard, the connector paint block, the generation rail. ## Tests Two focused tests in `StammbaumTree.svelte.test.ts`, scoped to in-SVG `role="button"` elements (canvas + controls + side panel are also focusable, so whole-page Tab counts would be brittle): 1. `renders node tab stops in reading order, not backend array order` — scrambled input, asserts explicit sequence `Eugenie, Walter, Clara, Hans`. 2. `orders tab stops by rendered position regardless of input order` — self-validating: DOM order equals the same nodes sorted by their rendered (y, x) transform. Written red first (38 other tests passing), then green: `40 passed (40)` on `--project=client`. Prettier + ESLint clean. `svelte-check` shows only the pre-existing project-wide baseline; none in changed files. No docs needed (no DB/route/package/error-code/permission change). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 1 commit 2026-06-02 20:52:32 +02:00

fix(stammbaum): order keyboard tab stops by visual layout, not DB order (#718 )

CI / Unit & Component Tests (pull_request) Successful in 3m26s

Details

CI / OCR Service Tests (pull_request) Successful in 22s

Details

CI / Backend Unit Tests (pull_request) Successful in 3m40s

Details

CI / fail2ban Regex (pull_request) Successful in 46s

Details

CI / Semgrep Security Scan (pull_request) Successful in 22s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m5s

Details

eab4153b97

Person nodes rendered in `nodes` array order (backend/DB row order), so
Tab focus hopped between nodes unrelated to their on-screen position,
failing WCAG 2.4.3 Focus Order (Level A).

Render the node loop in reading order instead: sort by layout y (top
generation first) then x (left-to-right within a row), via a
`nodesInReadingOrder` derived. Nodes without a layout position sort last
(mirroring the `{#if pos}` guard); node.id is the final tie-break for a
total, deterministic comparator. Shift+Tab and reload-stability fall out
for free (reversed render order; x/y independent of backend order).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-06-02 20:53:48 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

I reviewed this purely through the architecture lens: layer boundaries, coupling, module responsibilities, separation of concerns, and documentation currency. This is a tight, single-responsibility change and it lands cleanly.

What I checked

Layer boundaries / separation of concerns — The fix lives entirely in the presentation layer (StammbaumTree.svelte). Spatial layout remains owned by the buildLayout module; the new nodesInReadingOrder $derived.by consumes layout.positions (the module's published output) rather than reimplementing rank/packing logic. That is the correct direction of dependency: the view reads from the layout engine, the layout engine knows nothing about iteration/DOM concerns. No business or layout logic leaked into the component.
Coupling — The only new coupling is StammbaumTree → layout.positions, which already existed (the {@const pos = layout.positions.get(node.id)} render guard reads the same map). No new module-to-module edges, no cross-domain reach-through, no shared/duplicated types. The comparator's undefined → Infinity fallback deliberately mirrors the existing {#if pos} guard, so the two stay semantically aligned.
Single source of truth — Good that you did not add a parallel sort inside buildLayout and instead derived ordering from its existing geometry. One place computes positions; the view orders by them. The localeCompare(id) final tie-break makes the comparator total/deterministic, which protects reload stability without a second source of ordering.
Documentation currency — Walked the full doc-trigger table: no Flyway migration, no join table/FK, no new backend package/controller/service, no new SvelteKit route, no Docker service, no external system, no new ErrorCode/Permission, no new domain term, no decision with lasting structural consequence. None of the rows fire. The PR's "no docs needed" claim is correct — confirmed, not taken on faith.
Testability — Tests assert against the published behavioural contract (DOM order vs. rendered transform), not internals. The second, self-validating test (orders tab stops by rendered position regardless of input order) is the architecturally robust one: it survives any future layout-internal reshuffle, so it won't ossify the alphabetical assumption. Nice.

Blockers

None.

Suggestions

The LOAD-BEARING comment is doing real architectural work. It documents that the tab-order sort depends on uniform per-generation y (y = g * (NODE_H + ROW_GAP)). That dependency lives in buildLayout.ts but the comment lives in the component — exactly the kind of cross-file invariant that quietly rots. Not a blocker, but I'd consider a one-line back-reference comment at the const y = g * (NODE_H + ROW_GAP); site in buildLayout.ts (e.g. "tab order in StammbaumTree depends on uniform y per generation, #718") so a future vertical-stagger change is warned at the point of change, not only at the point of consumption. This is the lightweight alternative to a full ADR — the invariant isn't weighty enough to warrant one.

Solid, boring, well-scoped. Approved.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** I reviewed this purely through the architecture lens: layer boundaries, coupling, module responsibilities, separation of concerns, and documentation currency. This is a tight, single-responsibility change and it lands cleanly. ### What I checked - **Layer boundaries / separation of concerns** — The fix lives entirely in the presentation layer (`StammbaumTree.svelte`). Spatial layout remains owned by the `buildLayout` module; the new `nodesInReadingOrder` `$derived.by` *consumes* `layout.positions` (the module's published output) rather than reimplementing rank/packing logic. That is the correct direction of dependency: the view reads from the layout engine, the layout engine knows nothing about iteration/DOM concerns. No business or layout logic leaked into the component. - **Coupling** — The only new coupling is `StammbaumTree` → `layout.positions`, which already existed (the `{@const pos = layout.positions.get(node.id)}` render guard reads the same map). No new module-to-module edges, no cross-domain reach-through, no shared/duplicated types. The comparator's `undefined → Infinity` fallback deliberately mirrors the existing `{#if pos}` guard, so the two stay semantically aligned. - **Single source of truth** — Good that you did *not* add a parallel sort inside `buildLayout` and instead derived ordering from its existing geometry. One place computes positions; the view orders by them. The `localeCompare(id)` final tie-break makes the comparator total/deterministic, which protects reload stability without a second source of ordering. - **Documentation currency** — Walked the full doc-trigger table: no Flyway migration, no join table/FK, no new backend package/controller/service, no new SvelteKit route, no Docker service, no external system, no new `ErrorCode`/`Permission`, no new domain term, no decision with lasting structural consequence. None of the rows fire. The PR's "no docs needed" claim is correct — confirmed, not taken on faith. - **Testability** — Tests assert against the published behavioural contract (DOM order vs. rendered transform), not internals. The second, self-validating test (`orders tab stops by rendered position regardless of input order`) is the architecturally robust one: it survives any future layout-internal reshuffle, so it won't ossify the alphabetical assumption. Nice. ### Blockers None. ### Suggestions - **The LOAD-BEARING comment is doing real architectural work.** It documents that the tab-order sort depends on uniform per-generation `y` (`y = g * (NODE_H + ROW_GAP)`). That dependency lives in `buildLayout.ts` but the comment lives in the component — exactly the kind of cross-file invariant that quietly rots. Not a blocker, but I'd consider a one-line back-reference comment at the `const y = g * (NODE_H + ROW_GAP);` site in `buildLayout.ts` (e.g. "tab order in StammbaumTree depends on uniform y per generation, #718") so a future vertical-stagger change is warned at the point of change, not only at the point of consumption. This is the lightweight alternative to a full ADR — the invariant isn't weighty enough to warrant one. Solid, boring, well-scoped. Approved.

marcel commented

2026-06-02 20:54:16 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

LGTM from the infra/ops chair — nothing in my domain to flag. This is a pure frontend change: a $derived.by sort added to StammbaumTree.svelte plus two scoped Vitest cases. True changeset against the merge base is exactly 2 files, +147/-2.

What I checked (all clean / untouched):

CI workflows & runners — no changes to .gitea/workflows/* or composite actions. Existing vitest --project=client job covers the new tests; no new job, no pipeline reconfiguration needed.
Dependencies — package.json / package-lock.json untouched. No new transitive deps, no supply-chain surface, nothing for Renovate to track.
Build config — no vite.config.ts, eslint.config.js, or tsconfig changes. No build-time or bundle-size impact (a comparator on an in-memory array).
Docker / Compose / Caddy — not touched. No image tags, volumes, ports, or reverse-proxy rules involved.
DB / migrations — no Flyway migration, no schema change, no runtime/ops risk.
Secrets / env vars — none introduced; nothing logged.
Observability — no metrics, traces, or log lines affected; this is a render-order change with zero runtime cost worth instrumenting.

Blockers

None.

Suggestions

Minor, non-blocking, and outside my lane: the two browser-scoped tests run under --project=client, which (per project convention) is CI-trusted rather than reliably runnable locally. That's consistent with the rest of the suite, so no action needed — just noting that the regression guard lives entirely in CI, which is the right place for it.

Ship it.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** LGTM from the infra/ops chair — nothing in my domain to flag. This is a pure frontend change: a `$derived.by` sort added to `StammbaumTree.svelte` plus two scoped Vitest cases. True changeset against the merge base is exactly 2 files, +147/-2. What I checked (all clean / untouched): - **CI workflows & runners** — no changes to `.gitea/workflows/*` or composite actions. Existing `vitest --project=client` job covers the new tests; no new job, no pipeline reconfiguration needed. - **Dependencies** — `package.json` / `package-lock.json` untouched. No new transitive deps, no supply-chain surface, nothing for Renovate to track. - **Build config** — no `vite.config.ts`, `eslint.config.js`, or tsconfig changes. No build-time or bundle-size impact (a comparator on an in-memory array). - **Docker / Compose / Caddy** — not touched. No image tags, volumes, ports, or reverse-proxy rules involved. - **DB / migrations** — no Flyway migration, no schema change, no runtime/ops risk. - **Secrets / env vars** — none introduced; nothing logged. - **Observability** — no metrics, traces, or log lines affected; this is a render-order change with zero runtime cost worth instrumenting. ### Blockers None. ### Suggestions - Minor, non-blocking, and outside my lane: the two browser-scoped tests run under `--project=client`, which (per project convention) is CI-trusted rather than reliably runnable locally. That's consistent with the rest of the suite, so no action needed — just noting that the regression guard lives entirely in CI, which is the right place for it. Ship it.

marcel commented

2026-06-02 20:54:19 +02:00

👨‍💻 Felix Brandt — Fullstack Developer

Verdict: ✅ Approved

Small, surgical, well-tested fix. I checked TDD evidence, the comparator's correctness/determinism, the Svelte 5 reactivity rules, and the load-bearing comment against the actual buildLayout source. It holds up.

What I verified

Comparator is total and deterministic (StammbaumTree.svelte:84-96). y → x → id.localeCompare tie-break. A missing layout position resolves to Infinity, so it sorts last and the comparator never dereferences undefined. That mirrors the {#if pos} render guard at line 343, so an unplaced node can't sneak a g[role="button"] ahead of a placed one. Good.
Svelte 5 rules respected. nodesInReadingOrder is a $derived.by (not a $state+$effect recompute), and the {#each ... (node.id)} key is preserved — reordering won't corrupt the per-node focused state in StammbaumNode. This is exactly the case where a stale key would have bitten you, and it's keyed correctly.
The load-bearing comment is accurate. buildLayout assigns y = g * (NODE_H + ROW_GAP) uniformly per generation (buildLayout.ts:92), so the y-then-x sort genuinely encodes reading order today, and the comment correctly names the one assumption (no vertical stagger) that would regress it. This is a why comment, not a what comment — the kind I want.
Tests are meaningful, not vacuous. The first test (StammbaumTree.svelte.test.ts) feeds a deliberately scrambled gen1→gen0→gen1→gen0 input and asserts the explicit ['Eugenie', 'Walter', 'Clara', 'Hans'] sequence. The second is self-validating: it reads the rendered transform off each g[role="button"] and asserts DOM order equals the same nodes sorted by (y, x), so it survives layout-internal reordering without re-encoding the alphabetical assumption. Two complementary angles on one behavior — nicely done. The aria-label-split-on-, extraction is safe here since the test nodes carry no birth/death years.

Blockers

None.

Suggestions (non-blocking)

Red-first is claimed but not visible in history. The branch is one squashed commit (eab4153b), so I can't independently confirm the test was red before the {#each} swap. I trust the PR body's 40 passed (40) note and the tests are clearly non-trivial — just flagging that for a fix this WCAG-relevant, a separate red commit would have left the proof in the tree. Not asking you to re-do it.
Tiny perf note, no action needed. The comparator does two layout.positions.get() lookups per comparison, i.e. O(n log n) map reads on every layout change. For genealogy-sized graphs this is noise. If a tree ever gets large, precomputing an id → {x,y} snapshot once before sorting would drop it to O(n) lookups — but that's premature today. KISS wins here.

Acceptance criteria all fall out of this one change correctly: Shift+Tab reversal and reload-stability are genuine free consequences of reordering DOM source order, since x/y are independent of backend row order. Approved.

## 👨‍💻 Felix Brandt — Fullstack Developer **Verdict: ✅ Approved** Small, surgical, well-tested fix. I checked TDD evidence, the comparator's correctness/determinism, the Svelte 5 reactivity rules, and the load-bearing comment against the actual `buildLayout` source. It holds up. ### What I verified - **Comparator is total and deterministic** (`StammbaumTree.svelte:84-96`). y → x → `id.localeCompare` tie-break. A missing layout position resolves to `Infinity`, so it sorts last and the comparator never dereferences `undefined`. That mirrors the `{#if pos}` render guard at line 343, so an unplaced node can't sneak a `g[role="button"]` ahead of a placed one. Good. - **Svelte 5 rules respected.** `nodesInReadingOrder` is a `$derived.by` (not a `$state`+`$effect` recompute), and the `{#each ... (node.id)}` key is preserved — reordering won't corrupt the per-node `focused` state in `StammbaumNode`. This is exactly the case where a stale key would have bitten you, and it's keyed correctly. - **The load-bearing comment is accurate.** `buildLayout` assigns `y = g * (NODE_H + ROW_GAP)` uniformly per generation (`buildLayout.ts:92`), so the y-then-x sort genuinely encodes reading order today, and the comment correctly names the one assumption (no vertical stagger) that would regress it. This is a *why* comment, not a *what* comment — the kind I want. - **Tests are meaningful, not vacuous.** The first test (`StammbaumTree.svelte.test.ts`) feeds a deliberately scrambled gen1→gen0→gen1→gen0 input and asserts the explicit `['Eugenie', 'Walter', 'Clara', 'Hans']` sequence. The second is self-validating: it reads the rendered `transform` off each `g[role="button"]` and asserts DOM order equals the same nodes sorted by (y, x), so it survives layout-internal reordering without re-encoding the alphabetical assumption. Two complementary angles on one behavior — nicely done. The `aria-label`-split-on-`,` extraction is safe here since the test nodes carry no birth/death years. ### Blockers None. ### Suggestions (non-blocking) 1. **Red-first is claimed but not visible in history.** The branch is one squashed commit (`eab4153b`), so I can't independently confirm the test was red before the `{#each}` swap. I trust the PR body's `40 passed (40)` note and the tests are clearly non-trivial — just flagging that for a fix this WCAG-relevant, a separate red commit would have left the proof in the tree. Not asking you to re-do it. 2. **Tiny perf note, no action needed.** The comparator does two `layout.positions.get()` lookups per comparison, i.e. O(n log n) map reads on every layout change. For genealogy-sized graphs this is noise. If a tree ever gets large, precomputing an `id → {x,y}` snapshot once before sorting would drop it to O(n) lookups — but that's premature today. KISS wins here. Acceptance criteria all fall out of this one change correctly: Shift+Tab reversal and reload-stability are genuine free consequences of reordering DOM source order, since x/y are independent of backend row order. Approved.

marcel commented

2026-06-02 20:54:30 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

I reviewed this from an adversarial lens looking for injection, XSS, authorization, untrusted-input, and data-exposure issues. This is a client-side SVG render-ordering change with no security-relevant surface added. Clean from my side.

What I checked

No new XSS sink. The diff only reorders the {#each} iteration (nodes → nodesInReadingOrder). The node labels in StammbaumNode.svelte render person data via Svelte's auto-escaping interpolation — both {node.displayName} inside <text> (line 89) and aria-label="{node.displayName}{datesLabel}" (line 38) are HTML-attribute/text-content escaped by the compiler. Grep across the genealogy directory confirms no {@html}, innerHTML, insertAdjacentHTML, or eval. So even if a displayName carried "><script>... from the data layer, it cannot break out of the SVG text node or the attribute context. The reorder does not touch any of this — same sink, same escaping, just a different array order. (CWE-79: not present.)
No new data exposure. The same node set is rendered; only the sequence changes. Nothing previously hidden becomes visible, and no additional fields are read or emitted. The comparator reads layout.positions (x/y geometry) and node.id only — no sensitive data enters the sort key or any new DOM attribute.
Untrusted-input robustness of the comparator. Good defensive shape: missing layout positions sort to Infinity (last), mirroring the {#if pos} render guard, so the comparator never dereferences undefined. The node.id localeCompare tie-break makes it total and deterministic — no comparator-instability surprises, and no DoS-style pathological behavior on adversarial input ordering. [...nodes] copies before sorting, so the prop array isn't mutated in place (avoids surprising side effects on shared state).
No server/auth/injection surface. Pure frontend; no endpoint, query, permission annotation, CORS, or DB access changed. Data still arrives through the existing server load path — authorization is enforced upstream and is unaffected here.

Blockers

None.

Suggestions

None blocking. One forward-looking note (already half-covered by the author's LOAD-BEARING comment): the security/escaping posture here depends entirely on displayName never being rendered through a raw-HTML sink. If a future change ever wants rich-text names in the tree, route them through DOMPurify rather than {@html} — but that's hypothetical and out of scope for this PR.

Nice work on writing the tests red-first and scoping them to in-SVG role="button" elements. Approved.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** I reviewed this from an adversarial lens looking for injection, XSS, authorization, untrusted-input, and data-exposure issues. This is a client-side SVG render-ordering change with no security-relevant surface added. Clean from my side. ### What I checked - **No new XSS sink.** The diff only reorders the `{#each}` iteration (`nodes` → `nodesInReadingOrder`). The node labels in `StammbaumNode.svelte` render person data via Svelte's auto-escaping interpolation — both `{node.displayName}` inside `<text>` (line 89) and `aria-label="{node.displayName}{datesLabel}"` (line 38) are HTML-attribute/text-content escaped by the compiler. Grep across the genealogy directory confirms **no `{@html}`, `innerHTML`, `insertAdjacentHTML`, or `eval`**. So even if a `displayName` carried `"><script>...` from the data layer, it cannot break out of the SVG text node or the attribute context. The reorder does not touch any of this — same sink, same escaping, just a different array order. (CWE-79: not present.) - **No new data exposure.** The same node set is rendered; only the sequence changes. Nothing previously hidden becomes visible, and no additional fields are read or emitted. The comparator reads `layout.positions` (x/y geometry) and `node.id` only — no sensitive data enters the sort key or any new DOM attribute. - **Untrusted-input robustness of the comparator.** Good defensive shape: missing layout positions sort to `Infinity` (last), mirroring the `{#if pos}` render guard, so the comparator never dereferences `undefined`. The `node.id` `localeCompare` tie-break makes it total and deterministic — no comparator-instability surprises, and no DoS-style pathological behavior on adversarial input ordering. `[...nodes]` copies before sorting, so the prop array isn't mutated in place (avoids surprising side effects on shared state). - **No server/auth/injection surface.** Pure frontend; no endpoint, query, permission annotation, CORS, or DB access changed. Data still arrives through the existing server load path — authorization is enforced upstream and is unaffected here. ### Blockers None. ### Suggestions None blocking. One forward-looking note (already half-covered by the author's LOAD-BEARING comment): the security/escaping posture here depends entirely on `displayName` never being rendered through a raw-HTML sink. If a future change ever wants rich-text names in the tree, route them through DOMPurify rather than `{@html}` — but that's hypothetical and out of scope for this PR. Nice work on writing the tests red-first and scoping them to in-SVG `role="button"` elements. Approved.

marcel commented

2026-06-02 20:54:34 +02:00

📋 Elicit — Requirements Engineer / Business Analyst

Verdict: ✅ Approved

I reviewed this PR strictly against the acceptance criteria in issue #718 — does the change satisfy every AC, and is each one backed by evidence? My job here is traceability, not code aesthetics, so below is a scenario-by-scenario map from each Gherkin AC to the evidence in the diff and tests.

Traceability matrix (AC → evidence)

#	Acceptance criterion (#718)	Evidence	Status
AC1	Canvas first, then focus moves to leftmost node of the topmost generation	Canvas SVG (`tabindex=0`) still precedes nodes in DOM (untouched). `nodesInReadingOrder` sorts ascending by `y` then `x`, so the first node is top-generation, leftmost. Test 1 asserts the first in-SVG `role="button"` is `Eugenie`.	✅ Met
AC2	Tab moves left-to-right within a generation	Equal-`y` nodes tie-break on `x` ascending. Test 1 asserts `Eugenie, Walter` (gen 0, L→R).	✅ Met
AC3	Tab drops to next generation after finishing a row	`y` ascending orders gen 0 before gen 1. Test 1 asserts `Clara, Hans` follow gen 0.	✅ Met
AC4	Shift+Tab reverses the same order	No code needed — browser walks DOM in reverse; correct forward order guarantees correct reverse order. The issue itself confirms this falls out for free.	⚠️ Met by construction, not by a test (see Suggestions)
AC5	Focus order stable across reloads (independent of backend row order)	Sort key is layout `x`/`y`, which `buildLayout` derives from an alphabetical per-generation sort — independent of input array order. Test 2 is self-validating: it scrambles the input array and asserts DOM order equals the same nodes sorted by their rendered `(y, x)`.	✅ Met

Every Must-have behavioural AC is satisfied and four of the five are covered by an executing test. The two defensive requirements the issue called out explicitly are also honoured and visible in the diff:

Defensive comparator — missing layout positions sort to Infinity (last), mirroring the {#if pos} render guard, so the comparator never dereferences undefined. ✅ matches the issue's hard requirement.
Total / deterministic comparator — final tie-break on node.id. ✅
Load-bearing dependency documented — the comment naming the uniform per-generation y = g * (NODE_H + ROW_GAP) assumption is present, so a future vertical-stagger change won't silently regress tab order. This was an explicit ask in the issue and is satisfied. ✅

Scope discipline is good: the PR touches only the node iteration order and leaves the canvas/controls/side-panel tab stops, the {#if pos} guard, the connector block, and the generation rail untouched — exactly as the issue scoped it. No scope creep, no gold-plating.

Blockers

None. No acceptance criterion is unmet, and none is silently out of scope.

Suggestions

These are non-blocking — the behaviour is correct, but the verification trail has two soft spots a future regression could slip through:

AC4 (Shift+Tab) has no direct assertion. It's genuinely a browser guarantee given correct forward order, so I won't block on it — but there is no test that would fail if someone later introduced a positive tabindex or split nodes across DOM containers (either of which would break reverse order while leaving the forward test green). A one-line assertion that the reverse of nodesInReadingOrder equals the nodes sorted descending would close the loop cheaply. Defer if you trust the DOM-source-order invariant.
AC1's canvas→first-node boundary is asserted only by construction. Both tests deliberately scope to in-SVG role="button" elements (correct per the issue's testing note — whole-page Tab counts would be brittle). That means the "canvas is the first tab stop, then the leftmost node" relationship is verified by reasoning, not by a test. The issue itself accepts this trade-off, so this is informational only: if the canvas's tabindex were ever removed, no test here would catch it.
Single-generation / unpositioned-node edge cases are untested. The comparator's Infinity fallback for nodes missing a layout position is defensive code with no test exercising it. Low value to add (it mirrors a render guard that already drops those nodes), but worth a note in case the layout ever starts returning partial position maps.

Net: the requirement is fully and faithfully delivered, the two defensive constraints from the issue are honoured, and the load-bearing assumption is documented. Approved.

## 📋 Elicit — Requirements Engineer / Business Analyst **Verdict: ✅ Approved** I reviewed this PR strictly against the acceptance criteria in issue #718 — does the change satisfy *every* AC, and is each one backed by evidence? My job here is traceability, not code aesthetics, so below is a scenario-by-scenario map from each Gherkin AC to the evidence in the diff and tests. ### Traceability matrix (AC → evidence) | # | Acceptance criterion (#718) | Evidence | Status | |---|------|----------|--------| | AC1 | Canvas first, then focus moves to **leftmost node of the topmost generation** | Canvas SVG (`tabindex=0`) still precedes nodes in DOM (untouched). `nodesInReadingOrder` sorts ascending by `y` then `x`, so the first node is top-generation, leftmost. Test 1 asserts the first in-SVG `role="button"` is `Eugenie`. | ✅ Met | | AC2 | Tab moves **left-to-right within a generation** | Equal-`y` nodes tie-break on `x` ascending. Test 1 asserts `Eugenie, Walter` (gen 0, L→R). | ✅ Met | | AC3 | Tab **drops to next generation after finishing a row** | `y` ascending orders gen 0 before gen 1. Test 1 asserts `Clara, Hans` follow gen 0. | ✅ Met | | AC4 | **Shift+Tab reverses** the same order | No code needed — browser walks DOM in reverse; correct forward order guarantees correct reverse order. The issue itself confirms this falls out for free. | ⚠️ Met by construction, not by a test (see Suggestions) | | AC5 | Focus order **stable across reloads** (independent of backend row order) | Sort key is layout `x`/`y`, which `buildLayout` derives from an alphabetical per-generation sort — independent of input array order. Test 2 is self-validating: it scrambles the input array and asserts DOM order equals the same nodes sorted by their rendered `(y, x)`. | ✅ Met | Every Must-have behavioural AC is satisfied and four of the five are covered by an executing test. The two defensive requirements the issue called out explicitly are also honoured and visible in the diff: - **Defensive comparator** — missing layout positions sort to `Infinity` (last), mirroring the `{#if pos}` render guard, so the comparator never dereferences `undefined`. ✅ matches the issue's hard requirement. - **Total / deterministic comparator** — final tie-break on `node.id`. ✅ - **Load-bearing dependency documented** — the comment naming the uniform per-generation `y = g * (NODE_H + ROW_GAP)` assumption is present, so a future vertical-stagger change won't silently regress tab order. This was an explicit ask in the issue and is satisfied. ✅ Scope discipline is good: the PR touches only the node iteration order and leaves the canvas/controls/side-panel tab stops, the `{#if pos}` guard, the connector block, and the generation rail untouched — exactly as the issue scoped it. No scope creep, no gold-plating. ### Blockers None. No acceptance criterion is unmet, and none is silently out of scope. ### Suggestions These are non-blocking — the behaviour is correct, but the *verification trail* has two soft spots a future regression could slip through: 1. **AC4 (Shift+Tab) has no direct assertion.** It's genuinely a browser guarantee given correct forward order, so I won't block on it — but there is no test that would fail if someone later introduced a positive `tabindex` or split nodes across DOM containers (either of which would break reverse order while leaving the forward test green). A one-line assertion that the *reverse* of `nodesInReadingOrder` equals the nodes sorted descending would close the loop cheaply. Defer if you trust the DOM-source-order invariant. 2. **AC1's canvas→first-node boundary is asserted only by construction.** Both tests deliberately scope to in-SVG `role="button"` elements (correct per the issue's testing note — whole-page Tab counts would be brittle). That means the "canvas is the first tab stop, *then* the leftmost node" relationship is verified by reasoning, not by a test. The issue itself accepts this trade-off, so this is informational only: if the canvas's `tabindex` were ever removed, no test here would catch it. 3. **Single-generation / unpositioned-node edge cases are untested.** The comparator's `Infinity` fallback for nodes missing a layout position is defensive code with no test exercising it. Low value to add (it mirrors a render guard that already drops those nodes), but worth a note in case the layout ever starts returning partial position maps. Net: the requirement is fully and faithfully delivered, the two defensive constraints from the issue are honoured, and the load-bearing assumption is documented. Approved.

marcel commented

2026-06-02 20:54:55 +02:00

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate

Verdict: ✅ Approved

This is a clean, correct accessibility fix and it lands exactly where it should. I traced the geometry and the paint-order implications rather than taking the PR description on faith, and it holds up. WCAG 2.4.3 Focus Order (A) and 1.3.2 Meaningful Sequence (A) are both genuinely satisfied — not just nominally.

Why this is correct (not just plausible)

Focus order = DOM order in SVG. SVG has no tabindex-reordering subtlety here — every node <g> carries tabindex="0" and focus walks document source order. By reordering the {#each} to iterate nodesInReadingOrder (sorted by layout y, then x), Tab now sweeps top generation → left-to-right, which is exactly how a sighted user's eye reads the grid. Shift+Tab reversing for free is a real property of source-order tabbing, not hand-waving.
Screen-reader reading order improves too (1.3.2). This is the part that often gets forgotten when people fix "tab order" — but because the fix reorders the actual DOM (not a tabindex overlay), a screen reader linearizing the SVG now also encounters Eugenie → Walter → Clara → Hans in reading order. One change, both criteria. Good.
Reload stability. buildLayout sorts each generation alphabetically (buildLayout.ts:62-68) before packing, so x/y are independent of backend row order. Tab order is now deterministic across reloads — that matters for the 60+ researchers who build muscle memory.

Paint-order / visual-regression check (the thing I most wanted to verify)

Reordering an SVG {#each} changes paint order, and SVG has no z-index — later siblings paint on top. So I checked whether a node could now occlude a neighbor's focus ring. It cannot:

Nodes are packed on a non-overlapping grid: COL_GAP = 40px horizontal (enforced at buildLayout.ts:269), NODE_H + ROW_GAP = 136px vertical between rows. Cards never overlap, so their relative paint order is visually inert.
The focus ring is only a 3px halo (x="-3", width={NODE_W + 6} in StammbaumNode.svelte). 3px cannot reach across a 40px / 80px gap into a neighbor's box, so no neighbor can ever paint over it.
The ring renders only for the focused node ({#if focused}, per-node $state), so only one ring exists at a time — there's no two-rings-fighting-for-z scenario.
Connectors are still painted in their own <StammbaumConnectors> block before the nodes, untouched. The connectors-under-cards layering and the opaque card fill (the #703 comment in StammbaumNode) are both preserved.

Conclusion: zero visual regression. No focus-ring clipping, no connector bleed-through, no card overlap. I'm satisfied this is safe.

Blockers

None.

Suggestions (non-blocking, all minor)

The LOAD-BEARING comment is exactly right — keep that habit. Documenting that the y-then-x sort depends on uniform per-generation y is precisely the kind of tripwire that prevents a future vertical-stagger change from silently re-breaking 2.4.3. This is the single best thing in the diff. No action needed; called out as praise.
Consider a test that asserts the column-gap invariant the sort depends on. Both new tests are solid (one explicit sequence, one self-validating against rendered transforms). If you ever want belt-and-suspenders, a tiny unit test on buildLayout asserting x strictly increases within a row would catch a layout regression at its source rather than via the downstream tab-order test. Optional.
Focus-ring contrast — verify --c-focus-ring against both card states. Out of scope for this PR (you didn't touch the ring), but while focus traversal is now correct, the ring is only useful if it's visible. Please confirm --c-focus-ring clears 3:1 (WCAG 1.4.11 non-text contrast) against both --c-surface (unselected card) and --c-primary (selected card fill), in light and dark mode. The 2px stroke at a 6px radius is a reasonable target size. Track separately if it hasn't been checked.

Nice, surgical fix. Approved.

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate **Verdict: ✅ Approved** This is a clean, correct accessibility fix and it lands exactly where it should. I traced the geometry and the paint-order implications rather than taking the PR description on faith, and it holds up. WCAG **2.4.3 Focus Order (A)** and **1.3.2 Meaningful Sequence (A)** are both genuinely satisfied — not just nominally. ### Why this is correct (not just plausible) - **Focus order = DOM order in SVG.** SVG has no `tabindex`-reordering subtlety here — every node `<g>` carries `tabindex="0"` and focus walks document source order. By reordering the `{#each}` to iterate `nodesInReadingOrder` (sorted by layout `y`, then `x`), Tab now sweeps top generation → left-to-right, which is exactly how a sighted user's eye reads the grid. Shift+Tab reversing for free is a real property of source-order tabbing, not hand-waving. - **Screen-reader reading order improves too (1.3.2).** This is the part that often gets forgotten when people fix "tab order" — but because the fix reorders the actual DOM (not a `tabindex` overlay), a screen reader linearizing the SVG now also encounters Eugenie → Walter → Clara → Hans in reading order. One change, both criteria. Good. - **Reload stability.** `buildLayout` sorts each generation alphabetically (`buildLayout.ts:62-68`) before packing, so `x`/`y` are independent of backend row order. Tab order is now deterministic across reloads — that matters for the 60+ researchers who build muscle memory. ### Paint-order / visual-regression check (the thing I most wanted to verify) Reordering an SVG `{#each}` changes **paint order**, and SVG has no `z-index` — later siblings paint on top. So I checked whether a node could now occlude a neighbor's focus ring. It cannot: - Nodes are packed on a non-overlapping grid: `COL_GAP = 40px` horizontal (enforced at `buildLayout.ts:269`), `NODE_H + ROW_GAP = 136px` vertical between rows. Cards never overlap, so their relative paint order is visually inert. - The focus ring is only a **3px halo** (`x="-3"`, `width={NODE_W + 6}` in `StammbaumNode.svelte`). 3px cannot reach across a 40px / 80px gap into a neighbor's box, so no neighbor can ever paint over it. - The ring renders only for the focused node (`{#if focused}`, per-node `$state`), so only one ring exists at a time — there's no two-rings-fighting-for-z scenario. - Connectors are still painted in their own `<StammbaumConnectors>` block **before** the nodes, untouched. The connectors-under-cards layering and the opaque card fill (the `#703` comment in `StammbaumNode`) are both preserved. **Conclusion: zero visual regression.** No focus-ring clipping, no connector bleed-through, no card overlap. I'm satisfied this is safe. ### Blockers None. ### Suggestions (non-blocking, all minor) 1. **The LOAD-BEARING comment is exactly right — keep that habit.** Documenting that the `y`-then-`x` sort depends on uniform per-generation `y` is precisely the kind of tripwire that prevents a future vertical-stagger change from silently re-breaking 2.4.3. This is the single best thing in the diff. No action needed; called out as praise. 2. **Consider a test that asserts the column-gap invariant the sort depends on.** Both new tests are solid (one explicit sequence, one self-validating against rendered transforms). If you ever want belt-and-suspenders, a tiny unit test on `buildLayout` asserting `x` strictly increases within a row would catch a layout regression at its source rather than via the downstream tab-order test. Optional. 3. **Focus-ring contrast — verify `--c-focus-ring` against both card states.** Out of scope for this PR (you didn't touch the ring), but while focus traversal is now correct, the ring is only useful if it's *visible*. Please confirm `--c-focus-ring` clears 3:1 (WCAG 1.4.11 non-text contrast) against **both** `--c-surface` (unselected card) and `--c-primary` (selected card fill), in light **and** dark mode. The 2px stroke at a 6px radius is a reasonable target size. Track separately if it hasn't been checked. Nice, surgical fix. Approved.

marcel commented

2026-06-02 20:54:59 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The fix is minimal and correct, and the two tests are real-DOM (vitest-browser-svelte), correctly scoped to in-SVG g[role="button"] elements rather than a brittle whole-page Tab count — exactly the right call given the canvas/controls/side-panel are also focusable. The PR body's "tab order follows DOM source order" claim is the right thing to lock. But the suite locks the happy path and leaves every branch the comparator explicitly handles untested. None are correctness blockers for this change; one is a coverage gate concern.

Blockers

None. The fix ships behavior that the tests do constrain on the main path.

Suggestions

1. The comparator has three branches; the tests exercise one. The $derived.by comparator (StammbaumTree.svelte:84-96) has three decision points: ay !== by (y), ax !== bx (x), and the id.localeCompare tie-break. Tests 1 and 2 only walk the y-then-x path. Two branches are dead from a coverage standpoint:

Unpositioned node → Infinity (sorts last). The PR body sells "a missing layout position sorts last (mirrors the {#if pos} guard)" as a feature, but nothing proves it. Add a node with no layout position (e.g. an isolated familyMember with no edges, or a node id absent from layout.positions) and assert it renders last in g[role="button"] order — and that it doesn't crash the comparator (the whole point of the pa ? ... : Infinity guard). This is the branch most likely to regress silently.
The id tie-break. This is the one the LOAD-BEARING comment is explicitly worried about — same y, same x. Today nothing can reach it because buildLayout never co-locates two nodes, so the warning ("a future vertical-stagger change would silently regress tab order") has no test backing it. A focused test that constructs two positions with identical (y, x) and asserts the order falls back to id.localeCompare would make that comment enforceable instead of aspirational. If it's genuinely unreachable through the public component API, that's worth a one-line note — but then the comment is documenting an untestable invariant.

2. Determinism / degenerate inputs are unasserted. Per my edge-case checklist: there is no nodes: [] test (empty tree should render zero node buttons and not throw) and no single-node test for this describe block. There's a single-node viewBox test elsewhere (line 66) but nothing tying single-node to tab order. These are cheap and they're the inputs most likely to hit a real archive on first load (a person with no relations yet).

3. Test 2 is close to a tautology. orders tab stops by rendered position regardless of input order re-derives the sort key (a.y - b.y || a.x - b.x) from the same transform attributes the component just wrote, then asserts DOM order equals that sort. It will catch a partial sort regression, so it's not worthless — but it can never catch a bug in the (y, x) choice of key, because it mirrors the implementation's key exactly. Test 1's explicit literal ['Eugenie','Walter','Clara','Hans'] is the stronger test and the one I'd trust. Consider whether Test 2 earns its keep, or strengthen it to assert against an independently-computed expectation (e.g. generation index from edges) rather than the rendered transforms.

4. Shift+Tab is an acceptance criterion with no test. The PR body lists "Shift+Tab reverses the same order" as ✅ and argues it "falls out for free" from reversed render order. That reasoning is correct, but an AC checked by argument rather than assertion is exactly how a future tabindex change slips through. Since backward tabbing is pure DOM-order reversal, a one-liner asserting [...nodeLabelsInDomOrder()].reverse() is the intended Shift+Tab sequence would convert the claim into a guard. Lower priority than 1–2.

5. Brittleness note (acceptable, flagging for the record). Test 1 depends on buildLayout sorting each generation alphabetically (Eugenie before Walter). That's a layout-internal detail leaking into a tab-order test. The inline comment acknowledges it and Test 2 is layout-internal-agnostic, so the coupling is contained — no change required, just be aware that an alphabetical-sort change in buildLayout will turn Test 1 red for reasons unrelated to tab order.

Flakiness: none — both tests are fully synchronous, no timers, no async focus walk. Good.

Net: the main-path behavior is genuinely locked and the scoping is correct. My one real ask before this is "done" by my standards is branch coverage on the comparator (suggestion 1) — the unpositioned-last and id-tie-break branches are coded, documented as load-bearing, and completely unverified.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The fix is minimal and correct, and the two tests are real-DOM (`vitest-browser-svelte`), correctly scoped to in-SVG `g[role="button"]` elements rather than a brittle whole-page Tab count — exactly the right call given the canvas/controls/side-panel are also focusable. The PR body's "tab order follows DOM source order" claim is the right thing to lock. But the suite locks the happy path and leaves every branch the comparator explicitly handles untested. None are correctness blockers for *this* change; one is a coverage gate concern. ### Blockers None. The fix ships behavior that the tests do constrain on the main path. ### Suggestions **1. The comparator has three branches; the tests exercise one.** The `$derived.by` comparator (`StammbaumTree.svelte:84-96`) has three decision points: `ay !== by` (y), `ax !== bx` (x), and the `id.localeCompare` tie-break. Tests 1 and 2 only walk the y-then-x path. Two branches are dead from a coverage standpoint: - **Unpositioned node → `Infinity` (sorts last).** The PR body sells "a missing layout position sorts last (mirrors the `{#if pos}` guard)" as a feature, but nothing proves it. Add a node with no layout position (e.g. an isolated `familyMember` with no edges, or a node id absent from `layout.positions`) and assert it renders last in `g[role="button"]` order — and that it doesn't crash the comparator (the whole point of the `pa ? ... : Infinity` guard). This is the branch most likely to regress silently. - **The id tie-break.** This is the one the LOAD-BEARING comment is explicitly worried about — same `y`, same `x`. Today nothing can reach it because `buildLayout` never co-locates two nodes, so the warning ("a future vertical-stagger change would silently regress tab order") has no test backing it. A focused test that constructs two positions with identical (y, x) and asserts the order falls back to `id.localeCompare` would make that comment enforceable instead of aspirational. If it's genuinely unreachable through the public component API, that's worth a one-line note — but then the comment is documenting an untestable invariant. **2. Determinism / degenerate inputs are unasserted.** Per my edge-case checklist: there is no `nodes: []` test (empty tree should render zero node buttons and not throw) and no single-node test for *this* describe block. There's a single-node viewBox test elsewhere (line 66) but nothing tying single-node to tab order. These are cheap and they're the inputs most likely to hit a real archive on first load (a person with no relations yet). **3. Test 2 is close to a tautology.** `orders tab stops by rendered position regardless of input order` re-derives the sort key (`a.y - b.y || a.x - b.x`) from the same `transform` attributes the component just wrote, then asserts DOM order equals that sort. It will catch a *partial* sort regression, so it's not worthless — but it can never catch a bug in the (y, x) *choice* of key, because it mirrors the implementation's key exactly. Test 1's explicit literal `['Eugenie','Walter','Clara','Hans']` is the stronger test and the one I'd trust. Consider whether Test 2 earns its keep, or strengthen it to assert against an independently-computed expectation (e.g. generation index from edges) rather than the rendered transforms. **4. Shift+Tab is an acceptance criterion with no test.** The PR body lists "Shift+Tab reverses the same order" as ✅ and argues it "falls out for free" from reversed render order. That reasoning is correct, but an AC checked by argument rather than assertion is exactly how a future `tabindex` change slips through. Since backward tabbing is pure DOM-order reversal, a one-liner asserting `[...nodeLabelsInDomOrder()].reverse()` is the intended Shift+Tab sequence would convert the claim into a guard. Lower priority than 1–2. **5. Brittleness note (acceptable, flagging for the record).** Test 1 depends on `buildLayout` sorting each generation alphabetically (Eugenie before Walter). That's a layout-internal detail leaking into a tab-order test. The inline comment acknowledges it and Test 2 is layout-internal-agnostic, so the coupling is contained — no change required, just be aware that an alphabetical-sort change in `buildLayout` will turn Test 1 red for reasons unrelated to tab order. **Flakiness:** none — both tests are fully synchronous, no timers, no async focus walk. Good. Net: the main-path behavior is genuinely locked and the scoping is correct. My one real ask before this is "done" by my standards is **branch coverage on the comparator** (suggestion 1) — the unpositioned-last and id-tie-break branches are coded, documented as load-bearing, and completely unverified.

marcel commented

2026-06-03 07:47:30 +02:00

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate

Verdict: ✅ Approved

I reviewed this purely through the accessibility lens it claims to fix, and it lands cleanly. The a11y outcome is genuinely correct, and I checked the one thing that worried me — visual/paint-order side effects from reordering the SVG {#each} — and there is no regression.

WCAG 2.4.3 Focus Order (Level A) — Satisfied. SVG focus order follows DOM source order, and the node loop now iterates nodesInReadingOrder sorted by layout y then x. Tab now sweeps top generation left-to-right, drops a row, repeats. That is exactly the order a sighted keyboard user expects from the visual grid. The old DB-row order was a real Level A failure, not a nicety.
WCAG 1.3.2 Meaningful Sequence (Level A) — Also satisfied, and worth calling out explicitly since it's the screen-reader half of the same fix. Each node's aria-label is "{displayName}{dates}" on the <g role="button">, so an AT user reading sequentially now hears generations top-down, left-to-right — the genealogically meaningful sequence. Before this PR, the spoken order was DB noise. Good that the comparator drives both Tab order and AT reading order from one source of truth.
Shift+Tab — Falls out for free as the PR says; reversed DOM order reverses backward tabbing. No separate handling needed, correctly.
Determinism across reloads — The Infinity sort-last for missing positions plus the node.id.localeCompare final tie-break make the comparator total and stable. So focus order won't shimmer between reloads (an under-appreciated a11y property — keyboard users build muscle memory).

Paint-order / visual regression check (the part I actually had to verify)

SVG paints in document order, so reordering the loop changes z-stacking. I confirmed this is safe:

Connectors still paint first. StammbaumConnectors is rendered before the node {#each} and is untouched, so node cards continue to cover connector lines. The opaque-fill contract in StammbaumNode.svelte (the explicit "connectors drawn beneath the node never bleed through" comment) is unaffected.
No node overlap, so no focus-ring occlusion. The focus ring (StammbaumNode.svelte) is drawn at x=-3 y=-3, a 3px halo around each card. My concern was that a later-painted neighbour could clip that halo. It can't: buildLayout packs nodes with horizontal spacing and a ROW_GAP between generations, both vastly larger than 3px. Siblings and adjacent rows never come within the halo, so the focus ring is never overdrawn by a neighbour regardless of paint order.
Selected/dimmed visuals unchanged. Selection fill, accent bar, and lineage dim are per-node and position-independent, so reordering changes nothing visually.

Net: identical pixels, corrected focus and reading order. That's the ideal shape for an a11y fix.

Suggestions (non-blocking, all optional)

Document the focus-order contract where a designer/QA will see it, not only in code. The load-bearing comment about uniform per-generation y is excellent defensive engineering. Consider mirroring one line into the genealogy component's notes / an impl-ref row so the "Tab follows reading order" guarantee is a documented design invariant, not tribal knowledge living in a sort comparator. If a future vertical-stagger lands, the designer should know it must re-establish reading order.
Consider an automated a11y guard, not only the two unit tests. The two tests assert DOM order well. Since this is a recurring class of regression (any future change to nodes iteration could silently break it), an axe/Playwright focus-order assertion on /stammbaum in E2E would catch it at the page level. Not required for this PR — the unit tests are sufficient for the change as scoped.
Minor, future-facing: the canvas itself (role="img", tabindex=0) is the first stop before the nodes. That's correct and out of scope here. Just flagging that the overall stop sequence (canvas → first node → … → controls/side panel) should stay in that reading-coherent order if the surrounding chrome is ever re-laid-out.

Ship it. This is a textbook accessibility fix: correct against the actual success criteria, zero visual cost, and defended with a comment that names the assumption it depends on.

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate **Verdict: ✅ Approved** I reviewed this purely through the accessibility lens it claims to fix, and it lands cleanly. The a11y outcome is genuinely correct, and I checked the one thing that worried me — visual/paint-order side effects from reordering the SVG `{#each}` — and there is no regression. ### Why the a11y fix is correct - **WCAG 2.4.3 Focus Order (Level A)** — Satisfied. SVG focus order follows DOM source order, and the node loop now iterates `nodesInReadingOrder` sorted by layout `y` then `x`. Tab now sweeps top generation left-to-right, drops a row, repeats. That is exactly the order a sighted keyboard user expects from the visual grid. The old DB-row order was a real Level A failure, not a nicety. - **WCAG 1.3.2 Meaningful Sequence (Level A)** — Also satisfied, and worth calling out explicitly since it's the screen-reader half of the same fix. Each node's `aria-label` is `"{displayName}{dates}"` on the `<g role="button">`, so an AT user reading sequentially now hears generations top-down, left-to-right — the genealogically meaningful sequence. Before this PR, the spoken order was DB noise. Good that the comparator drives both Tab order *and* AT reading order from one source of truth. - **Shift+Tab** — Falls out for free as the PR says; reversed DOM order reverses backward tabbing. No separate handling needed, correctly. - **Determinism across reloads** — The `Infinity` sort-last for missing positions plus the `node.id.localeCompare` final tie-break make the comparator total and stable. So focus order won't shimmer between reloads (an under-appreciated a11y property — keyboard users build muscle memory). ### Paint-order / visual regression check (the part I actually had to verify) SVG paints in document order, so reordering the loop changes z-stacking. I confirmed this is safe: 1. **Connectors still paint first.** `StammbaumConnectors` is rendered before the node `{#each}` and is untouched, so node cards continue to cover connector lines. The opaque-fill contract in `StammbaumNode.svelte` (the explicit "connectors drawn beneath the node never bleed through" comment) is unaffected. 2. **No node overlap, so no focus-ring occlusion.** The focus ring (`StammbaumNode.svelte`) is drawn at `x=-3 y=-3`, a 3px halo around each card. My concern was that a later-painted neighbour could clip that halo. It can't: `buildLayout` packs nodes with horizontal spacing and a `ROW_GAP` between generations, both vastly larger than 3px. Siblings and adjacent rows never come within the halo, so the focus ring is never overdrawn by a neighbour regardless of paint order. 3. **Selected/dimmed visuals unchanged.** Selection fill, accent bar, and lineage dim are per-node and position-independent, so reordering changes nothing visually. Net: identical pixels, corrected focus and reading order. That's the ideal shape for an a11y fix. ### Suggestions (non-blocking, all optional) 1. **Document the focus-order contract where a designer/QA will see it, not only in code.** The load-bearing comment about uniform per-generation `y` is excellent defensive engineering. Consider mirroring one line into the genealogy component's notes / an `impl-ref` row so the "Tab follows reading order" guarantee is a documented design invariant, not tribal knowledge living in a sort comparator. If a future vertical-stagger lands, the designer should know it must re-establish reading order. 2. **Consider an automated a11y guard, not only the two unit tests.** The two tests assert DOM order well. Since this is a recurring class of regression (any future change to `nodes` iteration could silently break it), an `axe`/Playwright focus-order assertion on `/stammbaum` in E2E would catch it at the page level. Not required for this PR — the unit tests are sufficient for the change as scoped. 3. **Minor, future-facing:** the canvas itself (`role="img"`, `tabindex=0`) is the first stop before the nodes. That's correct and out of scope here. Just flagging that the overall stop sequence (canvas → first node → … → controls/side panel) should stay in that reading-coherent order if the surrounding chrome is ever re-laid-out. Ship it. This is a textbook accessibility fix: correct against the actual success criteria, zero visual cost, and defended with a comment that names the assumption it depends on.

marcel commented

2026-06-03 07:47:34 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The fix is small, correct, and the two tests are genuinely good. They assert user-visible behavior (rendered g[role="button"] order, aria-label, transform) rather than poking at the nodesInReadingOrder derived — exactly right. Scoping to in-SVG buttons (not whole-page Tab counts) is the correct call and well-justified in both the PR body and the test comment. Test 1 (explicit sequence on scrambled input) and Test 2 (self-validating against actual rendered transforms) are nicely complementary: one locks the literal expected order, the other locks the (y, x) invariant independent of the alphabetical assumption. No flakiness risk — both are synchronous, deterministic, browser-DOM reads with no timing.

That said, the comparator added in this PR has three deliberate, documented branches, and the tests exercise only one of them. The PR copy says the other behaviors "fall out for free" — but "falls out for free" is a hypothesis until a test pins it. The whole point of a regression test is to make a future refactor fail loudly. Right now several documented guarantees are unguarded.

Blockers

None. The behavior the PR primarily targets (reading-order tab stops) is locked by two solid tests, and the change is low-risk.

Suggestions (I'd want these before I'd call the behavior "locked")

No coverage for the unpositioned-node branch. The comparator's pa ? pa.y : Infinity / "sorts last" path is a deliberate code path mirroring the {#if pos} render guard, called out explicitly in the load-bearing comment — and it has zero tests. Add one node that buildLayout leaves without a position (e.g. an isolated node, or one filtered out of the layout) and assert it renders last among the role="button" groups (and, since {#if pos} guards rendering, confirm whether it renders at all). This is the branch most likely to silently break, so it's the one most worth a test.
Shift+Tab is asserted nowhere — only inferred. It's an explicit acceptance criterion ("✅ Shift+Tab reverses the same order") but the only thing that's actually verified is forward DOM order. Reverse tabbing is a browser-native consequence of DOM order, so I'm not asking for a real key-press simulation — but at minimum add a one-line assertion (or doc-comment in the test) making explicit that reverse(nodeLabelsInDomOrder()) is the backward order, so the AC maps to a concrete check rather than a claim in the PR description.
The node.id tie-break is untested. localeCompare is what makes the comparator total/deterministic — also called out in the comment. A test with two nodes sharing the same (y, x) (or a fabricated layout where x collides) asserting a stable id-ordered result would lock the determinism guarantee. Without it, the "stable across reloads" AC rests on an unverified branch.
Edge cases: empty tree and single node. No test renders nodes: [] (should produce zero role="button" groups, no throw — the [...nodes].sort on an empty array) or a single node (degenerate sort). These are cheap, fast, and the kind of input that shows up with a brand-new/empty family. Add them as guards.
Minor brittleness — aria-label coupling. (aria-label ?? '').split(',')[0] works only because display names contain no commas and the date suffix is comma-prefixed ({name}, {birth}–{death}). It's fine today, but it silently couples the test to the label format. A short helper comment, or asserting against displayName more directly, would make the intent obvious to the next maintainer.

Net: the primary behavior is well-tested and I'm comfortable approving. But the comparator added in this PR documents four guarantees (reading order, sort-last-when-unpositioned, total/deterministic tie-break, reload stability) and the suite currently nails one and a half of them. Closing the unpositioned-node and tie-break gaps (suggestions 1 and 3) would make this a textbook regression net rather than a happy-path snapshot.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The fix is small, correct, and the two tests are genuinely good. They assert **user-visible behavior** (rendered `g[role="button"]` order, aria-label, transform) rather than poking at the `nodesInReadingOrder` derived — exactly right. Scoping to in-SVG buttons (not whole-page Tab counts) is the correct call and well-justified in both the PR body and the test comment. Test 1 (explicit sequence on scrambled input) and Test 2 (self-validating against actual rendered transforms) are nicely complementary: one locks the literal expected order, the other locks the (y, x) invariant independent of the alphabetical assumption. No flakiness risk — both are synchronous, deterministic, browser-DOM reads with no timing. That said, the comparator added in this PR has **three deliberate, documented branches, and the tests exercise only one of them**. The PR copy says the other behaviors "fall out for free" — but "falls out for free" is a hypothesis until a test pins it. The whole point of a regression test is to make a future refactor fail loudly. Right now several documented guarantees are unguarded. ### Blockers None. The behavior the PR primarily targets (reading-order tab stops) is locked by two solid tests, and the change is low-risk. ### Suggestions (I'd want these before I'd call the behavior "locked") 1. **No coverage for the unpositioned-node branch.** The comparator's `pa ? pa.y : Infinity` / "sorts last" path is a deliberate code path mirroring the `{#if pos}` render guard, called out explicitly in the load-bearing comment — and it has **zero tests**. Add one node that `buildLayout` leaves without a position (e.g. an isolated node, or one filtered out of the layout) and assert it renders **last** among the `role="button"` groups (and, since `{#if pos}` guards rendering, confirm whether it renders at all). This is the branch most likely to silently break, so it's the one most worth a test. 2. **Shift+Tab is asserted nowhere — only inferred.** It's an explicit acceptance criterion ("✅ Shift+Tab reverses the same order") but the only thing that's actually verified is forward DOM order. Reverse tabbing is a browser-native consequence of DOM order, so I'm not asking for a real key-press simulation — but at minimum add a one-line assertion (or doc-comment in the test) making explicit that `reverse(nodeLabelsInDomOrder())` *is* the backward order, so the AC maps to a concrete check rather than a claim in the PR description. 3. **The `node.id` tie-break is untested.** `localeCompare` is what makes the comparator total/deterministic — also called out in the comment. A test with two nodes sharing the same (y, x) (or a fabricated layout where x collides) asserting a stable id-ordered result would lock the determinism guarantee. Without it, the "stable across reloads" AC rests on an unverified branch. 4. **Edge cases: empty tree and single node.** No test renders `nodes: []` (should produce zero `role="button"` groups, no throw — the `[...nodes].sort` on an empty array) or a single node (degenerate sort). These are cheap, fast, and the kind of input that shows up with a brand-new/empty family. Add them as guards. 5. **Minor brittleness — aria-label coupling.** `(aria-label ?? '').split(',')[0]` works only because display names contain no commas and the date suffix is comma-prefixed (`{name}, {birth}–{death}`). It's fine today, but it silently couples the test to the label format. A short helper comment, or asserting against `displayName` more directly, would make the intent obvious to the next maintainer. Net: the primary behavior is well-tested and I'm comfortable approving. But the comparator added in this PR documents four guarantees (reading order, sort-last-when-unpositioned, total/deterministic tie-break, reload stability) and the suite currently nails one and a half of them. Closing the unpositioned-node and tie-break gaps (suggestions 1 and 3) would make this a textbook regression net rather than a happy-path snapshot.

marcel merged commit e259908d6a into main

2026-06-03 07:55:47 +02:00

marcel deleted branch feat/issue-718-stammbaum-tab-order

2026-06-03 07:55:48 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#720