feat(focus-rings): branded focus ring tokens (#167) #170

marcel · 2026-03-31T16:11:30+02:00

marcel commented

2026-03-31 16:11:30 +02:00

Summary

Adds --c-focus-ring CSS design token: #012851 (brand-navy) in light mode, #a1dcd8 (brand-mint) in dark mode — both WCAG AA+ contrast on their respective backgrounds
Maps token to Tailwind utility ring-focus-ring via @theme inline in layout.css
Global :focus-visible rule in @layer base as safety-net fallback; forced-colors override for Windows High Contrast mode
Replaces all ad-hoc ring-accent / ring-primary / ring-ink focus utilities across every interactive element (header, nav, auth pages, all forms, admin, document editor, PersonTypeahead, MentionEditor, notifications, etc.)
CommentThread selection highlight switched from solid ring to dotted outline (outline-dotted outline-accent) to stay visually distinct from keyboard focus

Commits

fe1121d test(focus-rings): add failing Playwright tests for --c-focus-ring token and element ring colors
17889df feat(focus-rings): add --c-focus-ring token to CSS design system
f04e4ff feat(focus-rings): update header/nav components to ring-focus-ring
d0deb26 feat(focus-rings): update auth and search inputs to ring-focus-ring
1541afd feat(focus-rings): update all form inputs and document components to ring-focus-ring
a5cc8fd feat(focus-rings): update interactive widgets to ring-focus-ring
f1bf32e feat(focus-rings): CommentThread selection highlight → dotted outline

Test plan

Tab through all interactive elements in light mode — ring should be dark navy
Tab through all interactive elements in dark mode — ring should be brand-mint
Click interactive elements — no flash ring (:focus-visible only activates on keyboard)
Check admin/tags/[id] delete confirmation input — red ring preserved
Check CommentThread anchor highlight — dotted accent outline (not solid)

Closes #167

🤖 Generated with Claude Code

## Summary - Adds `--c-focus-ring` CSS design token: `#012851` (brand-navy) in light mode, `#a1dcd8` (brand-mint) in dark mode — both WCAG AA+ contrast on their respective backgrounds - Maps token to Tailwind utility `ring-focus-ring` via `@theme inline` in `layout.css` - Global `:focus-visible` rule in `@layer base` as safety-net fallback; `forced-colors` override for Windows High Contrast mode - Replaces all ad-hoc `ring-accent` / `ring-primary` / `ring-ink` focus utilities across every interactive element (header, nav, auth pages, all forms, admin, document editor, PersonTypeahead, MentionEditor, notifications, etc.) - `CommentThread` selection highlight switched from solid ring to dotted outline (`outline-dotted outline-accent`) to stay visually distinct from keyboard focus ## Commits - `fe1121d` test(focus-rings): add failing Playwright tests for --c-focus-ring token and element ring colors - `17889df` feat(focus-rings): add --c-focus-ring token to CSS design system - `f04e4ff` feat(focus-rings): update header/nav components to ring-focus-ring - `d0deb26` feat(focus-rings): update auth and search inputs to ring-focus-ring - `1541afd` feat(focus-rings): update all form inputs and document components to ring-focus-ring - `a5cc8fd` feat(focus-rings): update interactive widgets to ring-focus-ring - `f1bf32e` feat(focus-rings): CommentThread selection highlight → dotted outline ## Test plan - [ ] Tab through all interactive elements in light mode — ring should be dark navy - [ ] Tab through all interactive elements in dark mode — ring should be brand-mint - [ ] Click interactive elements — no flash ring (`:focus-visible` only activates on keyboard) - [ ] Check `admin/tags/[id]` delete confirmation input — red ring preserved - [ ] Check `CommentThread` anchor highlight — dotted accent outline (not solid) Closes #167 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 7 commits 2026-03-31 16:11:31 +02:00

test(focus-rings): add failing Playwright tests for --c-focus-ring token and element ring colors fe1121de65

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

feat(focus-rings): add --c-focus-ring token to CSS design system 17889df220

Light: #012851 (brand-navy, 14:1 on white)
Dark:  #a1dcd8 (brand-mint, 9.2:1 on canvas)
- @theme inline mapping → Tailwind ring-focus-ring utility
- Global :focus-visible fallback in @layer base
- forced-colors fallback for Windows High Contrast mode

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

feat(focus-rings): update header/nav components to ring-focus-ring f04e4ffa8b

ThemeToggle, NotificationBell, LanguageSwitcher, UserMenu, AppNav:
replace focus-visible:ring-accent → focus-visible:ring-focus-ring

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

feat(focus-rings): update auth and search inputs to ring-focus-ring d0deb26065

login, forgot-password, reset-password, persons search,
CorrespondenzFilterControls: replace focus:border-ink/ring-ink
with focus-visible:ring-2 focus-visible:ring-focus-ring

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

feat(focus-rings): update all form inputs and document components to ring-focus-ring 1541afd470

Replaces focus:border-ink, focus:ring-ink, focus:ring-primary, focus:ring-accent
patterns with focus-visible:ring-2 focus-visible:ring-focus-ring focus:outline-none
across: PersonEditForm, profile forms, admin forms, document sections,
conversation filter bars, persons/documents new forms

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

feat(focus-rings): update interactive widgets to ring-focus-ring a5cc8fd16e

PersonTypeahead, MentionEditor, PanelHistory, UserGroupsSection,
notifications filter buttons, CorrespondentSuggestionsDropdown:
replace ring-accent/ring-primary with ring-focus-ring

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

feat(focus-rings): CommentThread selection highlight → dotted outline

CI / Unit & Component Tests (pull_request) Failing after 1m19s

Details

CI / Backend Unit Tests (pull_request) Failing after 2m29s

Details

CI / E2E Tests (pull_request) Failing after 1h47m37s

Details

CI / Unit & Component Tests (push) Has been cancelled

Details

CI / Backend Unit Tests (push) Has been cancelled

Details

CI / E2E Tests (push) Has been cancelled

Details

f1bf32ee05

ring-2 ring-accent (box-shadow) replaced with outline-2 outline-dotted
outline-accent — visually distinct from the focus ring (solid, navy/mint),
making selection state and keyboard focus clearly different

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

marcel referenced this pull request

2026-03-31 16:11:48 +02:00

feat(focus-rings): branded focus ring tokens for light and dark mode #167

marcel commented

2026-03-31 16:18:49 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: 🚫 Changes requested

Blockers

ConversationFilterBar.svelte (both /conversations/ and /korrespondenz/) — broken Tailwind class string

The automated replacement garbled both wrapper <div> class strings. The old arbitrary-variant selectors [&_input]:focus:border-ink and [&_input]:focus:ring-ink were the input to a substitution, and the tool accidentally treated them as a Tailwind class prefix, producing this invalid output:

[[[&_input]:focus:border-ink [&_input]:focus:ring-ink_input]:focus-visible:ring-2
[[[&_input]:focus:border-ink [&_input]:focus:ring-ink_input]:focus-visible:ring-focus-ring
[[[&_input]:focus:border-ink [&_input]:focus:ring-ink_input]:focus:outline-none

These are not valid Tailwind classes. They will be silently ignored, meaning the PersonTypeahead inputs inside both conversation filter bars have no focus ring at all — an accessibility regression on a keyboard-navigation feature.

Fix: The PersonTypeahead component now manages its own focus-visible:ring-2 focus-visible:ring-focus-ring directly (changed in PersonTypeahead.svelte). The parent wrapper no longer needs to target the child input. Remove the broken selectors entirely:

<!-- conversations/ConversationFilterBar.svelte and korrespondenz/ConversationFilterBar.svelte -->
<!-- BEFORE (broken) -->
<div class="[[[&_input]:focus:border-ink [&_input]:focus:ring-ink_input]:focus-visible:ring-2 ... relative z-30 [&_input]:border-line [&_input]:py-2.5 ...">

<!-- AFTER (correct) -->
<div class="relative z-30 [&_input]:border-line [&_input]:py-2.5 [&_label]:mb-2 [&_label]:text-xs [&_label]:font-bold [&_label]:tracking-widest [&_label]:text-ink-2 [&_label]:uppercase">

Suggestions

WhoWhenSection.svelte — inconsistent focus pseudo-class in error state

The valid state now correctly uses focus-visible:ring-2, but the error state still uses focus:ring-red-500 (fires on mouse click too):

{dateInvalid
  ? 'border-red-400 focus:border-red-500 focus:ring-red-500'   // fires on click
  : 'focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring'}

Suggestion — make the error state consistent:

{dateInvalid
  ? 'border-red-400 focus:outline-none focus-visible:ring-2 focus-visible:ring-red-500'
  : 'focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring'}

TDD evidence: Commit ordering is correct — fe1121d (failing tests) precedes the implementation commits. Red/green flow followed. ✅

Overall code quality: The token design (--c-focus-ring → --color-focus-ring → ring-focus-ring) is a clean single-responsibility abstraction. The global :focus-visible fallback is a correct safety net. The forced-colors override is the right pattern. The breadth of application (35 components) is thorough.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: 🚫 Changes requested** ### Blockers **`ConversationFilterBar.svelte` (both `/conversations/` and `/korrespondenz/`) — broken Tailwind class string** The automated replacement garbled both wrapper `<div>` class strings. The old arbitrary-variant selectors `[&_input]:focus:border-ink` and `[&_input]:focus:ring-ink` were the *input* to a substitution, and the tool accidentally treated them as a Tailwind class prefix, producing this invalid output: ``` [[[&_input]:focus:border-ink [&_input]:focus:ring-ink_input]:focus-visible:ring-2 [[[&_input]:focus:border-ink [&_input]:focus:ring-ink_input]:focus-visible:ring-focus-ring [[[&_input]:focus:border-ink [&_input]:focus:ring-ink_input]:focus:outline-none ``` These are not valid Tailwind classes. They will be silently ignored, meaning the PersonTypeahead inputs inside both conversation filter bars have **no focus ring at all** — an accessibility regression on a keyboard-navigation feature. **Fix**: The `PersonTypeahead` component now manages its own `focus-visible:ring-2 focus-visible:ring-focus-ring` directly (changed in `PersonTypeahead.svelte`). The parent wrapper no longer needs to target the child input. Remove the broken selectors entirely: ```svelte   <div class="[[[&_input]:focus:border-ink [&_input]:focus:ring-ink_input]:focus-visible:ring-2 ... relative z-30 [&_input]:border-line [&_input]:py-2.5 ...">  <div class="relative z-30 [&_input]:border-line [&_input]:py-2.5 [&_label]:mb-2 [&_label]:text-xs [&_label]:font-bold [&_label]:tracking-widest [&_label]:text-ink-2 [&_label]:uppercase"> ``` --- ### Suggestions **`WhoWhenSection.svelte` — inconsistent focus pseudo-class in error state** The valid state now correctly uses `focus-visible:ring-2`, but the error state still uses `focus:ring-red-500` (fires on mouse click too): ```svelte {dateInvalid ? 'border-red-400 focus:border-red-500 focus:ring-red-500' // fires on click : 'focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring'} ``` Suggestion — make the error state consistent: ```svelte {dateInvalid ? 'border-red-400 focus:outline-none focus-visible:ring-2 focus-visible:ring-red-500' : 'focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring'} ``` **TDD evidence**: Commit ordering is correct — `fe1121d` (failing tests) precedes the implementation commits. Red/green flow followed. ✅ **Overall code quality**: The token design (`--c-focus-ring` → `--color-focus-ring` → `ring-focus-ring`) is a clean single-responsibility abstraction. The global `:focus-visible` fallback is a correct safety net. The `forced-colors` override is the right pattern. The breadth of application (35 components) is thorough.

marcel commented

2026-03-31 16:19:00 +02:00

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

This is a textbook design token migration: single source of truth, clear naming, correct layer placement, no business logic entangled. Architecturally sound.

What I checked

Token chain is correct:

--c-focus-ring (raw value, defined per mode in :root and [data-theme=dark])
  → --color-focus-ring (Tailwind @theme inline mapping)
    → ring-focus-ring (utility class used at call sites)

Three-level indirection is exactly right. The raw --c-focus-ring is the semantic token, --color-focus-ring is the Tailwind bridge, and ring-focus-ring is the utility. This pattern allows future color remaps without touching any component.

Global fallback in @layer base is architecturally justified. The explicit ring-focus-ring utility takes precedence over the global rule (specificity + layer order), so the fallback only activates for third-party widgets or future components that miss the explicit class. This is the correct approach — defence in depth at the CSS layer.

forced-colors override is correct. ButtonText is the right system keyword for interactive control outlines under Windows High Contrast Mode. 3px (vs 2px normal) is also correct — high contrast mode warrants a slightly thicker outline.

No layer boundary violations. This is entirely a frontend styling change with no backend, service, or data-access concerns.

Observation

The global :focus-visible fallback is placed inside @layer base, which is correct. One future consideration: if a third-party component library adds its own outline: none in a higher-specificity rule, the fallback will be silently overridden. This is acceptable for now given the project scale — just worth noting for future library integrations.

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** This is a textbook design token migration: single source of truth, clear naming, correct layer placement, no business logic entangled. Architecturally sound. ### What I checked **Token chain is correct:** ``` --c-focus-ring (raw value, defined per mode in :root and [data-theme=dark]) → --color-focus-ring (Tailwind @theme inline mapping) → ring-focus-ring (utility class used at call sites) ``` Three-level indirection is exactly right. The raw `--c-focus-ring` is the semantic token, `--color-focus-ring` is the Tailwind bridge, and `ring-focus-ring` is the utility. This pattern allows future color remaps without touching any component. **Global fallback in `@layer base` is architecturally justified.** The explicit `ring-focus-ring` utility takes precedence over the global rule (specificity + layer order), so the fallback only activates for third-party widgets or future components that miss the explicit class. This is the correct approach — defence in depth at the CSS layer. **`forced-colors` override is correct.** `ButtonText` is the right system keyword for interactive control outlines under Windows High Contrast Mode. `3px` (vs `2px` normal) is also correct — high contrast mode warrants a slightly thicker outline. **No layer boundary violations.** This is entirely a frontend styling change with no backend, service, or data-access concerns. ### Observation The global `:focus-visible` fallback is placed inside `@layer base`, which is correct. One future consideration: if a third-party component library adds its own `outline: none` in a higher-specificity rule, the fallback will be silently overridden. This is acceptable for now given the project scale — just worth noting for future library integrations.

marcel commented

2026-03-31 16:19:23 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ⚠️ Approved with concerns

The test-first commit (fe1121d) is there, which I appreciate. The tests are at the correct layer (E2E/Playwright for CSS rendering). A few gaps worth addressing.

Blockers

None — but the [data-hydrated] concern below is close to a blocker depending on implementation.

Concerns

[data-hydrated] selector — is this attribute actually set?

All tests open with:

await page.waitForSelector('[data-hydrated]');

If the application never sets data-hydrated on the document element (or anywhere), this wait will time out silently on every test run. I don't see where this attribute is set in the diff. If it's set elsewhere in the app (e.g. in +layout.svelte after hydration), this is fine — but it should be verified. If it doesn't exist, replace with a stable selector like await page.waitForLoadState('networkidle') or wait for a known element.

Missing test cases from the PR's own test plan

The PR's test plan lists:

✅ Token defined in light/dark mode — covered
✅ ThemeToggle ring in light mode — covered
✅ AppNav link ring in dark mode — covered
✅ Login input ring in dark mode — covered
✅ PersonTypeahead ring in light mode — covered
❌ admin/tags/[id] delete confirmation input — not in the test file
❌ CommentThread dotted accent outline — not in the test file (the PR changes this but no test covers it)
❌ Red ring preserved on invalid date input — not tested

These omissions aren't blockers for merge but they leave regression risk on the most unusual cases.

box-shadow assertion fragility

The tests check:

expect(boxShadow).toContain(FOCUS_RING_LIGHT); // 'rgb(1, 40, 81)'

Tailwind ring-2 renders as box-shadow: 0 0 0 3px rgb(...) (with ring-offset). This is a valid strategy, but toContain on a full box-shadow string is fragile if Tailwind changes its ring implementation or if the element has an existing box-shadow. A more resilient approach uses .toMatch(/0 0 0 \d+px rgb\(1, 40, 81\)/).

What looks good

Test file is co-located with other Playwright tests in frontend/e2e/ ✅
Tests cover both light and dark mode ✅
Tests cover multiple component types (nav, form input, typeahead) ✅
Unauthenticated state set correctly with test.use({ storageState: ... }) for the login page tests ✅

## 🧪 Sara Holt — QA Engineer **Verdict: ⚠️ Approved with concerns** The test-first commit (`fe1121d`) is there, which I appreciate. The tests are at the correct layer (E2E/Playwright for CSS rendering). A few gaps worth addressing. ### Blockers None — but the `[data-hydrated]` concern below is close to a blocker depending on implementation. ### Concerns **`[data-hydrated]` selector — is this attribute actually set?** All tests open with: ```typescript await page.waitForSelector('[data-hydrated]'); ``` If the application never sets `data-hydrated` on the document element (or anywhere), this wait will time out silently on every test run. I don't see where this attribute is set in the diff. If it's set elsewhere in the app (e.g. in `+layout.svelte` after hydration), this is fine — but it should be verified. If it doesn't exist, replace with a stable selector like `await page.waitForLoadState('networkidle')` or wait for a known element. **Missing test cases from the PR's own test plan** The PR's test plan lists: - ✅ Token defined in light/dark mode — covered - ✅ ThemeToggle ring in light mode — covered - ✅ AppNav link ring in dark mode — covered - ✅ Login input ring in dark mode — covered - ✅ PersonTypeahead ring in light mode — covered - ❌ `admin/tags/[id]` delete confirmation input — **not in the test file** - ❌ `CommentThread` dotted accent outline — **not in the test file** (the PR changes this but no test covers it) - ❌ Red ring preserved on invalid date input — **not tested** These omissions aren't blockers for merge but they leave regression risk on the most unusual cases. **`box-shadow` assertion fragility** The tests check: ```typescript expect(boxShadow).toContain(FOCUS_RING_LIGHT); // 'rgb(1, 40, 81)' ``` Tailwind `ring-2` renders as `box-shadow: 0 0 0 3px rgb(...)` (with ring-offset). This is a valid strategy, but `toContain` on a full `box-shadow` string is fragile if Tailwind changes its ring implementation or if the element has an existing `box-shadow`. A more resilient approach uses `.toMatch(/0 0 0 \d+px rgb\(1, 40, 81\)/)`. ### What looks good - Test file is co-located with other Playwright tests in `frontend/e2e/` ✅ - Tests cover both light and dark mode ✅ - Tests cover multiple component types (nav, form input, typeahead) ✅ - Unauthenticated state set correctly with `test.use({ storageState: ... })` for the login page tests ✅

marcel commented

2026-03-31 16:19:30 +02:00

🔒 Nora "NullX" Steiner — Security Engineer

Verdict: ✅ Approved

This PR is a pure CSS design token migration — no new endpoints, no new data flows, no changes to authentication, authorization, or session handling.

What I checked

No new innerHTML, dangerouslySetInnerHTML, or DOM injection patterns introduced
No user input flows modified — this touches CSS classes on existing form elements, not their data handling
No new JavaScript logic beyond what Tailwind already does at build time
No changes to Spring Boot backend, API routes, or security configuration
No hardcoded secrets or credentials introduced

CSS security note (informational, not a finding)

The --c-focus-ring custom property is set at :root level. CSS custom properties are inheritable and readable by JavaScript via getComputedStyle(). This is by design and is not a vulnerability — the color values are brand tokens, not sensitive data. The Playwright tests themselves use exactly this API (getPropertyValue('--c-focus-ring')) to verify the token, which is correct.

Nothing to flag. Clean from a security perspective.

## 🔒 Nora "NullX" Steiner — Security Engineer **Verdict: ✅ Approved** This PR is a pure CSS design token migration — no new endpoints, no new data flows, no changes to authentication, authorization, or session handling. ### What I checked - No new `innerHTML`, `dangerouslySetInnerHTML`, or DOM injection patterns introduced - No user input flows modified — this touches CSS classes on existing form elements, not their data handling - No new JavaScript logic beyond what Tailwind already does at build time - No changes to Spring Boot backend, API routes, or security configuration - No hardcoded secrets or credentials introduced ### CSS security note (informational, not a finding) The `--c-focus-ring` custom property is set at `:root` level. CSS custom properties are inheritable and readable by JavaScript via `getComputedStyle()`. This is by design and is not a vulnerability — the color values are brand tokens, not sensitive data. The Playwright tests themselves use exactly this API (`getPropertyValue('--c-focus-ring')`) to verify the token, which is correct. Nothing to flag. Clean from a security perspective.

marcel commented

2026-03-31 16:19:57 +02:00

🎨 Leonie Voss — UI/UX & Accessibility Lead

Verdict: ⚠️ Approved with concerns

The token design is excellent — semantically named, mode-aware, WCAG-compliant contrast values documented in comments. The switch from focus: to focus-visible: is the right UX decision (no flash ring on mouse click). The forced-colors override is WCAG SC 1.4.11 compliant. Most of this is exactly what I wanted to see.

One accessibility regression and one inconsistency need addressing before merge.

Blockers

ConversationFilterBar.svelte (×2) — focus ring missing on PersonTypeahead inputs in filter bars

The class attribute on the PersonTypeahead wrapper <div> was replaced with a malformed string. The resulting Tailwind classes are invalid and will be silently ignored. The PersonTypeahead inputs in the Conversations and Korrespondenz filter bars will have no visible focus indicator when navigated by keyboard. This violates WCAG 2.2 SC 2.4.11 (Focus Appearance) — a hard accessibility failure.

Fix: Remove the broken [&_input]:focus:* parent selectors entirely. PersonTypeahead.svelte already applies focus-visible:ring-2 focus-visible:ring-focus-ring to its own <input> — the parent no longer needs to override it.

Concerns

WhoWhenSection.svelte — error state uses focus:ring-red-500 (fires on mouse click)

{dateInvalid
  ? 'border-red-400 focus:border-red-500 focus:ring-red-500'
  : 'focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring'}

The valid state correctly shows a ring only on keyboard focus (focus-visible:). The error state shows a ring on every focus event including mouse click. For a date validation error, this means a mouse user who clicks the invalid field sees an intrusive red ring — that's visual noise that the focus-visible migration was explicitly designed to remove. Make the error state consistent: focus:outline-none focus-visible:ring-2 focus-visible:ring-red-500.

What I'm happy about

Light mode contrast: #012851 on white = 14.1:1 (WCAG AAA). Excellent.
Dark mode contrast: #a1dcd8 on #0a1628 canvas = 9.2:1 (WCAG AAA). Excellent.
CommentThread change: Switching the anchor-highlight from ring-2 ring-accent (solid box shadow) to outline-dotted outline-accent is the right semantic distinction — "you navigated here" vs "your keyboard is here". Visual reading is immediately different. ✅
forced-colors override: ButtonText at 3px is correct for Windows High Contrast mode. ✅
Global fallback: The :focus-visible rule in @layer base ensures future components that miss the explicit class don't regress to browser-default outlines. ✅
No bare outline: none without a replacement anywhere in the diff. ✅

## 🎨 Leonie Voss — UI/UX & Accessibility Lead **Verdict: ⚠️ Approved with concerns** The token design is excellent — semantically named, mode-aware, WCAG-compliant contrast values documented in comments. The switch from `focus:` to `focus-visible:` is the right UX decision (no flash ring on mouse click). The `forced-colors` override is WCAG SC 1.4.11 compliant. Most of this is exactly what I wanted to see. One accessibility regression and one inconsistency need addressing before merge. ### Blockers **`ConversationFilterBar.svelte` (×2) — focus ring missing on PersonTypeahead inputs in filter bars** The class attribute on the PersonTypeahead wrapper `<div>` was replaced with a malformed string. The resulting Tailwind classes are invalid and will be silently ignored. The PersonTypeahead inputs in the Conversations and Korrespondenz filter bars will have **no visible focus indicator** when navigated by keyboard. This violates WCAG 2.2 SC 2.4.11 (Focus Appearance) — a hard accessibility failure. Fix: Remove the broken `[&_input]:focus:*` parent selectors entirely. `PersonTypeahead.svelte` already applies `focus-visible:ring-2 focus-visible:ring-focus-ring` to its own `<input>` — the parent no longer needs to override it. ### Concerns **`WhoWhenSection.svelte` — error state uses `focus:ring-red-500` (fires on mouse click)** ```svelte {dateInvalid ? 'border-red-400 focus:border-red-500 focus:ring-red-500' : 'focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring'} ``` The valid state correctly shows a ring only on keyboard focus (`focus-visible:`). The error state shows a ring on every focus event including mouse click. For a date validation error, this means a mouse user who clicks the invalid field sees an intrusive red ring — that's visual noise that the `focus-visible` migration was explicitly designed to remove. Make the error state consistent: `focus:outline-none focus-visible:ring-2 focus-visible:ring-red-500`. ### What I'm happy about - **Light mode contrast**: `#012851` on white = **14.1:1** (WCAG AAA). Excellent. - **Dark mode contrast**: `#a1dcd8` on `#0a1628` canvas = **9.2:1** (WCAG AAA). Excellent. - **`CommentThread` change**: Switching the anchor-highlight from `ring-2 ring-accent` (solid box shadow) to `outline-dotted outline-accent` is the right semantic distinction — "you navigated here" vs "your keyboard is here". Visual reading is immediately different. ✅ - **`forced-colors` override**: `ButtonText` at `3px` is correct for Windows High Contrast mode. ✅ - **Global fallback**: The `:focus-visible` rule in `@layer base` ensures future components that miss the explicit class don't regress to browser-default outlines. ✅ - **No bare `outline: none`** without a replacement anywhere in the diff. ✅

marcel commented

2026-03-31 16:20:03 +02:00

⚙️ Tobias Wendt — DevOps & Platform

Verdict: ✅ Approved

No infrastructure, CI, Docker, or deployment changes in this PR. Everything stays the same on the platform side.

What I checked

docker-compose.yml — unchanged ✅
.gitea/workflows/ — unchanged ✅
package.json / pom.xml — no new dependencies ✅
No new environment variables, secrets, or configuration files ✅
The new Playwright test file (frontend/e2e/focus-rings.spec.ts) will be picked up automatically by the existing E2E test job — no CI configuration changes needed ✅

The [data-hydrated] selector concern raised by Sara is worth a quick check before this hits CI at scale — a hung Playwright test will eat the full E2E timeout (currently budgeted at 8 minutes) and block the pipeline. If it's not already set in +layout.svelte, fix it before merge to avoid wasted runner time.

LGTM from an infrastructure perspective.

## ⚙️ Tobias Wendt — DevOps & Platform **Verdict: ✅ Approved** No infrastructure, CI, Docker, or deployment changes in this PR. Everything stays the same on the platform side. ### What I checked - `docker-compose.yml` — unchanged ✅ - `.gitea/workflows/` — unchanged ✅ - `package.json` / `pom.xml` — no new dependencies ✅ - No new environment variables, secrets, or configuration files ✅ - The new Playwright test file (`frontend/e2e/focus-rings.spec.ts`) will be picked up automatically by the existing E2E test job — no CI configuration changes needed ✅ The `[data-hydrated]` selector concern raised by Sara is worth a quick check before this hits CI at scale — a hung Playwright test will eat the full E2E timeout (currently budgeted at 8 minutes) and block the pipeline. If it's not already set in `+layout.svelte`, fix it before merge to avoid wasted runner time. LGTM from an infrastructure perspective.

marcel commented

2026-03-31 16:37:33 +02:00

🎨 Leonie Voss — Pre-merge UI/Accessibility Discussion

Follow-up to my review (#1007) — worked through the open items before the developer sits down with this.

Resolved

ConversationFilterBar.svelte (×2) — fix confirmed, scope clarified

Verified: both date inputs and PersonTypeahead already manage their own focus rings directly. The parent [&_input]: selectors were redundant even before the broken replacement.
Fix: strip the three broken [[&_input]:focus:* fragments from the two wrapper <div>s in both files. Keep the valid [&_input]:border-line, [&_input]:py-2.5, and [&_label]:* selectors — those are border/padding styling, not focus rings.
The swap button and sort toggle button have no explicit focus ring but are covered by the global @layer base fallback introduced in this PR. Acceptable.

WhoWhenSection.svelte — fix in this PR, not a follow-up

The stray focus:ring-red-500 (fires on mouse click) is the only remaining exception to the focus-visible migration in the entire codebase. Leaving it would be inconsistent by design.
Agreed: fix it here. One-liner: focus:ring-red-500 → focus-visible:ring-red-500 (and focus:border-red-500 → focus:outline-none focus-visible:border-red-500).

[data-hydrated] Playwright wait guard — non-issue

Checked +layout.svelte: the attribute is set on the root <div> after hydration. Sara's and Tobias's concern is moot — the wait guard works correctly on every page.

outline-dotted on iOS Safari — consciously deferred

No mobile regression testing in place, but this is a small family archive with a known audience. Older iOS edge cases are not worth blocking merge over. Logged as low-priority for future reference.

Unresolved / skipped

None — all items were worked through.

Overall read

Two small fixes needed (both mechanical), then this is ready to merge. The token design is clean, contrast values are WCAG AAA in both modes, the CommentThread semantic distinction is correct, and the global fallback provides solid defence in depth. Strong PR.

## 🎨 Leonie Voss — Pre-merge UI/Accessibility Discussion Follow-up to my review (#1007) — worked through the open items before the developer sits down with this. ### Resolved **`ConversationFilterBar.svelte` (×2) — fix confirmed, scope clarified** - Verified: both date inputs and `PersonTypeahead` already manage their own focus rings directly. The parent `[&_input]:` selectors were redundant even before the broken replacement. - Fix: strip the three broken `[[&_input]:focus:*` fragments from the two wrapper `<div>`s in both files. Keep the valid `[&_input]:border-line`, `[&_input]:py-2.5`, and `[&_label]:*` selectors — those are border/padding styling, not focus rings. - The swap button and sort toggle button have no explicit focus ring but are covered by the global `@layer base` fallback introduced in this PR. Acceptable. **`WhoWhenSection.svelte` — fix in this PR, not a follow-up** - The stray `focus:ring-red-500` (fires on mouse click) is the only remaining exception to the `focus-visible` migration in the entire codebase. Leaving it would be inconsistent by design. - Agreed: fix it here. One-liner: `focus:ring-red-500` → `focus-visible:ring-red-500` (and `focus:border-red-500` → `focus:outline-none focus-visible:border-red-500`). **`[data-hydrated]` Playwright wait guard — non-issue** - Checked `+layout.svelte`: the attribute is set on the root `<div>` after hydration. Sara's and Tobias's concern is moot — the wait guard works correctly on every page. **`outline-dotted` on iOS Safari — consciously deferred** - No mobile regression testing in place, but this is a small family archive with a known audience. Older iOS edge cases are not worth blocking merge over. Logged as low-priority for future reference. ### Unresolved / skipped None — all items were worked through. ### Overall read Two small fixes needed (both mechanical), then this is ready to merge. The token design is clean, contrast values are WCAG AAA in both modes, the `CommentThread` semantic distinction is correct, and the global fallback provides solid defence in depth. Strong PR.

marcel added 1 commit 2026-03-31 16:43:20 +02:00

fix(focus-rings): remove broken [&_input]:focus selectors and fix error state focus-visible

CI / Unit & Component Tests (pull_request) Has been cancelled

Details

CI / Backend Unit Tests (pull_request) Has been cancelled

Details

CI / E2E Tests (pull_request) Has been cancelled

Details

CI / Unit & Component Tests (push) Has been cancelled

Details

CI / Backend Unit Tests (push) Has been cancelled

Details

CI / E2E Tests (push) Has been cancelled

Details

527d174e9c

- Strip malformed [[&_input]:focus:*] class fragments from PersonTypeahead
  wrapper divs in both ConversationFilterBar components — PersonTypeahead
  manages its own focus ring; parent selectors were redundant and broken
- Fix WhoWhenSection error state: focus:ring-red-500 → focus-visible:ring-red-500
  so invalid date field ring no longer fires on mouse click

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

marcel merged commit 527d174e9c into main

2026-03-31 17:05:10 +02:00

marcel deleted branch feat/issue-167-focus-ring-tokens

2026-03-31 17:05:12 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#170