fix(a11y): replace clickable divs with button elements in PdfViewer and AnnotationLayer #114

New Issue

marcel · 2026-03-27T17:53:23+01:00

marcel commented

2026-03-27 17:53:23 +01:00

Problem

PdfViewer.svelte and AnnotationLayer.svelte use <div onclick={...}> for interactive elements. These are invisible to keyboard navigation and screen readers — users who cannot use a mouse cannot interact with annotations.

<!-- Current — inaccessible -->
<div onclick={() => (showAnnotations = !showAnnotations)}>...</div>

Fix

Replace all interactive <div> elements with <button>, or add the minimum required attributes where a <button> would break layout:

<!-- Option A: semantic button (preferred) -->
<button
  onclick={() => (showAnnotations = !showAnnotations)}
  aria-pressed={showAnnotations}
  aria-label={m.pdf_toggle_annotations()}
  class="..."
>
  ...
</button>

<!-- Option B: when layout requires a div -->
<div
  role="button"
  tabindex="0"
  onclick={handler}
  onkeydown={(e) => e.key === 'Enter' || e.key === ' ' ? handler() : null}
  aria-label="..."
>

Affected Files

src/lib/components/PdfViewer.svelte
src/lib/components/AnnotationLayer.svelte (if applicable)

Acceptance Criteria

All interactive click targets are reachable via Tab key
Enter and Space activate the controls
Screen reader announces the control purpose via aria-label
Existing mouse behaviour unchanged

## Problem `PdfViewer.svelte` and `AnnotationLayer.svelte` use `<div onclick={...}>` for interactive elements. These are invisible to keyboard navigation and screen readers — users who cannot use a mouse cannot interact with annotations. ```svelte  <div onclick={() => (showAnnotations = !showAnnotations)}>...</div> ``` ## Fix Replace all interactive `<div>` elements with `<button>`, or add the minimum required attributes where a `<button>` would break layout: ```svelte  <button onclick={() => (showAnnotations = !showAnnotations)} aria-pressed={showAnnotations} aria-label={m.pdf_toggle_annotations()} class="..." > ... </button>  <div role="button" tabindex="0" onclick={handler} onkeydown={(e) => e.key === 'Enter' || e.key === ' ' ? handler() : null} aria-label="..." > ``` ## Affected Files - `src/lib/components/PdfViewer.svelte` - `src/lib/components/AnnotationLayer.svelte` (if applicable) ## Acceptance Criteria - All interactive click targets are reachable via Tab key - Enter and Space activate the controls - Screen reader announces the control purpose via `aria-label` - Existing mouse behaviour unchanged

marcel added the bug ui labels 2026-03-31 20:49:48 +02:00

marcel commented

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

👨‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

Option A (semantic <button>) is always preferred. A <button> gets keyboard support, focus management, and screen reader semantics for free. Only fall back to Option B (div + role="button") if there is a demonstrated layout breakage — and even then, challenge whether the layout constraint is real.
Replacing a <div> with a <button> in a PDF viewer context often causes styling issues because browsers apply default button styles (border, background, padding). The implementation must include an explicit CSS reset or Tailwind reset classes (appearance-none bg-transparent border-0 p-0 cursor-pointer).
The onkeydown handler in Option B is incomplete — it only handles Enter and Space but misses keyup for Space (standard button behaviour fires on keyup for space). If using Option A, this is handled automatically by the browser.

Suggestions

Test with @testing-library/svelte: render the component, getByRole('button', { name: /annotations/i }), then userEvent.keyboard('{Enter}') and userEvent.keyboard(' ') — assert the toggle fires for both.
Also verify aria-pressed toggles correctly: after activation it should reflect the new state.
Check that existing mouse behaviour tests (if any) still pass after the swap — the onclick handler itself doesn't change, so they should.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Questions & Observations - Option A (semantic `<button>`) is always preferred. A `<button>` gets keyboard support, focus management, and screen reader semantics for free. Only fall back to Option B (div + role="button") if there is a demonstrated layout breakage — and even then, challenge whether the layout constraint is real. - Replacing a `<div>` with a `<button>` in a PDF viewer context often causes styling issues because browsers apply default button styles (border, background, padding). The implementation must include an explicit CSS reset or Tailwind reset classes (`appearance-none bg-transparent border-0 p-0 cursor-pointer`). - The `onkeydown` handler in Option B is incomplete — it only handles `Enter` and `Space` but misses `keyup` for Space (standard button behaviour fires on `keyup` for space). If using Option A, this is handled automatically by the browser. ### Suggestions - Test with `@testing-library/svelte`: render the component, `getByRole('button', { name: /annotations/i })`, then `userEvent.keyboard('{Enter}')` and `userEvent.keyboard(' ')` — assert the toggle fires for both. - Also verify `aria-pressed` toggles correctly: after activation it should reflect the new state. - Check that existing mouse behaviour tests (if any) still pass after the swap — the `onclick` handler itself doesn't change, so they should.

marcel commented

2026-03-31 21:00:57 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Questions & Observations

Accessibility and security overlap here. Clickable <div> elements are occasionally used to bypass CSP script-src restrictions via event handler injection — the attack surface is narrow but real. Semantic <button> elements are also harder to target with certain clickjacking techniques. This is a minor security co-benefit of the a11y fix.
aria-label must not be user-controlled. The issue correctly uses m.pdf_toggle_annotations() (a Paraglide i18n function) for the label. Confirm this value is statically defined in the locale files and never derived from user input — a user-controlled aria-label could be a minor XSS vector if the value is ever rendered unsanitised elsewhere.
Focus trapping in the PDF viewer. Once keyboard users can reach the annotation controls, verify there's no focus trap that prevents them from tabbing out of the viewer entirely. A PDF viewer that captures all keyboard events (common in PDF.js integrations) can strand keyboard users.

Suggestions

No critical security findings — this is primarily an accessibility issue. The main security note is the focus trap check above, which should be part of the acceptance criteria.
After implementation, run axe-playwright on the document detail page and confirm zero WCAG violations for keyboard accessibility.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Questions & Observations - **Accessibility and security overlap here.** Clickable `<div>` elements are occasionally used to bypass CSP `script-src` restrictions via event handler injection — the attack surface is narrow but real. Semantic `<button>` elements are also harder to target with certain clickjacking techniques. This is a minor security co-benefit of the a11y fix. - **`aria-label` must not be user-controlled.** The issue correctly uses `m.pdf_toggle_annotations()` (a Paraglide i18n function) for the label. Confirm this value is statically defined in the locale files and never derived from user input — a user-controlled `aria-label` could be a minor XSS vector if the value is ever rendered unsanitised elsewhere. - **Focus trapping in the PDF viewer.** Once keyboard users can reach the annotation controls, verify there's no focus trap that prevents them from tabbing out of the viewer entirely. A PDF viewer that captures all keyboard events (common in PDF.js integrations) can strand keyboard users. ### Suggestions - No critical security findings — this is primarily an accessibility issue. The main security note is the focus trap check above, which should be part of the acceptance criteria. - After implementation, run `axe-playwright` on the document detail page and confirm zero WCAG violations for keyboard accessibility.

marcel commented

2026-03-31 21:02:48 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Test Strategy

Component tests (@testing-library/svelte):

it('annotation toggle button is reachable by keyboard', async () => {
  render(PdfViewer, { props: { src: '/test.pdf' } });
  const btn = screen.getByRole('button', { name: /annotations/i });
  expect(btn).toBeInTheDocument();
  await userEvent.tab();
  expect(btn).toHaveFocus();
});

it('activates toggle on Enter key', async () => {
  render(PdfViewer, { props: { src: '/test.pdf' } });
  const btn = screen.getByRole('button', { name: /annotations/i });
  await userEvent.type(btn, '{Enter}');
  expect(btn).toHaveAttribute('aria-pressed', 'true');
});

it('activates toggle on Space key', async () => {
  render(PdfViewer, { props: { src: '/test.pdf' } });
  const btn = screen.getByRole('button', { name: /annotations/i });
  await userEvent.type(btn, ' ');
  expect(btn).toHaveAttribute('aria-pressed', 'true');
});

E2E (Playwright + axe):

test('document viewer has no accessibility violations', async ({ page }) => {
  await page.goto('/documents/some-id');
  await checkA11y(page); // should catch any remaining clickable divs
});

Edge Cases to Cover

All interactive elements in AnnotationLayer.svelte must be checked — not just the toggle in PdfViewer.svelte
Verify focus ring is visible on the converted buttons (not suppressed by a CSS reset)

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Test Strategy **Component tests (`@testing-library/svelte`):** ```typescript it('annotation toggle button is reachable by keyboard', async () => { render(PdfViewer, { props: { src: '/test.pdf' } }); const btn = screen.getByRole('button', { name: /annotations/i }); expect(btn).toBeInTheDocument(); await userEvent.tab(); expect(btn).toHaveFocus(); }); it('activates toggle on Enter key', async () => { render(PdfViewer, { props: { src: '/test.pdf' } }); const btn = screen.getByRole('button', { name: /annotations/i }); await userEvent.type(btn, '{Enter}'); expect(btn).toHaveAttribute('aria-pressed', 'true'); }); it('activates toggle on Space key', async () => { render(PdfViewer, { props: { src: '/test.pdf' } }); const btn = screen.getByRole('button', { name: /annotations/i }); await userEvent.type(btn, ' '); expect(btn).toHaveAttribute('aria-pressed', 'true'); }); ``` **E2E (Playwright + axe):** ```typescript test('document viewer has no accessibility violations', async ({ page }) => { await page.goto('/documents/some-id'); await checkA11y(page); // should catch any remaining clickable divs }); ``` ### Edge Cases to Cover - All interactive elements in `AnnotationLayer.svelte` must be checked — not just the toggle in `PdfViewer.svelte` - Verify focus ring is visible on the converted buttons (not suppressed by a CSS reset)

marcel commented

2026-03-31 21:04:46 +02:00

🏗️ Markus Keller — Application Architect

Questions & Observations

No architectural concerns. This is a component-internal fix — it doesn't cross module boundaries, doesn't affect the API, and doesn't require any backend changes. The scope is exactly right.
Option A is non-negotiable. A <div role="button"> with a tabindex and an onkeydown handler is three attributes doing the work that one <button> element does natively. Option B exists for documentation completeness, but in practice there is almost never a layout reason to prefer it. The comment in the issue is correct: challenge the layout constraint before reaching for Option B.
PDF.js integration concern. PDF.js viewers sometimes intercept keyboard events at a canvas or container level. After the fix, manually verify that Tab actually reaches the new <button> elements — the keyboard event handling in PDF.js can absorb keystrokes before they reach the DOM. This is a runtime concern, not a code concern.

Suggestions

Scope this PR tightly: only PdfViewer.svelte and AnnotationLayer.svelte. Don't sweep other components for similar patterns in the same PR — keep the diff reviewable.
If there are other interactive <div> elements elsewhere in the codebase, open a separate issue for them rather than bundling into this one.

## 🏗️ Markus Keller — Application Architect ### Questions & Observations - **No architectural concerns.** This is a component-internal fix — it doesn't cross module boundaries, doesn't affect the API, and doesn't require any backend changes. The scope is exactly right. - **Option A is non-negotiable.** A `<div role="button">` with a `tabindex` and an `onkeydown` handler is three attributes doing the work that one `<button>` element does natively. Option B exists for documentation completeness, but in practice there is almost never a layout reason to prefer it. The comment in the issue is correct: challenge the layout constraint before reaching for Option B. - **PDF.js integration concern.** PDF.js viewers sometimes intercept keyboard events at a canvas or container level. After the fix, manually verify that `Tab` actually reaches the new `<button>` elements — the keyboard event handling in PDF.js can absorb keystrokes before they reach the DOM. This is a runtime concern, not a code concern. ### Suggestions - Scope this PR tightly: only `PdfViewer.svelte` and `AnnotationLayer.svelte`. Don't sweep other components for similar patterns in the same PR — keep the diff reviewable. - If there are other interactive `<div>` elements elsewhere in the codebase, open a separate issue for them rather than bundling into this one.

marcel referenced this issue

2026-03-31 21:09:23 +02:00

fix(a11y): add skip-to-main-content link in layout for keyboard navigation #117

marcel commented

2026-03-31 21:12:08 +02:00

🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist

Questions & Observations

This is critical for our senior users. Keyboard navigation is not a niche use case — it's essential for anyone with motor difficulties, anyone using a screen reader, and anyone who simply prefers keyboard-driven workflows. The current clickable <div> pattern is invisible to all of them.
Focus ring must be visible. When the <div> is replaced with a <button>, the browser applies a default focus ring. However, this project likely has CSS resets (Tailwind's preflight base styles reset button appearance). Explicitly verify the focus state is visible after the change — check both the default browser ring and any custom focus styles in the existing CSS. outline: none without a replacement is a WCAG 2.4.7 violation.
aria-label language. The annotation toggle label must be translated — m.pdf_toggle_annotations() is correct. Make sure all three locale files have a meaningful label, not just English. For German: "Anmerkungen ein-/ausblenden"; Spanish: "Mostrar/ocultar anotaciones".
aria-pressed state must update on toggle. Screen readers announce the pressed state to users — aria-pressed={showAnnotations} must be reactive and reflect the actual current state.

Suggestions

Touch target: the annotation toggle button must be at least 44×44px. PDF viewer controls are often small by default — verify dimensions explicitly. Use min-w-[44px] min-h-[44px] if the natural size is smaller.

## 🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist ### Questions & Observations - **This is critical for our senior users.** Keyboard navigation is not a niche use case — it's essential for anyone with motor difficulties, anyone using a screen reader, and anyone who simply prefers keyboard-driven workflows. The current clickable `<div>` pattern is invisible to all of them. - **Focus ring must be visible.** When the `<div>` is replaced with a `<button>`, the browser applies a default focus ring. However, this project likely has CSS resets (Tailwind's `preflight` base styles reset button appearance). Explicitly verify the focus state is visible after the change — check both the default browser ring and any custom focus styles in the existing CSS. `outline: none` without a replacement is a WCAG 2.4.7 violation. - **`aria-label` language.** The annotation toggle label must be translated — `m.pdf_toggle_annotations()` is correct. Make sure all three locale files have a meaningful label, not just English. For German: *"Anmerkungen ein-/ausblenden"*; Spanish: *"Mostrar/ocultar anotaciones"*. - **`aria-pressed` state must update on toggle.** Screen readers announce the pressed state to users — `aria-pressed={showAnnotations}` must be reactive and reflect the actual current state. ### Suggestions - Touch target: the annotation toggle button must be at least 44×44px. PDF viewer controls are often small by default — verify dimensions explicitly. Use `min-w-[44px] min-h-[44px]` if the natural size is smaller.

marcel commented

2026-03-31 21:13:41 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

No infrastructure concerns — this is a frontend component change with no deployment, config, or Docker impact.

One CI note: if axe-playwright is added to the E2E suite as part of this issue (which it should be), make sure the E2E job in the Gitea Actions workflow has the axe-playwright package installed. Check the package.json devDependencies and the CI workflow's npm install step — no separate action needed, it installs with the rest of the dependencies.

## 🚀 Tobias Wendt — DevOps & Platform Engineer No infrastructure concerns — this is a frontend component change with no deployment, config, or Docker impact. One CI note: if axe-playwright is added to the E2E suite as part of this issue (which it should be), make sure the E2E job in the Gitea Actions workflow has the axe-playwright package installed. Check the `package.json` devDependencies and the CI workflow's npm install step — no separate action needed, it installs with the rest of the dependencies.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#114