test(a11y): add axe-playwright E2E gate for PDF viewer WCAG 2.1 AA compliance #353

New Issue

marcel · 2026-04-26T21:26:43+02:00

marcel commented

2026-04-26 21:26:43 +02:00

Problem

PR #349 fixed the text-accent contrast failure on the annotation toggle by switching to text-primary. The unit tests assert the correct CSS class is applied, but they do not verify the computed contrast ratio in a real browser.

A future regression that changes the value of --c-primary without touching the class name would pass all current tests undetected.

Acceptance Criteria

An axe-playwright test exists in frontend/e2e/ that navigates to a document page with annotations and asserts zero WCAG 2.1 AA violations on the PDF controls bar
The test uses @axe-core/playwright and runs via the existing npm run test:e2e (or equivalent) script
The test fixture includes a document with at least one annotation so the toggle button is rendered

Proposed implementation

// frontend/e2e/pdf-viewer-a11y.spec.ts
import AxeBuilder from '@axe-core/playwright';

test('pdf controls pass WCAG 2.1 AA in light mode', async ({ page }) => {
    await page.goto('/documents/<id-with-annotations>');
    const results = await new AxeBuilder({ page })
        .withTags(['wcag2a', 'wcag2aa'])
        .analyze();
    expect(results.violations).toEqual([]);
});

Fixture concern: the E2E environment needs a seeded document with annotations. Consider using the existing E2E seed data or adding a fixture document to the CI seed script.

Background

Raised by Sara Holt (QA) and Elicit (Requirements) in PR #349 review
Original fix: PR #349 (issue #341)
NFR: zero axe WCAG2AA violations on the PDF viewer page

## Problem PR #349 fixed the `text-accent` contrast failure on the annotation toggle by switching to `text-primary`. The unit tests assert the correct CSS class is applied, but they do **not** verify the computed contrast ratio in a real browser. A future regression that changes the value of `--c-primary` without touching the class name would pass all current tests undetected. ## Acceptance Criteria - [ ] An axe-playwright test exists in `frontend/e2e/` that navigates to a document page with annotations and asserts zero WCAG 2.1 AA violations on the PDF controls bar - [ ] The test uses `@axe-core/playwright` and runs via the existing `npm run test:e2e` (or equivalent) script - [ ] The test fixture includes a document with at least one annotation so the toggle button is rendered ## Proposed implementation ```typescript // frontend/e2e/pdf-viewer-a11y.spec.ts import AxeBuilder from '@axe-core/playwright'; test('pdf controls pass WCAG 2.1 AA in light mode', async ({ page }) => { await page.goto('/documents/<id-with-annotations>'); const results = await new AxeBuilder({ page }) .withTags(['wcag2a', 'wcag2aa']) .analyze(); expect(results.violations).toEqual([]); }); ``` **Fixture concern:** the E2E environment needs a seeded document with annotations. Consider using the existing E2E seed data or adding a fixture document to the CI seed script. ## Background - Raised by Sara Holt (QA) and Elicit (Requirements) in PR #349 review - Original fix: PR #349 (issue #341) - NFR: zero axe WCAG2AA violations on the PDF viewer page

marcel added the P2-medium test ui labels 2026-04-26 21:26:56 +02:00

marcel referenced this issue

2026-04-26 21:27:04 +02:00

fix(viewer): fix "Annotieren anzeigen" contrast and add lint rule (#341) #349

marcel commented

2026-05-07 17:48:52 +02:00

👨‍💻 Markus Keller — Application Architect

Observations

The issue correctly identifies a structural gap in the test pyramid: the unit test in PdfControls.svelte.spec.ts asserts the CSS class name (text-primary) but cannot verify the computed contrast ratio produced by the underlying CSS custom property at runtime. This is a valid architectural concern — the test is testing the wrong layer.
The existing accessibility.spec.ts pattern is sound and already in use: it uses AxeBuilder with withTags(['wcag2a', 'wcag2aa']), waits for [data-hydrated], and follows the same auth setup. The proposed new test should mirror this pattern exactly rather than invent a new one.
The document detail page (/documents/[id]) is already a SvelteKit SSR route. Navigating to it in Playwright will trigger server-side rendering, which means the annotations API and transcription API are both involved. The test fixture must seed a document with at least one transcription block (which carries the annotation) to exercise the toggle button path.
The annotations.spec.ts file already contains a working pattern for seeding a document with annotations via the REST API in beforeAll. That same fixture setup — POST /api/documents, PUT /api/documents/:id with a PDF, POST /api/documents/:id/transcription-blocks — is the correct reuse point. Do not duplicate this logic: extract it into a helper alongside upload-empty-document.ts.
The CI pipeline (ci.yml) currently has no E2E job. The e2e/CLAUDE.md confirms this: "E2E tests are not currently run in CI." This issue adds a test that cannot be gated unless a CI E2E job is added. A test that only runs locally provides a much weaker regression guarantee.

Recommendations

Place the new spec at frontend/e2e/pdf-controls-a11y.spec.ts (not pdf-viewer-a11y.spec.ts as proposed) — the scope is specifically the PDF controls bar, and naming it precisely makes future navigation easier.
Extract the annotation seeding logic from annotations.spec.ts into frontend/e2e/helpers/create-annotated-document.ts so both annotations.spec.ts and the new a11y spec share it without duplication. This reduces maintenance cost when the API changes.
Run axe in both light and dark mode, following the precedent in accessibility.spec.ts. The annotation toggle button uses text-primary whose contrast is the whole point of this issue — dark mode may produce different computed values.
The PDF controls bar uses a custom border-pdf-ctrl token and bg-surface/10 with opacity. These computed values need real browser rendering to evaluate — this is exactly why the axe E2E test is necessary and why the unit test is insufficient.
Accepting this issue without a CI E2E job is accepting a test that is invisible to the merge gate. Recommend filing a follow-up issue to add the Playwright job to ci.yml before this issue is closed.

Open Decisions

Should this issue be blocked until a CI E2E job exists, or should the test be merged now (providing local assurance) with a separate CI issue filed? The current project has no E2E CI gate at all, so this is a systemic question, not one specific to this issue.

## 👨‍💻 Markus Keller — Application Architect ### Observations - The issue correctly identifies a structural gap in the test pyramid: the unit test in `PdfControls.svelte.spec.ts` asserts the CSS class name (`text-primary`) but cannot verify the computed contrast ratio produced by the underlying CSS custom property at runtime. This is a valid architectural concern — the test is testing the wrong layer. - The existing `accessibility.spec.ts` pattern is sound and already in use: it uses `AxeBuilder` with `withTags(['wcag2a', 'wcag2aa'])`, waits for `[data-hydrated]`, and follows the same auth setup. The proposed new test should mirror this pattern exactly rather than invent a new one. - The document detail page (`/documents/[id]`) is already a SvelteKit SSR route. Navigating to it in Playwright will trigger server-side rendering, which means the annotations API and transcription API are both involved. The test fixture must seed a document with at least one transcription block (which carries the annotation) to exercise the toggle button path. - The `annotations.spec.ts` file already contains a working pattern for seeding a document with annotations via the REST API in `beforeAll`. That same fixture setup — `POST /api/documents`, `PUT /api/documents/:id` with a PDF, `POST /api/documents/:id/transcription-blocks` — is the correct reuse point. Do not duplicate this logic: extract it into a helper alongside `upload-empty-document.ts`. - The CI pipeline (`ci.yml`) currently has no E2E job. The `e2e/CLAUDE.md` confirms this: "E2E tests are **not** currently run in CI." This issue adds a test that cannot be gated unless a CI E2E job is added. A test that only runs locally provides a much weaker regression guarantee. ### Recommendations - Place the new spec at `frontend/e2e/pdf-controls-a11y.spec.ts` (not `pdf-viewer-a11y.spec.ts` as proposed) — the scope is specifically the PDF controls bar, and naming it precisely makes future navigation easier. - Extract the annotation seeding logic from `annotations.spec.ts` into `frontend/e2e/helpers/create-annotated-document.ts` so both `annotations.spec.ts` and the new a11y spec share it without duplication. This reduces maintenance cost when the API changes. - Run axe in both light and dark mode, following the precedent in `accessibility.spec.ts`. The annotation toggle button uses `text-primary` whose contrast is the whole point of this issue — dark mode may produce different computed values. - The PDF controls bar uses a custom `border-pdf-ctrl` token and `bg-surface/10` with opacity. These computed values need real browser rendering to evaluate — this is exactly why the axe E2E test is necessary and why the unit test is insufficient. - Accepting this issue without a CI E2E job is accepting a test that is invisible to the merge gate. Recommend filing a follow-up issue to add the Playwright job to `ci.yml` before this issue is closed. ### Open Decisions - Should this issue be blocked until a CI E2E job exists, or should the test be merged now (providing local assurance) with a separate CI issue filed? The current project has no E2E CI gate at all, so this is a systemic question, not one specific to this issue.

marcel commented

2026-05-07 17:49:11 +02:00

👨‍💻 Felix Brandt — Fullstack Developer

Observations

The unit test in PdfControls.svelte.spec.ts uses container.querySelectorAll('button') and manual array scanning to find the annotation toggle. This is testing DOM structure rather than accessible semantics — page.getByRole('button', { name: /annotierungen/i }) would be more robust and idiomatic (exactly how other tests in the file find buttons).
The existing axe check inside annotations.spec.ts (line 265–271) runs new AxeBuilder({ page }).analyze() without withTags(['wcag2a', 'wcag2aa']). This means it runs the full default ruleset rather than the project's declared WCAG 2.1 AA scope. The new dedicated spec should use .withTags(['wcag2a', 'wcag2aa']) consistently with accessibility.spec.ts.
The proposed implementation in the issue navigates to /documents/<id-with-annotations> with a hardcoded placeholder ID. The working pattern from annotations.spec.ts seeds the document via the API in beforeAll using the request fixture, which is the correct approach — no hardcoded IDs, no dependency on pre-existing database state.
The helper function createEmptyDocument in e2e/helpers/upload-empty-document.ts handles document creation and PDF upload. The new test needs an extension of that helper that also creates a transcription block. This is one new helper function, not a large change.
PdfControls.svelte uses annotationCount > 0 to conditionally render the toggle button. The fixture must produce a document where annotationCount > 0. The transcription block endpoint (POST /api/documents/:id/transcription-blocks) returns an annotationId, confirming that creating a block creates the annotation automatically — the same mechanism used in annotations.spec.ts.

Recommendations

Write the helper as createAnnotatedDocument(request: APIRequestContext): Promise<{ docId: string; annotationId: string }> — returns both IDs so the test can verify annotation rendering if needed, and can reuse it in annotations.spec.ts to eliminate the duplicated setup there.
Name the axe test following the existing sentence pattern: 'pdf controls bar passes wcag2a/wcag2aa with annotations visible' and 'pdf controls bar passes wcag2a/wcag2aa with annotations hidden' — two tests, one for each toggle state, since toggling changes the button's CSS class.
Wait for [data-hydrated] and then for .tabular-nums (the page counter) before running axe — this is the established pattern from annotations.spec.ts and ensures the PDF has finished rendering before accessibility rules are evaluated.
The existing axe call inside annotations.spec.ts should be updated to add .withTags(['wcag2a', 'wcag2aa']) as part of this issue's work, since it is a direct predecessor of the gap being fixed.

## 👨‍💻 Felix Brandt — Fullstack Developer ### Observations - The unit test in `PdfControls.svelte.spec.ts` uses `container.querySelectorAll('button')` and manual array scanning to find the annotation toggle. This is testing DOM structure rather than accessible semantics — `page.getByRole('button', { name: /annotierungen/i })` would be more robust and idiomatic (exactly how other tests in the file find buttons). - The existing axe check inside `annotations.spec.ts` (line 265–271) runs `new AxeBuilder({ page }).analyze()` without `withTags(['wcag2a', 'wcag2aa'])`. This means it runs the full default ruleset rather than the project's declared WCAG 2.1 AA scope. The new dedicated spec should use `.withTags(['wcag2a', 'wcag2aa'])` consistently with `accessibility.spec.ts`. - The proposed implementation in the issue navigates to `/documents/<id-with-annotations>` with a hardcoded placeholder ID. The working pattern from `annotations.spec.ts` seeds the document via the API in `beforeAll` using the `request` fixture, which is the correct approach — no hardcoded IDs, no dependency on pre-existing database state. - The helper function `createEmptyDocument` in `e2e/helpers/upload-empty-document.ts` handles document creation and PDF upload. The new test needs an extension of that helper that also creates a transcription block. This is one new helper function, not a large change. - `PdfControls.svelte` uses `annotationCount > 0` to conditionally render the toggle button. The fixture must produce a document where `annotationCount > 0`. The transcription block endpoint (`POST /api/documents/:id/transcription-blocks`) returns an `annotationId`, confirming that creating a block creates the annotation automatically — the same mechanism used in `annotations.spec.ts`. ### Recommendations - Write the helper as `createAnnotatedDocument(request: APIRequestContext): Promise<{ docId: string; annotationId: string }>` — returns both IDs so the test can verify annotation rendering if needed, and can reuse it in `annotations.spec.ts` to eliminate the duplicated setup there. - Name the axe test following the existing sentence pattern: `'pdf controls bar passes wcag2a/wcag2aa with annotations visible'` and `'pdf controls bar passes wcag2a/wcag2aa with annotations hidden'` — two tests, one for each toggle state, since toggling changes the button's CSS class. - Wait for `[data-hydrated]` and then for `.tabular-nums` (the page counter) before running axe — this is the established pattern from `annotations.spec.ts` and ensures the PDF has finished rendering before accessibility rules are evaluated. - The existing axe call inside `annotations.spec.ts` should be updated to add `.withTags(['wcag2a', 'wcag2aa'])` as part of this issue's work, since it is a direct predecessor of the gap being fixed.

marcel commented

2026-05-07 17:49:38 +02:00

👨‍💻 Tobias Wendt — DevOps & Platform Engineer

Observations

The ci.yml pipeline has three jobs: unit-tests, ocr-tests, and backend-unit-tests. There is no E2E job. The e2e/CLAUDE.md explicitly states "E2E tests are not currently run in CI." This means the regression gate this issue intends to create does not exist at the merge level — the test can only be run locally.
The unit-tests CI job runs inside mcr.microsoft.com/playwright:v1.58.2-noble, which has Chromium pre-installed. The axe assertions in this issue are an E2E Playwright test, not a component test. Running E2E in CI requires a full stack: frontend dev server, Spring Boot backend, PostgreSQL, and MinIO. The existing CI jobs run with none of those services.
A minimal E2E CI job would need: docker compose up -d db minio, wait for health, start the backend JAR or Spring Boot via Maven, start the SvelteKit dev server, then run npx playwright test. The annotation seeding also calls the backend REST API, so the backend must be healthy before beforeAll runs.
The @axe-core/playwright package is already in package.json at ^4.11.1. No new dependency is needed.
The Playwright image version in CI (v1.58.2-noble) and the local @playwright/test version should match. If they drift, the E2E job will fail on browser binary not found. The E2E CI job should use the same image version, or install via npx playwright install chromium inside the job.

Recommendations

File a separate DevOps issue to add the E2E CI job to ci.yml. Without it, this test is local-only and provides no merge protection. The E2E job pattern from devops.md — docker compose up -d db minio, backend start, npm run test:e2e — is the template.
In the E2E CI job, use docker compose -f docker-compose.yml -f docker-compose.ci.yml up -d if a CI overlay exists (it currently does not). If not, create a minimal overlay that disables bind mounts and uses ephemeral named volumes.
Use ${{ secrets.E2E_ADMIN_PASSWORD }} for the test credentials in CI, not a hardcoded value. The auth.setup.ts file reads credentials from environment variables — verify this is the case before adding the CI job.
The fullyParallel: false, workers: 1 setting in playwright.config.ts means the E2E suite runs serially. This keeps CI time predictable but means a slow test (PDF rendering can take 15–40s per annotations.spec.ts) will block subsequent tests. Monitor the total E2E time after adding this test — it should stay under 8 minutes.
The Playwright version in CI is pinned to v1.58.2-noble. Renovate should be configured to bump this when @playwright/test is updated, otherwise the CI image and the local package drift silently.

Open Decisions

Should the E2E CI job be in scope for this issue, or tracked separately? Adding it here makes the acceptance criteria complete; tracking it separately makes this issue smaller. Given that a test without a CI gate provides weak assurance, I lean toward making it in-scope.

## 👨‍💻 Tobias Wendt — DevOps & Platform Engineer ### Observations - The `ci.yml` pipeline has three jobs: `unit-tests`, `ocr-tests`, and `backend-unit-tests`. There is no E2E job. The `e2e/CLAUDE.md` explicitly states "E2E tests are **not** currently run in CI." This means the regression gate this issue intends to create does not exist at the merge level — the test can only be run locally. - The `unit-tests` CI job runs inside `mcr.microsoft.com/playwright:v1.58.2-noble`, which has Chromium pre-installed. The axe assertions in this issue are an E2E Playwright test, not a component test. Running E2E in CI requires a full stack: frontend dev server, Spring Boot backend, PostgreSQL, and MinIO. The existing CI jobs run with none of those services. - A minimal E2E CI job would need: `docker compose up -d db minio`, wait for health, start the backend JAR or Spring Boot via Maven, start the SvelteKit dev server, then run `npx playwright test`. The annotation seeding also calls the backend REST API, so the backend must be healthy before `beforeAll` runs. - The `@axe-core/playwright` package is already in `package.json` at `^4.11.1`. No new dependency is needed. - The Playwright image version in CI (`v1.58.2-noble`) and the local `@playwright/test` version should match. If they drift, the E2E job will fail on browser binary not found. The E2E CI job should use the same image version, or install via `npx playwright install chromium` inside the job. ### Recommendations - File a separate DevOps issue to add the E2E CI job to `ci.yml`. Without it, this test is local-only and provides no merge protection. The E2E job pattern from `devops.md` — `docker compose up -d db minio`, backend start, `npm run test:e2e` — is the template. - In the E2E CI job, use `docker compose -f docker-compose.yml -f docker-compose.ci.yml up -d` if a CI overlay exists (it currently does not). If not, create a minimal overlay that disables bind mounts and uses ephemeral named volumes. - Use `${{ secrets.E2E_ADMIN_PASSWORD }}` for the test credentials in CI, not a hardcoded value. The `auth.setup.ts` file reads credentials from environment variables — verify this is the case before adding the CI job. - The `fullyParallel: false, workers: 1` setting in `playwright.config.ts` means the E2E suite runs serially. This keeps CI time predictable but means a slow test (PDF rendering can take 15–40s per `annotations.spec.ts`) will block subsequent tests. Monitor the total E2E time after adding this test — it should stay under 8 minutes. - The Playwright version in CI is pinned to `v1.58.2-noble`. Renovate should be configured to bump this when `@playwright/test` is updated, otherwise the CI image and the local package drift silently. ### Open Decisions - Should the E2E CI job be in scope for this issue, or tracked separately? Adding it here makes the acceptance criteria complete; tracking it separately makes this issue smaller. Given that a test without a CI gate provides weak assurance, I lean toward making it in-scope.

marcel commented

2026-05-07 17:49:58 +02:00

👨‍💻 Elicit — Requirements Engineer

Observations

The acceptance criteria are well-formed: specific, testable, and unambiguous. The three criteria are each independently verifiable. This is above average for test-task issues.
The criterion "The test fixture includes a document with at least one annotation so the toggle button is rendered" is correct but leaves one edge case underdefined: should the test also assert the toggle button is in its "annotations hidden" state and re-run axe in that state? PdfControls.svelte renders differently depending on showAnnotations — both CSS states should be covered by the axe gate, since the contrast fix in PR #349 applies to the hidden state (the text-primary class).
The "Proposed implementation" block shows .withTags(['wcag2a', 'wcag2aa']) which is consistent with the project's stated NFR ("zero axe WCAG2AA violations"). However the scope is limited to "the PDF controls bar" — the .analyze() call without .include() will scan the entire page, not just the controls bar. This is actually better coverage than limiting to the bar, but the issue title says "PDF controls bar" — the scope should be clarified in the spec.
The issue references PR #349 and issue #341 but does not define a "Definition of Done" beyond the three acceptance criteria. The DoD should include: test is green on a clean run, test fails when text-primary is replaced with text-accent (proving the gate would catch the original regression).
The NFR "zero axe WCAG2AA violations on the PDF viewer page" is stated but the scope "PDF viewer page" includes more than the controls bar (annotations, transcription panel, document metadata). The test as proposed will cover the full page — this is fine but the criterion should say "document detail page" rather than "PDF controls bar" to match what is actually tested.

Recommendations

Add a fourth acceptance criterion: "The test verifies the controls bar in both the showAnnotations=true and showAnnotations=false states" — two separate test() calls or two axe runs in sequence with a toggle interaction between them.
Clarify the scope in the issue title or body: "PDF viewer page (full page scan)" vs. "PDF controls bar (scoped scan with .include()). Full page scan is the right choice — it catches regressions anywhere on the document detail page, not just in the controls bar.
Add a negative validation note to the DoD: "Manually verify the test fails when text-primary is changed to text-accent on the annotation toggle button." This proves the axe gate would have caught the original PR #349 regression.
The issue is labeled P2-medium. Given that the original regression was a WCAG AA contrast failure on a primary UI control used by 60+ year-old transcribers, consider whether P1-high is more appropriate.

## 👨‍💻 Elicit — Requirements Engineer ### Observations - The acceptance criteria are well-formed: specific, testable, and unambiguous. The three criteria are each independently verifiable. This is above average for test-task issues. - The criterion "The test fixture includes a document with at least one annotation so the toggle button is rendered" is correct but leaves one edge case underdefined: should the test also assert the toggle button is in its "annotations hidden" state and re-run axe in that state? `PdfControls.svelte` renders differently depending on `showAnnotations` — both CSS states should be covered by the axe gate, since the contrast fix in PR #349 applies to the hidden state (the `text-primary` class). - The "Proposed implementation" block shows `.withTags(['wcag2a', 'wcag2aa'])` which is consistent with the project's stated NFR ("zero axe WCAG2AA violations"). However the scope is limited to "the PDF controls bar" — the `.analyze()` call without `.include()` will scan the entire page, not just the controls bar. This is actually better coverage than limiting to the bar, but the issue title says "PDF controls bar" — the scope should be clarified in the spec. - The issue references PR #349 and issue #341 but does not define a "Definition of Done" beyond the three acceptance criteria. The DoD should include: test is green on a clean run, test fails when `text-primary` is replaced with `text-accent` (proving the gate would catch the original regression). - The NFR "zero axe WCAG2AA violations on the PDF viewer page" is stated but the scope "PDF viewer page" includes more than the controls bar (annotations, transcription panel, document metadata). The test as proposed will cover the full page — this is fine but the criterion should say "document detail page" rather than "PDF controls bar" to match what is actually tested. ### Recommendations - Add a fourth acceptance criterion: "The test verifies the controls bar in both the `showAnnotations=true` and `showAnnotations=false` states" — two separate `test()` calls or two axe runs in sequence with a toggle interaction between them. - Clarify the scope in the issue title or body: "PDF viewer page (full page scan)" vs. "PDF controls bar (scoped scan with `.include()`). Full page scan is the right choice — it catches regressions anywhere on the document detail page, not just in the controls bar. - Add a negative validation note to the DoD: "Manually verify the test fails when `text-primary` is changed to `text-accent` on the annotation toggle button." This proves the axe gate would have caught the original PR #349 regression. - The issue is labeled P2-medium. Given that the original regression was a WCAG AA contrast failure on a primary UI control used by 60+ year-old transcribers, consider whether P1-high is more appropriate.

marcel commented

2026-05-07 17:50:17 +02:00

👨‍💻 Nora "NullX" Steiner — Security Engineer

Observations

No direct security vulnerabilities are introduced by this change. The issue is adding a test, not changing application logic.
The annotation seeding in annotations.spec.ts makes unauthenticated API calls in beforeAll using the request fixture. The request fixture in Playwright inherits the storageState from the project configuration, which contains the admin session cookie. This is correct — the seeding runs as an authenticated admin. No credential exposure concern.
The proposed test will navigate to /documents/<id> using a dynamically created document ID. The document is created by the test and deleted by nothing — there is no afterAll cleanup in annotations.spec.ts either. Over time, CI runs accumulate test documents in the database. This is a data hygiene concern, not a security one, but worth noting.
The test credentials referenced in e2e/.auth/user.json are the dev admin credentials (admin@familyarchive.local / admin123 per the memory file). These credentials are committed to the test artifacts path. Confirm that e2e/.auth/ is in .gitignore so session tokens are not committed to git.
The axe accessibility tests do not introduce any new API surface, network calls to external services, or elevated privilege operations. Security posture is unchanged.

Recommendations

Verify e2e/.auth/ is in .gitignore. If not, add it. Session cookies committed to git are a credential leak, even for dev environments.
Add an afterAll cleanup to the new spec (and consider backfilling it to annotations.spec.ts) that deletes the test document via DELETE /api/documents/:id. This keeps the dev/CI database clean and prevents test pollution across runs.
When the CI E2E job is eventually added, use ${{ secrets.E2E_ADMIN_PASSWORD }} and never hardcode credentials in the workflow YAML. The auth.setup.ts should read from process.env.E2E_PASSWORD with a fallback to the dev default only in non-CI environments.
The @axe-core/playwright package at ^4.11.1 is a semver range. Pin it to an exact version in package.json for reproducible test results — axe rule updates can flip a passing test to failing between minor versions without any code change.

## 👨‍💻 Nora "NullX" Steiner — Security Engineer ### Observations - No direct security vulnerabilities are introduced by this change. The issue is adding a test, not changing application logic. - The annotation seeding in `annotations.spec.ts` makes unauthenticated API calls in `beforeAll` using the `request` fixture. The `request` fixture in Playwright inherits the `storageState` from the project configuration, which contains the admin session cookie. This is correct — the seeding runs as an authenticated admin. No credential exposure concern. - The proposed test will navigate to `/documents/<id>` using a dynamically created document ID. The document is created by the test and deleted by nothing — there is no `afterAll` cleanup in `annotations.spec.ts` either. Over time, CI runs accumulate test documents in the database. This is a data hygiene concern, not a security one, but worth noting. - The test credentials referenced in `e2e/.auth/user.json` are the dev admin credentials (`admin@familyarchive.local / admin123` per the memory file). These credentials are committed to the test artifacts path. Confirm that `e2e/.auth/` is in `.gitignore` so session tokens are not committed to git. - The axe accessibility tests do not introduce any new API surface, network calls to external services, or elevated privilege operations. Security posture is unchanged. ### Recommendations - Verify `e2e/.auth/` is in `.gitignore`. If not, add it. Session cookies committed to git are a credential leak, even for dev environments. - Add an `afterAll` cleanup to the new spec (and consider backfilling it to `annotations.spec.ts`) that deletes the test document via `DELETE /api/documents/:id`. This keeps the dev/CI database clean and prevents test pollution across runs. - When the CI E2E job is eventually added, use `${{ secrets.E2E_ADMIN_PASSWORD }}` and never hardcode credentials in the workflow YAML. The `auth.setup.ts` should read from `process.env.E2E_PASSWORD` with a fallback to the dev default only in non-CI environments. - The `@axe-core/playwright` package at `^4.11.1` is a semver range. Pin it to an exact version in `package.json` for reproducible test results — axe rule updates can flip a passing test to failing between minor versions without any code change.

marcel commented

2026-05-07 17:50:35 +02:00

👨‍💻 Sara Holt — QA Engineer & Test Strategist

Observations

This issue correctly diagnoses the gap in the test pyramid: the unit test asserts a class name (text-primary) but cannot verify the computed contrast ratio at runtime. The proposed axe E2E test is the correct layer for this assertion. The diagnosis is sound.
The existing axe check inside annotations.spec.ts (line 265–271) runs without .withTags(['wcag2a', 'wcag2aa']). This is a gap — it runs the full axe ruleset rather than the project's scoped WCAG 2.1 AA check. This should be fixed in the same PR as a cleanup, to align all axe calls with the pattern in accessibility.spec.ts.
The accessibility.spec.ts file tests five pages (home, persons, aktivitaeten, admin, admin-system) but not the document detail page. This is a significant gap even beyond the annotation toggle — the document detail page is one of the most complex pages in the application (PDF viewer, annotation layer, transcription panel, comments) and has never had an axe gate.
The existing annotations.spec.ts already seeds a document with annotations and runs an axe check inside the same describe block. The new spec in this issue would duplicate that fixture setup unless the helper is extracted. The test strategy should avoid two places that create annotated documents.
Test naming: the existing axe test in annotations.spec.ts is named 'transcribe mode passes axe accessibility check'. The new spec should follow the sentence pattern established in accessibility.spec.ts: 'document detail page has no wcag2a/wcag2aa violations with annotations visible'.
The proposed test runs axe once. It should run twice — once with annotations visible (default state) and once after clicking the toggle to hide them — since the annotation toggle button changes CSS class between the two states. Both states must pass.

Recommendations

Extract the annotated document seeding into e2e/helpers/create-annotated-document.ts. Both annotations.spec.ts and the new a11y spec should use this helper. This eliminates duplication and ensures fixture consistency.
Add both toggle states to the test: navigate, wait for PDF load, run axe (annotations visible), click the toggle button, run axe again (annotations hidden). This gives two data points and specifically verifies the contrast fix from PR #349 in both states.
Add [data-hydrated] wait and .tabular-nums wait before axe analysis, matching the timing pattern from annotations.spec.ts. Axe on a partially-rendered PDF page will produce false positives from elements that are not yet in the DOM.
Upgrade the scope in accessibility.spec.ts to include the document detail page. Use the same annotated document helper. This closes the broader gap, not just the annotation toggle gap.
The test should not be marked with test.skip or test.fixme — it must be green before the PR merges. The DoD is: test green on a full npm run test:e2e run.

## 👨‍💻 Sara Holt — QA Engineer & Test Strategist ### Observations - This issue correctly diagnoses the gap in the test pyramid: the unit test asserts a class name (`text-primary`) but cannot verify the computed contrast ratio at runtime. The proposed axe E2E test is the correct layer for this assertion. The diagnosis is sound. - The existing axe check inside `annotations.spec.ts` (line 265–271) runs without `.withTags(['wcag2a', 'wcag2aa'])`. This is a gap — it runs the full axe ruleset rather than the project's scoped WCAG 2.1 AA check. This should be fixed in the same PR as a cleanup, to align all axe calls with the pattern in `accessibility.spec.ts`. - The `accessibility.spec.ts` file tests five pages (home, persons, aktivitaeten, admin, admin-system) but not the document detail page. This is a significant gap even beyond the annotation toggle — the document detail page is one of the most complex pages in the application (PDF viewer, annotation layer, transcription panel, comments) and has never had an axe gate. - The existing `annotations.spec.ts` already seeds a document with annotations and runs an axe check inside the same `describe` block. The new spec in this issue would duplicate that fixture setup unless the helper is extracted. The test strategy should avoid two places that create annotated documents. - Test naming: the existing axe test in `annotations.spec.ts` is named `'transcribe mode passes axe accessibility check'`. The new spec should follow the sentence pattern established in `accessibility.spec.ts`: `'document detail page has no wcag2a/wcag2aa violations with annotations visible'`. - The proposed test runs axe once. It should run twice — once with annotations visible (default state) and once after clicking the toggle to hide them — since the annotation toggle button changes CSS class between the two states. Both states must pass. ### Recommendations - Extract the annotated document seeding into `e2e/helpers/create-annotated-document.ts`. Both `annotations.spec.ts` and the new a11y spec should use this helper. This eliminates duplication and ensures fixture consistency. - Add both toggle states to the test: navigate, wait for PDF load, run axe (annotations visible), click the toggle button, run axe again (annotations hidden). This gives two data points and specifically verifies the contrast fix from PR #349 in both states. - Add `[data-hydrated]` wait and `.tabular-nums` wait before axe analysis, matching the timing pattern from `annotations.spec.ts`. Axe on a partially-rendered PDF page will produce false positives from elements that are not yet in the DOM. - Upgrade the scope in `accessibility.spec.ts` to include the document detail page. Use the same annotated document helper. This closes the broader gap, not just the annotation toggle gap. - The test should not be marked with `test.skip` or `test.fixme` — it must be green before the PR merges. The DoD is: test green on a full `npm run test:e2e` run.

marcel commented

2026-05-07 17:50:47 +02:00

👨‍💻 Leonie Voss — UI/UX & Accessibility Strategist

Observations

This issue is a direct consequence of a design decision: using text-primary for the annotation toggle button when annotations are hidden. text-primary resolves to --c-primary, which was confirmed to pass WCAG AA contrast against the controls bar background. The regression risk identified here is real and worth gating — CSS variable values can change when design tokens are updated.
The PDF controls bar uses border-pdf-ctrl as its border token and bg-surface/10 (10% opacity surface color) as the button hover background. The annotation toggle button when annotations are hidden uses bg-surface/10 text-primary, and when annotations are visible uses text-ink-2 hover:bg-surface/10. These two states have different foreground colors and must both be verified — only checking one state misses a regression path.
The controls bar is the primary navigation tool for transcribers (the 60+ audience). These users are most affected by contrast failures. An automated axe gate on this specific UI region is critical, not just nice-to-have.
The toggle button includes an icon-only mode broken by aria-label — the label is set to the i18n string from m.pdf_annotations_hide() / m.pdf_annotations_show(). The axe scan will verify the aria-label is present and non-empty. This is a secondary benefit of the E2E axe gate.
The controls bar background context matters for contrast. The bg-surface/10 opacity on the button hover state means the actual rendered color depends on what is behind it (the PDF canvas). Axe evaluates contrast against the computed color in context — this is exactly what a unit test cannot do and why the E2E gate is the right tool.
Dark mode: the controls bar is within the PDF viewer which uses a dark-themed chrome (border-pdf-ctrl). Whether the PDF viewer respects the data-theme attribute or has a fixed dark palette should be verified. If the PDF viewer has a fixed dark background regardless of theme, the dark mode axe run must be done with the PDF viewer rendered.

Recommendations

Run axe in both light and dark mode for the document detail page, following the accessibility.spec.ts pattern of setting colorScheme: 'dark' via a new browser context. The PDF viewer chrome may have different contrast ratios in each mode.
After running axe with annotations visible, click the toggle button and run axe again. This directly covers the text-primary state that PR #349 fixed and ensures the gate catches future regressions to text-accent.
Consider adding the document detail page to AUTHENTICATED_PAGES in accessibility.spec.ts using the annotated document helper. This would extend the existing light/dark/system-dark matrix to cover the document page, rather than creating a second file that partially overlaps.
The aria-label on the toggle button uses i18n strings. Axe will check that the label is non-empty. If Paraglide fails to resolve the message (returns an empty string), the axe check catches it — this is a useful secondary gate for i18n correctness on accessibility-critical labels.

Open Decisions

Should the new axe spec be merged into accessibility.spec.ts (extending AUTHENTICATED_PAGES with a dynamically seeded document URL) or remain a standalone file? A standalone file keeps the annotation fixture lifecycle self-contained; merging gives a single authoritative list of axed pages. Both are valid — the choice affects how future pages are added to the a11y gate.

## 👨‍💻 Leonie Voss — UI/UX & Accessibility Strategist ### Observations - This issue is a direct consequence of a design decision: using `text-primary` for the annotation toggle button when annotations are hidden. `text-primary` resolves to `--c-primary`, which was confirmed to pass WCAG AA contrast against the controls bar background. The regression risk identified here is real and worth gating — CSS variable values can change when design tokens are updated. - The PDF controls bar uses `border-pdf-ctrl` as its border token and `bg-surface/10` (10% opacity surface color) as the button hover background. The annotation toggle button when annotations are hidden uses `bg-surface/10 text-primary`, and when annotations are visible uses `text-ink-2 hover:bg-surface/10`. These two states have different foreground colors and must both be verified — only checking one state misses a regression path. - The controls bar is the primary navigation tool for transcribers (the 60+ audience). These users are most affected by contrast failures. An automated axe gate on this specific UI region is critical, not just nice-to-have. - The toggle button includes an icon-only mode broken by `aria-label` — the label is set to the i18n string from `m.pdf_annotations_hide()` / `m.pdf_annotations_show()`. The axe scan will verify the `aria-label` is present and non-empty. This is a secondary benefit of the E2E axe gate. - The controls bar background context matters for contrast. The `bg-surface/10` opacity on the button hover state means the actual rendered color depends on what is behind it (the PDF canvas). Axe evaluates contrast against the computed color in context — this is exactly what a unit test cannot do and why the E2E gate is the right tool. - Dark mode: the controls bar is within the PDF viewer which uses a dark-themed chrome (`border-pdf-ctrl`). Whether the PDF viewer respects the `data-theme` attribute or has a fixed dark palette should be verified. If the PDF viewer has a fixed dark background regardless of theme, the dark mode axe run must be done with the PDF viewer rendered. ### Recommendations - Run axe in both light and dark mode for the document detail page, following the `accessibility.spec.ts` pattern of setting `colorScheme: 'dark'` via a new browser context. The PDF viewer chrome may have different contrast ratios in each mode. - After running axe with annotations visible, click the toggle button and run axe again. This directly covers the `text-primary` state that PR #349 fixed and ensures the gate catches future regressions to `text-accent`. - Consider adding the document detail page to `AUTHENTICATED_PAGES` in `accessibility.spec.ts` using the annotated document helper. This would extend the existing light/dark/system-dark matrix to cover the document page, rather than creating a second file that partially overlaps. - The `aria-label` on the toggle button uses i18n strings. Axe will check that the label is non-empty. If Paraglide fails to resolve the message (returns an empty string), the axe check catches it — this is a useful secondary gate for i18n correctness on accessibility-critical labels. ### Open Decisions - Should the new axe spec be merged into `accessibility.spec.ts` (extending `AUTHENTICATED_PAGES` with a dynamically seeded document URL) or remain a standalone file? A standalone file keeps the annotation fixture lifecycle self-contained; merging gives a single authoritative list of axed pages. Both are valid — the choice affects how future pages are added to the a11y gate.

marcel commented

2026-05-07 17:51:12 +02:00

Decision Queue

Consolidated open decisions raised across the persona reviews. Each needs a human call before implementation starts.

Theme 1 — CI Gate Scope

Raised by: Markus (Architect), Tobias (DevOps)

Should the E2E CI job be in scope for this issue, or tracked as a separate issue?

Option A (in-scope): This issue is not done until the test runs on every PR. The regression gate is meaningless without CI. Adds a full-stack Docker Compose setup to the issue's scope.
Option B (separate issue): Ship the local test now; file a follow-up DevOps issue for the CI E2E job. Smaller PR, but the test provides no merge protection until the follow-up lands.

The project currently has zero E2E CI coverage. Option A closes a systemic gap; Option B defers it. Markus and Tobias both lean toward Option A being the right long-term answer but acknowledge it doubles the issue's size.

Theme 2 — New File vs. Extend `accessibility.spec.ts`

Raised by: Leonie (UX), Sara (QA)

Should the new axe test live in a dedicated pdf-controls-a11y.spec.ts (or similar), or should the document detail page be added to the AUTHENTICATED_PAGES list in the existing accessibility.spec.ts?

Standalone file: Keeps the annotation fixture lifecycle (beforeAll seeding, afterAll cleanup) self-contained. Easier to read in isolation. Adds one more file to navigate.
Extend accessibility.spec.ts: Single authoritative list of pages with axe coverage. Requires passing a dynamically seeded document URL into a module-level array, which is slightly awkward with the current structure. Consistent pattern for future pages.

Both options are valid. The choice sets a precedent for how future pages with fixture requirements get added to the a11y gate.

Theme 3 — Priority Label

Raised by: Elicit (Requirements)

The issue is labeled P2-medium. The original regression was a WCAG AA contrast failure on the primary UI control for the 60+ transcriber audience. Should this be escalated to P1-high?

Keep P2: The contrast is now fixed (PR #349). This issue adds a test to prevent future regression, not to fix a live problem.
Escalate to P1: A future regression would silently re-introduce an accessibility barrier for the most affected user group, with no automated detection. The test gate is high-value.

## Decision Queue Consolidated open decisions raised across the persona reviews. Each needs a human call before implementation starts. --- ### Theme 1 — CI Gate Scope **Raised by:** Markus (Architect), Tobias (DevOps) Should the E2E CI job be in scope for this issue, or tracked as a separate issue? - **Option A (in-scope):** This issue is not done until the test runs on every PR. The regression gate is meaningless without CI. Adds a full-stack Docker Compose setup to the issue's scope. - **Option B (separate issue):** Ship the local test now; file a follow-up DevOps issue for the CI E2E job. Smaller PR, but the test provides no merge protection until the follow-up lands. The project currently has zero E2E CI coverage. Option A closes a systemic gap; Option B defers it. Markus and Tobias both lean toward Option A being the right long-term answer but acknowledge it doubles the issue's size. --- ### Theme 2 — New File vs. Extend `accessibility.spec.ts` **Raised by:** Leonie (UX), Sara (QA) Should the new axe test live in a dedicated `pdf-controls-a11y.spec.ts` (or similar), or should the document detail page be added to the `AUTHENTICATED_PAGES` list in the existing `accessibility.spec.ts`? - **Standalone file:** Keeps the annotation fixture lifecycle (`beforeAll` seeding, `afterAll` cleanup) self-contained. Easier to read in isolation. Adds one more file to navigate. - **Extend `accessibility.spec.ts`:** Single authoritative list of pages with axe coverage. Requires passing a dynamically seeded document URL into a module-level array, which is slightly awkward with the current structure. Consistent pattern for future pages. Both options are valid. The choice sets a precedent for how future pages with fixture requirements get added to the a11y gate. --- ### Theme 3 — Priority Label **Raised by:** Elicit (Requirements) The issue is labeled P2-medium. The original regression was a WCAG AA contrast failure on the primary UI control for the 60+ transcriber audience. Should this be escalated to P1-high? - **Keep P2:** The contrast is now fixed (PR #349). This issue adds a test to prevent future regression, not to fix a live problem. - **Escalate to P1:** A future regression would silently re-introduce an accessibility barrier for the most affected user group, with no automated detection. The test gate is high-value.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#353

test(a11y): add axe-playwright E2E gate for PDF viewer WCAG 2.1 AA compliance #353

Problem

Acceptance Criteria

Proposed implementation

Background

👨‍💻 Markus Keller — Application Architect

Observations

Recommendations

Open Decisions

👨‍💻 Felix Brandt — Fullstack Developer

Observations

Recommendations

👨‍💻 Tobias Wendt — DevOps & Platform Engineer

Observations

Recommendations

Open Decisions

👨‍💻 Elicit — Requirements Engineer

Observations

Recommendations

👨‍💻 Nora "NullX" Steiner — Security Engineer

Observations

Recommendations

👨‍💻 Sara Holt — QA Engineer & Test Strategist

Observations

Recommendations

👨‍💻 Leonie Voss — UI/UX & Accessibility Strategist

Observations

Recommendations

Open Decisions

Decision Queue

Theme 1 — CI Gate Scope

Theme 2 — New File vs. Extend accessibility.spec.ts

Theme 3 — Priority Label

Theme 2 — New File vs. Extend `accessibility.spec.ts`