From 7b9a5eb01a31d6426c24db6658b639ea0fefe7e3 Mon Sep 17 00:00:00 2001 From: John McLear Date: Fri, 24 Apr 2026 03:04:18 +0100 Subject: [PATCH] fix(a11y): dialog semantics, focus management, icon labels, html lang (#7584) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(a11y): negotiate lang/dir per request and set on Server-renders the html element with `lang` and `dir` matching the client's Accept-Language header (negotiated against availableLangs from i18n hooks). Falls back to `en`/`ltr` if no match. This gives screen readers a correct document language during the brief window before client-side html10n refines it (l10n.ts already sets both attributes after locale data loads). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(a11y): dialog semantics on popups; fix aria-role typo on userlist Adds role=dialog, aria-modal=true, and either aria-labelledby (when an h1 is present) or aria-label (for popups without an h1) to: - #settings, #import_export, #embed, #skin-variants (labelledby) - #connectivity, #users, #mycolorpicker (aria-label) Fixes the invalid aria-role="document" attribute on #otherusers; it's now role=region with aria-live=polite so screen readers announce collaborator joins/leaves. Container aria-label values are English-only for now — Etherpad's html10n implementation only supports localizing specific attributes (title, alt, placeholder, etc), not aria-label on container nodes. Localization can follow once html10n grows that affordance. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(a11y): focus management and Escape-to-close for popups Three additions to toggleDropDown / _bodyKeyEvent: - Remember the trigger element (document.activeElement) when opening a popup, so we can restore focus when it closes. - On open, focus the first focusable element inside the popup so keyboard users land inside the dialog instead of staying on the trigger button. - Escape pressed while focus is inside a popup closes it, then the restore-focus path runs and the trigger button is refocused. Replaces the previous behavior where Escape from inside a popup did nothing; users had to click outside to dismiss. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(a11y): make chaticon and chat header controls real buttons - #chaticon:
→ + ``` + Move the `onclick="chat.show();return false;"` to a JS handler in `chat.ts` `init()` (find existing init): + ```ts + $('#chaticon').on('click', (e) => { e.preventDefault(); chat.show(); }); + ``` + (If `chat.show()` is already wired by another listener, just remove the inline `onclick` and rely on the existing handler — confirm by greping.) + +- [ ] **Step 4.3:** Replace chat header close/stick anchors: + ```html + + + ``` + Move their inline `onClick` handlers to `chat.ts`: + ```ts + $('#titlecross').on('click', (e) => { e.preventDefault(); chat.hide(); }); + $('#titlesticky').on('click', (e) => { e.preventDefault(); chat.stickToScreen(true); }); + ``` + +- [ ] **Step 4.4:** Inspect CSS for `#chaticon` / `#titlecross` / `#titlesticky`. Buttons get default browser styling (border, padding) that may break the layout. Add a CSS reset block in `src/static/css/pad/chat.css` (or wherever those selectors already live): + ```css + #chaticon, #titlecross, #titlesticky { + background: transparent; + border: 0; + padding: 0; + font: inherit; + color: inherit; + cursor: pointer; + } + #chaticon:focus-visible, #titlecross:focus-visible, #titlesticky:focus-visible { + outline: 2px solid #0066cc; + outline-offset: 2px; + } + ``` + Find the right file by grepping `#chaticon` in `src/static/css`. + +- [ ] **Step 4.5:** ts-check: + ```bash + pnpm --dir src run ts-check + ``` + +- [ ] **Step 4.6:** Commit: + ```bash + git add src/templates/pad.html src/static/js/chat.ts src/static/css + git commit -m "fix(a11y): make chaticon and chat header controls real buttons" + ``` + +--- + +### Task 5: Add `icon.*` locale namespace and label icon-only controls + +**Files:** +- Modify: `src/locales/en.json` — add new keys +- Modify: `src/templates/pad.html` — apply `data-l10n-id` to `aria-label` on icon-only elements + +html10n supports per-attribute translation via `key.attr` style — for `aria-label`, the convention used elsewhere in this codebase is `data-l10n-id="key"` plus a sibling key `key.aria-label`. Check existing usage by grepping `aria-label` in `src/locales/en.json`: + +- [ ] **Step 5.1:** Grep current usage: + ```bash + grep -n "aria-label\|.title" src/locales/en.json | head -20 + ``` + Determine the convention. If html10n uses `{key}.aria-label`, follow that. Otherwise use plain `key` and apply via `aria-label` directly in HTML (no l10n on the aria-label) and accept English-only for now. + +- [ ] **Step 5.2:** Add to `src/locales/en.json` after the `pad.chat.*` block: + ```json + "pad.icon.export.etherpad": "Export as Etherpad", + "pad.icon.export.html": "Export as HTML", + "pad.icon.export.plain": "Export as plain text", + "pad.icon.export.word": "Export as Microsoft Word", + "pad.icon.export.pdf": "Export as PDF", + "pad.icon.export.opendocument": "Export as ODF", + "pad.icon.showmore": "Show more toolbar buttons", + ``` + (Insert with correct JSON commas.) + +- [ ] **Step 5.3:** Apply to the export `` elements in `src/templates/pad.html:215-232`: + ```html + + ``` + Repeat per format. Add `aria-hidden="true"` to the inner `` since the link itself now carries the label. + +- [ ] **Step 5.4:** Convert the show-more span to a button on `pad.html:74`: + ```html + + ``` + Verify CSS targeting `.show-more-icon-btn` doesn't depend on element type — grep first. + +- [ ] **Step 5.5:** Theme switcher knob (`pad.html:172`) currently has `aria-label="theme-switcher-knob"` which is a CSS-class-style label, not human text. Change to `aria-label="Toggle theme"`. + +- [ ] **Step 5.6:** Commit: + ```bash + git add src/locales/en.json src/templates/pad.html + git commit -m "fix(a11y): accessible names for icon-only buttons and links" + ``` + +--- + +### Task 6: Playwright test for dialog semantics + Escape + +**Files:** +- Create: `src/tests/frontend-new/specs/a11y_dialogs.spec.ts` + +Cover the high-impact promises: settings popup opens with role=dialog, Escape closes it, focus returns to trigger. + +- [ ] **Step 6.1:** Write the failing test: + ```ts + import {expect, test} from "@playwright/test"; + import {goToNewPad} from "../helper/padHelper"; + + test.beforeEach(async ({page}) => { await goToNewPad(page); }); + + test('settings popup has dialog semantics and Escape closes it', async ({page}) => { + const settingsButton = page.locator('.buttonicon.buttonicon-cog'); + await settingsButton.click(); + + const dialog = page.locator('#settings'); + await expect(dialog).toHaveAttribute('role', 'dialog'); + await expect(dialog).toHaveAttribute('aria-modal', 'true'); + await expect(dialog).toBeVisible(); + + await page.keyboard.press('Escape'); + await expect(dialog).toBeHidden(); + + // Focus should return to the trigger + const focused = await page.evaluate(() => document.activeElement?.className || ''); + expect(focused).toContain('buttonicon-cog'); + }); + + test('html element has lang attribute', async ({page}) => { + const lang = await page.locator('html').getAttribute('lang'); + expect(lang).toBeTruthy(); + expect(lang!.length).toBeGreaterThan(0); + }); + + test('export links have accessible names', async ({page}) => { + await page.locator('.buttonicon.buttonicon-import_export').click(); + const pdfLink = page.locator('#exportpdfa'); + const label = await pdfLink.getAttribute('aria-label'); + expect(label).toBeTruthy(); + }); + + test('chaticon is a button with accessible name', async ({page}) => { + const chatIcon = page.locator('#chaticon'); + const tagName = await chatIcon.evaluate(el => el.tagName.toLowerCase()); + expect(tagName).toBe('button'); + const label = await chatIcon.getAttribute('aria-label'); + expect(label).toBeTruthy(); + }); + ``` + +- [ ] **Step 6.2:** Verify the Playwright spec runs (headless per project rule): + ```bash + cd src && pnpm exec playwright test tests/frontend-new/specs/a11y_dialogs.spec.ts --reporter=list + ``` + Expected: all 4 tests pass. + +- [ ] **Step 6.3:** Commit: + ```bash + git add src/tests/frontend-new/specs/a11y_dialogs.spec.ts + git commit -m "test(a11y): verify dialog semantics, html lang, export labels, chat button" + ``` + +--- + +### Task 7: Run the full local checks before push + +- [ ] **Step 7.1:** ts-check from `src/`: + ```bash + pnpm --dir src run ts-check + ``` + +- [ ] **Step 7.2:** Backend tests: + ```bash + pnpm --dir src run test:backend + ``` + +- [ ] **Step 7.3:** Push and open a PR against `johnmclear/etherpad-lite`: + ```bash + git push -u fork fix/a11y-dialogs-labels-lang + gh pr create --repo johnmclear/etherpad-lite --base develop --head fix/a11y-dialogs-labels-lang \ + --title "fix(a11y): dialog semantics, icon labels, html lang" \ + --body "..." + ``` + +- [ ] **Step 7.4:** Post `/review` comment on the PR to trigger Qodo. + +--- + +## Self-review notes + +- **Spec coverage:** Original audit's high-impact items were (a) dialog semantics + focus trap, (b) aria-labels via icon.* namespace, (c) html lang. All three covered (Tasks 2+3, 4+5, 1). Bonus: aria-role typo on userlist (Task 2.8) and chat header buttons (Task 4.3). +- **Out of scope, deliberately:** focus-visible CSS sweep, contrast pass, touch-target sizing, full focus-trap library (we do simple init-focus + Escape, not Tab cycling — adequate for these short popups, library can come later). +- **Risk:** Adding `hidden` attribute to popups changes initial render — confirmed CSS does not depend on absence of `hidden` (CSS uses `.popup-show` to display). Need to check that `display: none` from `.popup` (default) and `hidden` don't conflict in unwanted ways; `hidden` is a stronger signal and should be fine. diff --git a/src/static/css/pad/chat.css b/src/static/css/pad/chat.css index bfda0109a..91d7312a4 100644 --- a/src/static/css/pad/chat.css +++ b/src/static/css/pad/chat.css @@ -76,11 +76,21 @@ text-align: right; text-decoration: none; cursor: pointer; + background: transparent; + border: 0; + padding: 0; + font-family: inherit; + line-height: 1; } #titlebar .stick-to-screen-btn { font-size: 10px; padding-top: 2px; } +#titlebar .stick-to-screen-btn:focus-visible, +#titlebar .hide-reduce-btn:focus-visible { + outline: 2px solid #0066cc; + outline-offset: 2px; +} /* -- MESSAGES -- */ #chattext { @@ -121,10 +131,32 @@ /* -- CHAT ICON -- */ #chaticon { + /* #chaticon was converted from to
<% e.begin_block("afterEditbar"); %><% e.end_block(); %> @@ -114,11 +117,11 @@ -