Mirrors/etherpad-lite
1
0
Fork 0
mirror of https://github.com/ether/etherpad-lite.git synced 2026-07-17 16:47:05 +00:00
Code Issues Projects Releases Wiki Activity

fix(a11y): action Qodo review on PR #7758 (#7764)

Browse source 
Three bugs and one test-flake risk that Qodo flagged on c4ea12d:

(1) The keyboard hint element was created with hidden=true. Per the
ARIA spec, the HTML hidden attribute removes the element from the
accessibility tree, so the body's aria-describedby pointer resolved to
a node with no exposed name and screen readers ignored the hint.
Removed the hidden attribute; the hint lives in the inner document's
<head> where it isn't rendered anyway, so there's no visual cost.

(2) Hint text was set once with html10n.get(), which returns undefined
when translations haven't finished loading. Since Ace2Editor.init()
races with html10n.localize(), the hint could permanently read
"undefined" (or be empty). Seed with a hardcoded English fallback so
the hint is always usable, and refresh from html10n on the 'localized'
event so the fallback is replaced once translations land.

(3) Same html10n.bind('localized', refreshHint) re-runs on every
runtime language change, fixing the stale-language bug where
applyLanguage() wouldn't update the inner-iframe hint.

(4) The "skip link is first Tab target" test asserted that
document.activeElement === <body> as a precondition. That invariant is
fragile — banners, modals, plugins can grab focus on load (the
goToNewPad helper documents one such workaround). Replaced the
precondition with an explicit blur of whatever is focused, so the
assertion is purely about tab order.

Qodo's #4 (skip link not feature-flagged) is intentional: accessibility
fixes shouldn't be opt-in. A flagged-off skip link wouldn't help
anyone, and "preserve pre-existing behavior" doesn't apply to a
WCAG 2.4.1 compliance fix. Replying separately on the PR thread.

Refs #7255

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear 2026-05-15 20:03:16 +01:00 • committed by GitHub
parent 986f139a61
commit 79f525b0a7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 59 additions and 14 deletions
Download patch file Download diff file Expand all files Collapse all files

39
src/static/js/ace.ts
View file

@ -328,18 +328,39 @@ const Ace2Editor = function () {
});
debugLog('Ace2Editor.init() Ace2Inner.init() returned');
// Screen-reader-only keyboard hint, target of the body's
// aria-describedby. We park it in <head> instead of <body> because
// Ace2Inner manages body children via its line model — anything
// unrelated inserted into body gets wiped by line splices. The ARIA
// spec allows the description target to live anywhere in the same
// document, and screen readers resolve it by ID; rendering doesn't
// matter because aria-describedby fetches the element's text.
// Screen-reader-only keyboard hint, target of the inner body's
// aria-describedby. Three things matter for AT to actually announce it:
//
// 1. Position: appended to the inner document's <head>. Anything in
// <body> gets wiped by Ace2Inner's line-model rebuild during the
// queued setBaseText/importText calls below. The ARIA spec lets
// aria-describedby resolve to any ID in the same document, so
// head placement is valid — screen readers look up by ID and
// read text content, rendering position doesn't matter.
// 2. Exposure: NOT `hidden`. The HTML `hidden` attribute removes the
// node from the accessibility tree, so aria-describedby would
// resolve to a node with no exposed name. We leave the element
// visible in markup but unrendered (head children don't paint).
// 3. Localization: html10n.get() returns undefined when translations
// are still loading (and Ace2Editor.init() races with that load),
// so seed with a hardcoded English fallback and re-pull the
// translated string on every html10n `localized` event — that
// keeps the text right both on first paint and on runtime
// language changes.
const HINT_FALLBACK_EN =
'Press Escape to exit the editor. Press Alt+F9 to access the toolbar.';
const hint = innerDocument.createElement('div');
hint.id = 'editor-keyboard-hint';
hint.hidden = true;
hint.textContent = html10n.get('pad.editor.keyboardHint');
const refreshHint = () => {
const localized = html10n.get('pad.editor.keyboardHint');
hint.textContent =
(typeof localized === 'string' && localized) ? localized : HINT_FALLBACK_EN;
};
refreshHint();
innerDocument.head.appendChild(hint);
if (html10n && typeof html10n.bind === 'function') {
html10n.bind('localized', refreshHint);
}
loaded = true;
doActionsPendingInit();

34
src/tests/frontend-new/specs/a11y_dialogs.spec.ts
View file

@ -163,16 +163,40 @@ test('skip-to-content link bypasses toolbar (WCAG 2.4.1, #7255)', async ({page})
});
test('skip link is the first Tab target from a fresh page (WCAG 2.4.1, #7255)', async ({page}) => {
// Pre-condition: pad.ts no longer auto-focuses the editor on load
// (that previously trapped Tab inside the editor iframe and left the
// skip link unreachable from the URL bar). Initial focus is on <body>.
const initialTag = await page.evaluate(() => document.activeElement?.tagName);
expect(initialTag).toBe('BODY');
// Don't assert what happens to be focused after page load — plugins,
// banners, or the privacy-token modal can grab focus before the test
// runs, and the actual invariant we care about is tab order, not the
// starting point. Defocus first, then press Tab from a known state.
await page.evaluate(() => {
const a = document.activeElement as HTMLElement | null;
if (a && a !== document.body && typeof a.blur === 'function') a.blur();
});
await page.keyboard.press('Tab');
const afterTabId = await page.evaluate(() => document.activeElement?.id || '');
expect(afterTabId).toBe('skip-to-content');
});
test('editor-keyboard-hint exists in the editor iframe with localized text (#7255)', async ({page}) => {
// Regression: PR #7451 added #editor-keyboard-hint as the target of the
// editor body's aria-describedby. The hint was being wiped by
// Ace2Inner.init()'s body management before it could be announced;
// PR #7758 reworked the insertion to run after doActionsPendingInit().
// Assert here that the hint actually exists post-init and carries the
// localized string — without this test, future ace internals changes
// could silently reintroduce the wipe.
const innerFrame = page.frameLocator('iframe[name="ace_outer"]')
.frameLocator('iframe[name="ace_inner"]');
const hint = innerFrame.locator('#editor-keyboard-hint');
await expect(hint).toHaveCount(1);
// html10n may take a moment to populate; toHaveText polls. The string is
// mandatory (mandatory English fallback in ace.ts), so neither '' nor
// 'undefined' is acceptable.
const text = await hint.textContent();
expect(text && text.length > 0).toBe(true);
expect(text).not.toBe('undefined');
expect(text).toContain('Escape');
});
test('line-number sidediv is hidden from screen readers (#7255)', async ({page}) => {
// sidediv lives in the outer ace iframe (ace_outer) — query the frame.
const outerFrame = page.frameLocator('iframe[name="ace_outer"]');