fix(a11y): dialog semantics, focus management, icon labels, html lang#7584
Open
JohnMcLear wants to merge 9 commits intoether:developfrom
Open
fix(a11y): dialog semantics, focus management, icon labels, html lang#7584JohnMcLear wants to merge 9 commits intoether:developfrom
JohnMcLear wants to merge 9 commits intoether:developfrom
Conversation
Member
Author
|
/review |
Code Review by Qodo
1. Stale aria-label after relocalize
|
Review Summary by QodoAccessibility: dialog semantics, focus management, icon labels, html lang
WalkthroughsDescription• Add lang and dir attributes to HTML templates negotiated from Accept-Language header • Implement dialog semantics (role, aria-modal, aria-labelledby/aria-label) on all popups • Add focus management: remember trigger element, focus first focusable in popup, restore focus on close • Enable Escape key to close open popups and restore focus to trigger button • Convert icon-only controls to semantic buttons with accessible names (chat icon, close/pin buttons, show-more) • Add aria-labels to export links and fix aria-role typo on userlist region • Add comprehensive Playwright tests for dialog semantics, focus restoration, and accessible names Diagramflowchart LR
A["HTML Templates<br/>pad.html, index.html,<br/>timeslider.html"] -->|"Add lang/dir<br/>from Accept-Language"| B["Server-rendered<br/>lang attribute"]
C["Popup Elements<br/>settings, import_export,<br/>embed, users, etc"] -->|"Add role=dialog<br/>aria-modal=true<br/>aria-labelledby/label"| D["Dialog Semantics"]
E["toggleDropDown<br/>in pad_editbar.ts"] -->|"Remember trigger,<br/>focus first focusable,<br/>restore on close"| F["Focus Management"]
G["Escape Key Handler<br/>in _bodyKeyEvent"] -->|"Close popup &<br/>restore focus"| F
H["Icon-only Controls<br/>chaticon, titlecross,<br/>titlesticky, show-more"] -->|"Convert to buttons<br/>Add aria-labels"| I["Semantic Buttons"]
J["Export Links &<br/>Userlist Region"] -->|"Add aria-labels<br/>Fix aria-role typo"| K["Accessible Names"]
L["Playwright Tests"] -->|"Verify dialog ARIA,<br/>Escape behavior,<br/>focus restoration"| M["Test Coverage"]
File Changes1. src/templates/pad.html
|
Code Review by Qodo
1. Users Escape close broken
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+330
to
+336
| // Escape from inside any open popup: close the popup and let | ||
| // toggleDropDown('none') restore focus to the trigger. | ||
| if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) { | ||
| this.toggleDropDown('none'); | ||
| evt.preventDefault(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
1. Users escape close broken 🐞 Bug ≡ Correctness
Pressing Escape inside the Users popup calls toggleDropDown('none'), but toggleDropDown('none')
explicitly skips the users module, so the Users dialog won’t close. Because the handler
preventsDefault() and returns, the existing Escape behavior won’t run either, leaving keyboard users
stuck in the Users popup.
Agent Prompt
### Issue description
Escape-to-close calls `toggleDropDown('none')`, but `toggleDropDown('none')` intentionally skips the `users` dropdown, so Escape does not close the Users popup.
### Issue Context
The new Escape handler is meant to close any open popup and restore focus. The Users popup is treated specially in `toggleDropDown('none')` and is skipped.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[329-336]
- src/static/js/pad_editbar.ts[232-246]
### Implementation notes
One of:
1) Remove the `users` skip in the `moduleName === 'none'` branch, but preserve the `stickyUsers` behavior (do not close if sticky), or
2) In the Escape handler, detect if `#users` is open and not sticky, and explicitly close it (remove `popup-show`, remove `.selected` from the toolbar button) instead of calling `toggleDropDown('none')`.
Ensure focus restoration still works after close.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+264
to
+273
| if (openedModule) { | ||
| // Move focus into the now-visible popup so keyboard users land inside the dialog. | ||
| const target = openedModule; | ||
| requestAnimationFrame(() => { | ||
| const focusable = target.find( | ||
| 'button:visible, a[href]:visible, input:not([disabled]):visible, ' + | ||
| 'select:not([disabled]):visible, textarea:not([disabled]):visible, ' + | ||
| '[tabindex]:not([tabindex="-1"]):visible').first(); | ||
| if (focusable.length) focusable[0].focus(); | ||
| }); |
There was a problem hiding this comment.
2. Embed focus overridden 🐞 Bug ≡ Correctness
toggleDropDown() now focuses the first focusable element in the opened popup on the next animation frame, which overrides existing command-specific focus logic. In the Embed popup this steals focus from #linkinput (which the embed command intentionally focuses/selects) and moves it to the readonly checkbox instead.
Agent Prompt
### Issue description
`toggleDropDown()` auto-focuses the first focusable control in the opened popup via `requestAnimationFrame()`. This can override existing command handlers that deliberately focus a particular element (notably the Embed command focuses/selects `#linkinput`).
### Issue Context
In `pad.html`, the Embed dialog has `#readonlyinput` before `#linkinput`, so the auto-focus targets `#readonlyinput`, stealing focus from `#linkinput`.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[264-273]
- src/static/js/pad_editbar.ts[429-433]
- src/templates/pad.html[328-338]
### Implementation notes
Modify the rAF auto-focus behavior to be conditional, for example:
- In the rAF callback, only focus the first element if focus is still on the trigger/body (or not already inside the opened popup), OR
- Allow callers to opt out (e.g., `toggleDropDown(moduleName, {autoFocus: false})`) and use that for `embed`, OR
- Prefer a more specific focus target for certain dialogs (Embed -> `#linkinput`).
Preserve the new general behavior for dialogs that don’t explicitly manage focus.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+72
to
+89
| 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'); | ||
| await expect(chatIcon).toHaveAttribute('aria-label', 'Open chat'); | ||
| }); | ||
|
|
||
| test('chat header close/pin controls are buttons with labels', async ({page}) => { | ||
| await page.locator('#chaticon').click(); | ||
| for (const [id, label, tag] of [ | ||
| ['#titlecross', 'Close chat', 'button'], | ||
| ['#titlesticky', 'Pin chat to screen', 'button'], | ||
| ] as const) { | ||
| const el = page.locator(id); | ||
| const tagName = await el.evaluate((node) => node.tagName.toLowerCase()); | ||
| expect(tagName).toBe(tag); | ||
| await expect(el).toHaveAttribute('aria-label', label); | ||
| } |
There was a problem hiding this comment.
3. L10n overwrites aria-labels 🐞 Bug ☼ Reliability
html10n unconditionally sets aria-label to the translated string for any node it translates, so elements with data-l10n-id ending in .title will have their aria-label overwritten. This makes the new Playwright assertions for chat button/pin label incorrect and potentially makes the in-HTML aria-label values ineffective at runtime.
Agent Prompt
### Issue description
The new tests assert hard-coded English `aria-label` values for elements that html10n will later overwrite because they have `data-l10n-id` values ending in `.title`.
### Issue Context
In this repo’s html10n implementation, translating an element sets both the translated property (e.g., `title`) and also sets `aria-label` to the same translated string.
### Fix Focus Areas
- src/static/js/vendors/html10n.ts[661-667]
- src/templates/pad.html[383-395]
- src/tests/frontend-new/specs/a11y_dialogs.spec.ts[72-90]
### Implementation notes
Pick one consistent approach:
1) **Rely on html10n for aria-label**: remove the hard-coded `aria-label` attributes from elements that have `data-l10n-id="*.title"`, and update the Playwright tests to assert that `aria-label` is non-empty (or equals the element’s localized `title`) rather than matching a fixed string.
OR
2) **Preserve explicit aria-label**: change html10n so it does **not** overwrite an existing `aria-label` attribute (only set it if missing). Then the tests can keep asserting the explicit labels.
Either way, ensure tests are stable across locales (avoid asserting fixed English strings if localization can run).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
- #chaticon: <div onclick> → <button type=button> with aria-label - #titlecross / #titlesticky: <a onClick> → <button type=button> with aria-label (Close chat / Pin chat to screen) - Decorative chat-bubble glyph gets aria-hidden=true so it isn't read alongside the button label - #chatcounter labelled "Unread messages" - Inline onclick attributes moved to chat.init() handlers - CSS reset on the new buttons (transparent bg, no border, inherit font/color) so they match the prior visual design - :focus-visible outlines for keyboard users Existing test selectors (#chaticon, #titlecross, #titlesticky) are unchanged and continue to work — they never relied on element type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Export links (#exportetherpada, #exporthtmla, #exportplaina, #exportworda, #exportpdfa, #exportopena): added aria-label so the link is announced as e.g. "Export as PDF". The inner icon span gets aria-hidden=true so screen readers don't read both the icon text and the link label. - Show-more toolbar toggle (.show-more-icon-btn): converted from <span> to <button type=button> with aria-label and aria-expanded. The click handler now toggles aria-expanded alongside the full-icons class so assistive tech reflects the open/closed state. - Theme switcher knob: aria-label changed from "theme-switcher-knob" (a class-style identifier, not human text) to "Toggle theme". Aria-label values are English-only for now. Etherpad's html10n implementation only localizes a fixed attribute list (title, alt, placeholder, value, innerHTML, textContent); aria-label is not included, so a clean l10n path requires a follow-up to either extend html10n or set aria-label client-side after locale loads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New Playwright spec verifies the a11y guarantees added by this branch:
- <html> has a non-empty lang attribute
- settings/import_export/embed/users popups expose role=dialog,
aria-modal=true, and either aria-labelledby (when an h1 exists)
or aria-label (when none does)
- Escape from inside the settings popup closes it AND restores
focus to the trigger button
- Export links each carry a descriptive aria-label
- #chaticon is a real <button> with aria-label
- #titlecross / #titlesticky are real <button>s with aria-label
- #otherusers uses role=region + aria-live=polite + aria-label
(and the previous aria-role typo is gone)
- .show-more-icon-btn is a <button> with aria-label and
aria-expanded
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnMcLear
force-pushed
the
fix/a11y-dialogs-labels-lang
branch
from
818dceb to
68a0761
Compare
Member
Author
|
/review |
|
Persistent review updated to latest commit 68a0761 |
Comment on lines
+14
to
+16
| test('settings popup has dialog semantics, Escape closes it, focus returns to trigger', | ||
| async ({page}) => { | ||
| const settingsButton = page.locator('button[data-l10n-id="pad.toolbar.settings.title"]'); |
There was a problem hiding this comment.
1. 4-space indent in a11y spec 📘 Rule violation ⚙ Maintainability
The new Playwright spec introduces 4-space indentation in wrapped function arguments, violating the repository's 2-space indentation requirement. This can reduce formatting consistency and trigger lint/format tooling conflicts.
Agent Prompt
## Issue description
The new Playwright spec contains indentation using 4 spaces (not a 2-space multiple per indentation level as required by the repo style rule).
## Issue Context
The compliance checklist requires 2-space indentation and forbids inconsistent indentation widths in new/modified code.
## Fix Focus Areas
- src/tests/frontend-new/specs/a11y_dialogs.spec.ts[14-16]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+213
to
+218
| // Remember the trigger so we can restore focus when the dialog closes. | ||
| const wasAnyOpen = $('.popup.popup-show').length > 0; | ||
| if (!wasAnyOpen && moduleName !== 'none') { | ||
| const active = document.activeElement; | ||
| if (active && active !== document.body) this._lastTrigger = active; | ||
| } |
There was a problem hiding this comment.
2. Focus restore captures wrong trigger 🐞 Bug ≡ Correctness
toggleDropDown() tries to remember the trigger via document.activeElement, but toolbar clicks blur the focused element before executing the command, so _lastTrigger is frequently unset or not the actual opener and focus is not reliably restored on close/Escape.
Agent Prompt
### Issue description
`toggleDropDown()` stores the trigger based on `document.activeElement`, but toolbar click handling blurs the currently focused element before calling the command, so `document.activeElement` is often `body` (or otherwise not the intended trigger). This makes focus restoration unreliable.
### Issue Context
The toolbar click handler intentionally blurs `:focus` before executing the dropdown command. `toggleDropDown()` should therefore not depend on `document.activeElement` at that moment.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[66-72]
- src/static/js/pad_editbar.ts[202-218]
### Suggested fix
- Capture the actual trigger element from the originating UI event (e.g., the clicked button) and store it before blurring, then pass it through to `toggleDropDown()` (or store it on the padeditbar instance before invoking the command).
- Alternatively, derive the trigger deterministically from `moduleName` (e.g., locate `li[data-key=<moduleName>] button`) instead of using `document.activeElement`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This was referenced Apr 23, 2026
1. Users Escape close broken - toggleDropDown('none') intentionally
skips the users module so switching between other popups doesn't
hide the user list. That meant Escape couldn't dismiss the Users
popup either. The Escape branch now checks for #users as the
focused popup and closes it explicitly (respecting stickyUsers)
before falling through to the normal close-all path.
2. Embed focus overridden - the rAF auto-focus in toggleDropDown
grabbed the first focusable descendant, which stole focus from
command handlers that target a specific control (notably the Embed
command's #linkinput). rAF now bails out if focus is already
inside the newly-opened popup.
3. Button click blurs :focus before toggleDropDown captures trigger -
discovered while investigating the Firefox Playwright failure for
"settings popup Escape restores focus". Button.bind() calls
$(':focus').trigger('blur') before invoking the callback, so by
the time toggleDropDown() captured document.activeElement as the
restore target it was already <body>. The click handler now
stashes padeditbar._lastTrigger to the clicked <button> before
blur runs; toggleDropDown only falls back to activeElement when
the pre-stash didn't happen (keyboard shortcut path).
4. html10n overwrites aria-label - html10n unconditionally set
aria-label to the translated string, clobbering explicit aria-label
on elements that also carry data-l10n-id. setAttribute now only
fires when the element has no aria-label; explicit author labels
win, unlabelled translated elements still get a name.
5. Button visual reset - the show-more-icon-btn and #chaticon
conversions inherited UA default button border/background/padding,
shifting icon glyphs visibly off-centre. Added appearance /
background / border / padding resets.
6. Export links test assumes soffice is installed - #exportworda,
#exportpdfa, #exportopena are removed client-side by pad_impexp.ts
when clientVars.exportAvailable === 'no'. The test now skips links
absent at runtime.
Verified locally: all 10 a11y_dialogs specs pass on both Chromium and
Firefox; backend suite remains 799/799 passing; ts-check clean.
Member
Author
|
/review |
|
Persistent review updated to latest commit c89444c |
Comment on lines
+268
to
+287
| if (openedModule) { | ||
| // Move focus into the now-visible popup so keyboard users land inside the dialog. | ||
| // Skip if a command handler already placed focus inside this popup — the Embed | ||
| // command focuses #linkinput deliberately, which is different from the first | ||
| // tabbable element (a readonly checkbox) and should win. | ||
| const target = openedModule; | ||
| requestAnimationFrame(() => { | ||
| if (target[0].contains(document.activeElement)) return; | ||
| const focusable = target.find( | ||
| 'button:visible, a[href]:visible, input:not([disabled]):visible, ' + | ||
| 'select:not([disabled]):visible, textarea:not([disabled]):visible, ' + | ||
| '[tabindex]:not([tabindex="-1"]):visible').first(); | ||
| if (focusable.length) focusable[0].focus(); | ||
| }); | ||
| } else if ($('.popup.popup-show').length === 0 && this._lastTrigger) { | ||
| // All popups closed — restore focus to the element that opened the first one. | ||
| const trigger = this._lastTrigger; | ||
| this._lastTrigger = null; | ||
| if (document.body.contains(trigger)) trigger.focus(); | ||
| } |
There was a problem hiding this comment.
1. Stale focus restoration 🐞 Bug ≡ Correctness
padeditbar.toggleDropDown('none') now restores focus to this._lastTrigger whenever no popups are
open, even if no popup was previously open. Because _lastTrigger is set on every toolbar button
click, background calls to toggleDropDown('none') (e.g., connection-state handling) can unexpectedly
move focus from the editor to a stale toolbar button.
Agent Prompt
### Issue description
`toggleDropDown('none')` restores focus to `this._lastTrigger` even when no popup was previously open, which can steal focus during programmatic calls (e.g., connectivity state changes).
### Issue Context
- `_lastTrigger` is set on every toolbar button click.
- Multiple code paths call `toggleDropDown('none')` as a cleanup step regardless of whether a popup is open.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[66-81]
- src/static/js/pad_editbar.ts[210-287]
### Suggested fix
- Gate the focus-restore block so it only runs when a popup was actually open at the start of the function (e.g., `wasAnyOpen === true`) and is now closed.
- Consider clearing `_lastTrigger` when handling non-dropdown toolbar commands, or only setting `_lastTrigger` for dropdown-opening commands, to avoid stale values lingering.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…layout Round 2 of ether#7584 review follow-ups. 1. Users popup Escape still didn't close the dialog (user-confirmed). Root cause: _bodyKeyEvent is bound to the OUTER document's body. When #users opens, the command handler tries to focus #myusernameedit but that input is `disabled`, so focus stays in the ace editor iframe. Keydown from inside the iframe does not bubble to the outer document, so Esc never reaches _bodyKeyEvent. Fix: in the open-popup rAF, if no command handler placed focus inside the dialog, focus the popup div itself (with tabindex=-1). That keeps subsequent keydown events on the outer document so Esc can dismiss the popup. Also broadened the Esc branch to fire whenever any popup is `.popup-show`, regardless of where :focus lives — some popups legitimately have no focusable content at open. Added a regression test that opens #users and asserts Esc closes it. Passes on both Chromium and Firefox. 2. Chat icon (#chaticon) visual still wrong after the first CSS fix. - My previous `border: 0` reset was overriding the intended `border: 1px solid #ccc; border-bottom: none` from the earlier rule. Removed `border: 0`; the earlier explicit border suffices to suppress UA defaults. - The `<span class="buttonicon">` inside `#chaticon` was picking up the global `.buttonicon { display: flex; }` rule meant for toolbar button instances, which broke the inline layout of the label + glyph + counter row. Added a scoped `#chaticon .buttonicon { display: inline; }` override. All 11 a11y_dialogs specs pass on Chromium and Firefox. Backend suite and ts-check remain clean.
Member
Author
|
/review |
|
Persistent review updated to latest commit f8548b2 |
Comment on lines
+347
to
+363
| // Escape while any popup is open: close it. We don't restrict to | ||
| // `:focus inside popup` because some popups (e.g. #users) have no | ||
| // focusable content on open — focus stays in the ace editor iframe — | ||
| // but Esc should still dismiss them for keyboard users. | ||
| if (evt.keyCode === 27 && $('.popup.popup-show').length > 0) { | ||
| // `toggleDropDown('none')` intentionally skips the users popup so | ||
| // switching between other popups doesn't hide the user list. For | ||
| // Escape we want the users popup to close too (unless pinned). | ||
| const openPopup = $('.popup.popup-show').first(); | ||
| if (openPopup.attr('id') === 'users' && !openPopup.hasClass('stickyUsers')) { | ||
| openPopup.removeClass('popup-show'); | ||
| $('li[data-key=users] > a').removeClass('selected'); | ||
| } | ||
| this.toggleDropDown('none'); | ||
| evt.preventDefault(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
1. Escape won't close colorpicker 🐞 Bug ≡ Correctness
padeditbar._bodyKeyEvent() intercepts Escape whenever any .popup has .popup-show, but it only
closes dropdown popups via toggleDropDown('none') (and special-cases #users). Popups that are
opened outside toggleDropDown (such as #mycolorpicker) keep .popup-show, so Escape becomes a
no-op while still calling preventDefault() and returning early.
Agent Prompt
### Issue description
Escape handling in `pad_editbar._bodyKeyEvent()` triggers for any `.popup.popup-show`, but the close logic only affects dropdown popups (and partially `#users`). Popups opened by other code paths (e.g. `#mycolorpicker` from `pad_userlist.ts`) remain open, so Escape is swallowed (preventDefault + early return) without dismissing the visible popup.
### Issue Context
`#mycolorpicker` is opened by directly adding `.popup-show` and is not part of `padeditbar.dropdowns`, so `toggleDropDown('none')` cannot close it.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[346-363]
- src/static/js/pad_editbar.ts[235-266]
- src/static/js/pad_userlist.ts[587-616]
### Suggested fix
Update the Escape branch to remove `.popup-show` from *all* open popups that should be dismissible via Escape (at least including `#mycolorpicker`, and excluding pinned/sticky cases such as `#users.stickyUsers`). For example:
- Identify all `$('.popup.popup-show')` elements.
- Filter out popups that should remain open (e.g. `#users.stickyUsers`).
- Remove `.popup-show` from the remaining open popups.
- Clear any corresponding toolbar `selected` states as needed.
- Then call `toggleDropDown('none')` (or equivalent) purely for shared cleanup + focus restore, ensuring it does not re-swallow Escape when no popups remain.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Round 3 follow-up. The previous Button.bind() change stashed every clicked toolbar button as padeditbar._lastTrigger before blurring :focus. That was necessary for popup-opening buttons (settings, import_export, etc.) so Escape could return focus to them — but it also fired for non-popup toolbar buttons (list toggles, bold/italic, indent/outdent, clearauthorship). For those, the stash held a stale reference that interfered with subsequent editor interactions and regressed Playwright tests: ordered_list, unordered_list, undo_clear_authorship. Fix: only stash when the clicked command is a registered dropdown (settings, import_export, embed, showusers, savedrevision, connectivity). Other commands return focus to the ace editor as before and leave _lastTrigger alone. Verified locally on Chromium: - ordered_list.spec.ts: 6/6 pass (was 4/6) - unordered_list.spec.ts: 6/6 pass (was 4/6) - undo_clear_authorship.spec.ts: 2/2 pass (was 0/2) - a11y_dialogs.spec.ts: 11/11 pass (unchanged)
Member
Author
|
/review |
|
Persistent review updated to latest commit c2062fe |
Comment on lines
+665
to
+673
| // Populate aria-label from the translation so screen readers always get | ||
| // a localized accessible name, but do not overwrite an explicit | ||
| // aria-label that an author has already set. This lets templates use | ||
| // static English aria-labels for icon-only controls (export links, | ||
| // chat icon, close/pin buttons) without losing them at localization | ||
| // time. See PR #7584 review feedback. | ||
| if (!node.hasAttribute('aria-label')) { | ||
| node.setAttribute('aria-label', str.str!); | ||
| } |
There was a problem hiding this comment.
1. Stale aria-label after relocalize 🐞 Bug ≡ Correctness
html10n.translateNode() now refuses to overwrite an existing aria-label, but html10n itself sets aria-label during the first localization pass, so later html10n.localize() calls (triggered by language changes) will not update accessible names. This can leave screen readers announcing labels in the old language even though the visible text has been translated, because aria-label overrides text content for the accessible name.
Agent Prompt
## Issue description
`html10n.translateNode()` sets `aria-label` from the translated string. After the first translation, the attribute exists, and with the new guard (`if (!node.hasAttribute('aria-label'))`) subsequent language changes will not update the aria-label. Because `aria-label` overrides an element’s text for accessible-name calculation, screen readers can announce stale (previous-language) labels.
## Issue Context
Etherpad re-localizes at runtime (language changes call `html10n.localize()` via `pad.applyLanguage()`), so `translateNode()` is expected to be idempotent and update derived attributes.
## Fix Focus Areas
- src/static/js/vendors/html10n.ts[661-673]
- src/static/js/pad.ts[570-573]
## Suggested fix
Change the guard to only preserve **author-supplied** aria-labels, while still allowing updates to aria-labels previously generated by html10n. One simple pattern:
- When html10n sets aria-label, also set a marker attribute (e.g. `data-l10n-aria-label="true"`).
- On subsequent translations, overwrite aria-label if the marker is present.
Example:
```ts
const generatedAttr = 'data-l10n-aria-label';
if (!node.hasAttribute('aria-label') || node.getAttribute(generatedAttr) === 'true') {
node.setAttribute('aria-label', str.str!);
node.setAttribute(generatedAttr, 'true');
}
```
This preserves explicit template-provided aria-labels, but keeps generated aria-labels in sync with language switches.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Highest-impact accessibility fixes from a fresh audit of the pad surface. Four themes:
Dialog semantics — every
.popup(#settings,#import_export,#connectivity,#embed,#users,#mycolorpicker,#skin-variants) now exposesrole="dialog",aria-modal="true", and eitheraria-labelledby(when an<h1>is present) oraria-label. Fixed the invalidaria-role="document"on#otherusers(it'srole, notaria-role); nowrole="region"+aria-live="polite"so collaborator joins/leaves get announced.Focus management —
toggleDropDownnow remembers the trigger button, focuses the first focusable element inside the popup on open, and restores focus to the trigger on close. Escape while focus is inside an open popup closes it (previously did nothing — users had to click outside).Real buttons for icon-only controls —
<div id="chaticon" onclick="…">→<button type="button" id="chaticon" aria-label="Open chat"><a id="titlecross" onClick="…">→<button aria-label="Close chat"><a id="titlesticky" onClick="…">→<button aria-label="Pin chat to screen"><span class="show-more-icon-btn">→<button aria-label="Show more toolbar buttons" aria-expanded>(toggles aria-expanded on click)#exportetherpada…#exportopena) get a descriptivearia-label; the inner icon spans getaria-hidden="true"so SR doesn't double-read.aria-label="theme-switcher-knob"(a class-style identifier) →"Toggle theme"(human text).#chatcounterare now properly labelled (aria-hiddenon the icon,aria-label="Unread messages"on the counter).<html>lang/dir negotiated per request —pad.html,index.html,timeslider.htmlnow render withlanganddirmatching the client'sAccept-Language(negotiated againsti18n.availableLangs), falling back toen/ltr. Client-sidel10n.tsalready refines both attributes after html10n loads, so this just gives screen readers a correct language hint during the brief pre-localization window.Out of scope (deliberate)
:focus-visiblesweep.Test plan
// @ts-nocheckso no type regressions possible)#chaticon/#titlecrossselectors which are unchanged)tests/frontend-new/specs/a11y_dialogs.spec.tspasses — covers<html lang>, dialog ARIA, Escape-closes-and-restores-focus, export labels, chat button conversion, userlist region, show-more button🤖 Generated with Claude Code