From 924059257dd577ca602583d7a56b11bafcc3ba94 Mon Sep 17 00:00:00 2001
From: John McLear <john@mclear.co.uk>
Date: Sat, 16 May 2026 13:08:33 +0100
Subject: [PATCH] fix(a11y): name role=toolbar regions, hide linemetricsdiv
 from AT (#7255) (#7777)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(a11y): name role=toolbar regions, hide linemetricsdiv from AT (#7255)

Two regressions called out in the 2026-05-16 follow-up on #7255, after the
firefox accessibility inspector flagged them:

(1) The "Ether X" announcement between the editor and chat button was the
outer ace iframe (titled "Ether") plus a single 'x' text leaf the renderer
appends to outerdocbody for line-height measurement (linemetricsdiv in
ace.ts). Add aria-hidden=true on creation so AT skips the measurement node
entirely. Same approach we used for sidediv in PR #7758.

(2) The two formatting/actions <ul role="toolbar"> regions and the
history-mode role=toolbar div had no accessible name. Lighthouse + the
firefox a11y panel both flagged this. Putting data-l10n-id directly on
the <ul> would either destroy its <li> children (textContent branch) or
not populate aria-label (the html10n auto-aria-label code path skips
non-form-control elements), and a hidden <span> child inside the <ul>
would be invalid HTML. Solution: three visually-hidden <span> labels
sitting just before #editbar, each carrying data-l10n-id for translation,
referenced from the toolbars via aria-labelledby. Apply the same treatment
to .show-more-icon-btn, whose aria-label was previously hardcoded English
(no data-l10n-id, so untranslated).

Adds Playwright assertions for linemetricsdiv aria-hidden and the resolved
accessible-name text of each toolbar. Updates the existing show-more test
to expect aria-labelledby (it previously asserted hardcoded English
aria-label).

Refs #7255

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(a11y): reuse translated history-controls label key (Qodo PR review)

Qodo flagged: adding `aria-labelledby="editbar-history-label"` on
#history-controls overrode the `aria-label` that pad_mode.ts sets from
`pad.historyMode.controlsLabel`. That key is already translated in
multiple locales (en/de/nl/...); the new `pad.editor.toolbar.history`
key was English-only, so non-English users would regress from a
localized history-toolbar name to the English fallback.

Point the hidden label span at the existing translated key instead of
minting a new one, drop the new key from en.json, and update the
Playwright expectation to match the translated string ("Pad history
controls"). The aria-label that pad_mode.ts still writes to
#history-controls is now redundant (aria-labelledby wins) but harmless,
and leaving it preserves the runtime relocalization path.

Refs ether/etherpad#7777

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(a11y): mark toolbar li/a wrappers presentational (Lighthouse, #7255)

Lighthouse's axe-core `listitem` rule fires on every toolbar button
because role="toolbar" on the <ul> overrides its implicit role="list",
leaving the <li> children "orphaned" by axe's heuristic. Murphy's
2026-05-16 follow-up on #7255 attached the Chrome DevTools Lighthouse
panel screenshot of this exact failure.

Marking the <li>+<a> wrappers role="presentation" tells axe-core they
are layout scaffolding for the toolbar role, while the inner <button>
keeps its semantics for AT. Same treatment for SelectButton's <li>
wrapper. The Separator's <li> also gets aria-hidden=true so AT does
not announce an empty list item between toolbar buttons.

CSS and JS selectors that still target `.toolbar ul li` continue to
work — role="presentation" only affects the accessibility tree, not
the DOM tree. No visual or behavioral change for sighted users.

Adds a Playwright spec that walks every rendered toolbar <li>/<a>
and asserts role="presentation" so future toolbar.ts tweaks can't
silently re-introduce the Lighthouse failure.

Refs ether/etherpad#7255

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(a11y): label #online_count for AT (#7255 - "number next to the user icon")

Murphy's 2026-05-16 follow-up cut off mid-bullet ("It's not clear what the
number next to the …"). Best guess: the user-count badge in the showusers
toolbar button. Currently it exposes a bare digit to AT — "5" with no
context — because the visible badge text is also the entire accessible
content.

Append a localized aria-label generated from a new pad.userlist.onlineCount
key (plural macro for one / other) whenever the count updates, so AT
announces "5 connected users" instead of the bare digit. Add role=status
and aria-live=polite so the count change is announced inline without
forcing the user to refocus the button.

Visible badge digit unchanged. html10n.get is null-safe (falls back to
an English template so AT never gets back "undefined" before the locale
bundle has loaded).

Adds a Playwright spec verifying role/aria-live and that the aria-label
contains "connected user".

Refs ether/etherpad#7255

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(a11y): tighten #online_count + plugin-emitted toolbar <li>s (#7255)

Two small follow-ups on top of the toolbar/online-count work:

(1) #online_count had no accessible label on solo-author pads.
updateNumberOfOnlineUsers — which writes the localized aria-label —
only fires on userJoin/userLeave/status change, never on initial
single-author load. Call it at the end of init() so the badge ships
with its label on first paint. Also bind html10n's 'localized' event
so non-English users get the translated label after the locale bundle
arrives (matches the keyboard-hint / history-toolbar pattern).
Harden the Playwright spec to use polling toHaveAttribute instead of
one-shot getAttribute.

(2) Sweep role="presentation" onto plugin-emitted toolbar <li>s in
pad_editbar.ts init(). Core's toolbar.ts emits its <li>s with the role
already, but plugins (ep_headings2, ep_align, ep_font_*, ep_print, ...)
ship their own editbarButtons.ejs templates that emit <li> directly,
so Lighthouse's listitem rule kept firing on the "with plugins" test
runs. Runtime sweep covers anything in the editbar at init time, no
plugin coordination needed.

Refs ether/etherpad#7255

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 src/locales/en.json                           |  4 +
 src/node/utils/toolbar.ts                     | 24 ++++-
 src/static/js/ace.ts                          |  5 ++
 src/static/js/pad_editbar.ts                  | 18 ++++
 src/static/js/pad_userlist.ts                 | 30 ++++++-
 src/templates/pad.html                        | 28 +++++-
 .../frontend-new/specs/a11y_dialogs.spec.ts   | 87 ++++++++++++++++++-
 7 files changed, 186 insertions(+), 10 deletions(-)

diff --git a/src/locales/en.json b/src/locales/en.json
index 5ae5bfa81..55f4159af 100644
--- a/src/locales/en.json
+++ b/src/locales/en.json
@@ -362,6 +362,9 @@
   "pad.historyMode.users.authorsHeader": "Authors at this revision",
   "pad.editor.skipToContent": "Skip to editor",
   "pad.editor.keyboardHint": "Press Escape to exit the editor. Press Alt+F9 to access the toolbar.",
+  "pad.editor.toolbar.formatting": "Formatting toolbar",
+  "pad.editor.toolbar.actions": "Pad actions toolbar",
+  "pad.editor.toolbar.showMore": "Show more toolbar buttons",
   "timeslider.toolbar.authors": "Authors:",
   "timeslider.toolbar.authorsList": "No Authors",
   "timeslider.toolbar.exportlink.title": "Export",
@@ -398,6 +401,7 @@
   "pad.savedrevs.timeslider": "You can see saved revisions by visiting the timeslider",
   "pad.userlist.entername": "Enter your name",
   "pad.userlist.unnamed": "unnamed",
+  "pad.userlist.onlineCount": "{[ plural(count) one: {{count}} connected user, other: {{count}} connected users ]}",
   "pad.editbar.clearcolors": "Clear authorship colors on entire document? This cannot be undone",
 
   "pad.impexp.importbutton": "Import Now",
diff --git a/src/node/utils/toolbar.ts b/src/node/utils/toolbar.ts
index f8e70fb30..81600797d 100644
--- a/src/node/utils/toolbar.ts
+++ b/src/node/utils/toolbar.ts
@@ -113,12 +113,23 @@ class Button {
     }
 
     render() {
+        // role="presentation" on the <li> + <a> wrappers tells axe-core /
+        // Lighthouse they are layout scaffolding for the role="toolbar"
+        // parent, not list items. Without it the listitem rule fires
+        // because role="toolbar" overrides the implicit role="list" on
+        // the <ul>, leaving every <li> "orphaned" by axe's heuristic.
+        // The inner <button> keeps its semantics. See ether/etherpad#7255.
         const liAttributes = {
             'data-type': 'button',
             'data-key': this.attributes.command,
+            'role': 'presentation',
         };
         return tag('li', liAttributes,
-            tag('a', {'class': this.grouping, 'data-l10n-id': this.attributes.localizationId},
+            tag('a', {
+                    'class': this.grouping,
+                    'role': 'presentation',
+                    'data-l10n-id': this.attributes.localizationId,
+                },
                 tag('button', {
                     'class': ` ${this.attributes.class}`,
                     'data-l10n-id': this.attributes.localizationId,
@@ -167,6 +178,8 @@ class SelectButton extends Button {
             'id': this.attributes.id,
             'data-key': this.attributes.command,
             'data-type': 'select',
+            // See Button.render() above for the role="presentation" rationale.
+            'role': 'presentation',
         };
         return tag('li', attributes, this.select({id: this.attributes.selectId}));
     }
@@ -184,7 +197,14 @@ class Separator {
     }
 
     public render() {
-        return tag('li', {class: 'separator'});
+        // See Button.render() above for the role="presentation" rationale.
+        // The separator is purely visual; aria-hidden makes that explicit so
+        // AT does not announce an empty list item between toolbar buttons.
+        return tag('li', {
+            'class': 'separator',
+            'role': 'presentation',
+            'aria-hidden': 'true',
+        });
 
     }
 }
diff --git a/src/static/js/ace.ts b/src/static/js/ace.ts
index ec8448c1f..c140e8a07 100644
--- a/src/static/js/ace.ts
+++ b/src/static/js/ace.ts
@@ -243,6 +243,11 @@ const Ace2Editor = function () {
     sideDiv.appendChild(sideDivInner);
     const lineMetricsDiv = outerDocument.createElement('div');
     lineMetricsDiv.id = 'linemetricsdiv';
+    // Measurement-only node: holds a single "x" so the renderer can read
+    // its computed line height. Without aria-hidden, AT exposes the stray
+    // glyph as a "text leaf" sandwiched between the editor iframe and the
+    // chat button — see ether/etherpad#7255 (comment "Ether X" announcement).
+    lineMetricsDiv.setAttribute('aria-hidden', 'true');
     lineMetricsDiv.appendChild(outerDocument.createTextNode('x'));
     outerDocument.body.appendChild(lineMetricsDiv);
 
diff --git a/src/static/js/pad_editbar.ts b/src/static/js/pad_editbar.ts
index f9f581d28..30fd55fa3 100644
--- a/src/static/js/pad_editbar.ts
+++ b/src/static/js/pad_editbar.ts
@@ -155,6 +155,24 @@ exports.padeditbar = new class {
       });
     });
 
+    // Lighthouse / axe-core's `listitem` rule fires on every <li> child of an
+    // element whose role isn't `list` — and role="toolbar" on our <ul>s
+    // overrides the implicit list role. Core's toolbar.ts emits its <li>s
+    // with role="presentation" already; plugins ship their own editbarButtons
+    // EJS templates (ep_headings2, ep_align, ep_font_*, ep_print, ...) that
+    // emit <li> directly without the role, which kept the Lighthouse error
+    // alive. Sweep here once the toolbar DOM is fully built — runs before
+    // postToolbarInit hooks so anything plugins add even later still has the
+    // role attribute added to it the moment it lands (covered by the same
+    // sweep on the next init() call). role="presentation" only affects the
+    // accessibility tree, so CSS / JS selectors keep working. #7255.
+    $('#editbar .menu_left > li, #editbar .menu_right > li, #history-controls > li')
+        .filter((_i, el) => !el.hasAttribute('role'))
+        .attr('role', 'presentation');
+    $('#editbar .menu_left > li > a, #editbar .menu_right > li > a, #history-controls > li > a')
+        .filter((_i, el) => !el.hasAttribute('role'))
+        .attr('role', 'presentation');
+
     $('body:not(#editorcontainerbox)').on('keydown', (evt) => {
       this._bodyKeyEvent(evt);
     });
diff --git a/src/static/js/pad_userlist.ts b/src/static/js/pad_userlist.ts
index 43adc4a38..d3ee82527 100644
--- a/src/static/js/pad_userlist.ts
+++ b/src/static/js/pad_userlist.ts
@@ -354,7 +354,25 @@ const paduserlist = (() => {
       self.setMyUserInfo(myInitialUserInfo);
 
       if ($('#online_count').length === 0) {
-        $('#editbar [data-key=showusers] > a').append('<span id="online_count">1</span>');
+        // role="status" + aria-live="polite" announces the count when it
+        // changes; the localized aria-label (set by updateNumberOfOnlineUsers
+        // below) turns the bare badge digit into "N connected users" so AT
+        // users get context, not a stray "5". See ether/etherpad#7255.
+        $('#editbar [data-key=showusers] > a').append(
+            '<span id="online_count" role="status" aria-live="polite">1</span>');
+      }
+      // Set the initial aria-label. updateNumberOfOnlineUsers otherwise only
+      // fires on userJoin / userLeave / status change — on a solo-author pad
+      // those events never come, and the badge would ship to AT with no
+      // accessible name (the regression Playwright caught on PR #7777).
+      self.updateNumberOfOnlineUsers();
+      // Re-localize on runtime language changes. updateNumberOfOnlineUsers
+      // reads html10n.get, which returns undefined before the bundle loads
+      // and returns translated text afterwards; binding here keeps the
+      // accessible name in sync with applyLanguage(). Same pattern used
+      // by the keyboard hint in ace.ts and the history toolbar labels.
+      if (html10n && typeof (html10n as any).bind === 'function') {
+        (html10n as any).bind('localized', () => self.updateNumberOfOnlineUsers());
       }
 
       $('#otheruserstable tr').remove();
@@ -547,7 +565,15 @@ const paduserlist = (() => {
         localStorage.setItem('recentPads', JSON.stringify(recentPadsList));
       }
 
-      $('#online_count').text(online);
+      // Set both visible text (the badge digit) and the accessible name in
+      // one place so they can't drift. html10n.get returns undefined if the
+      // locale bundle hasn't loaded yet — fall back to an English template
+      // so AT never reads back "undefined".
+      const $count = $('#online_count');
+      $count.text(online);
+      const label = html10n.get('pad.userlist.onlineCount', {count: online})
+          || `${online} connected user${online === 1 ? '' : 's'}`;
+      $count.attr('aria-label', label);
 
       return online;
     },
diff --git a/src/templates/pad.html b/src/templates/pad.html
index 3e981add9..c86dae080 100644
--- a/src/templates/pad.html
+++ b/src/templates/pad.html
@@ -80,10 +80,27 @@
   <!----------------------------->
   <!--------- TOOLBAR ----------->
   <!----------------------------->
+  <!-- Visually-hidden labels for the role=toolbar regions. Sitting outside the
+       <ul>s keeps the toolbar markup valid (HTML disallows non-<li>/<script>
+       children inside <ul>) while still giving each toolbar a translated
+       accessible name via aria-labelledby. See ether/etherpad#7255. -->
+  <span id="editbar-formatting-label" class="sr-only"
+        data-l10n-id="pad.editor.toolbar.formatting">Formatting toolbar</span>
+  <!-- Reuses pad.historyMode.controlsLabel rather than minting a new key:
+       pad_mode.ts already set this as aria-label on #history-controls via JS,
+       and the existing key has translations in nl/de/etc. A new English-only
+       key would cause non-English locales to regress to fallback English. -->
+  <span id="editbar-history-label" class="sr-only"
+        data-l10n-id="pad.historyMode.controlsLabel">Pad history controls</span>
+  <span id="editbar-actions-label" class="sr-only"
+        data-l10n-id="pad.editor.toolbar.actions">Pad actions toolbar</span>
+  <span id="editbar-showmore-label" class="sr-only"
+        data-l10n-id="pad.editor.toolbar.showMore">Show more toolbar buttons</span>
+
   <div id="editbar" class="toolbar">
       <div id="toolbar-overlay"></div>
 
-      <ul class="menu_left" role="toolbar">
+      <ul class="menu_left" role="toolbar" aria-labelledby="editbar-formatting-label">
           <% e.begin_block("editbarMenuLeft"); %>
           <%- toolbar.menu(settings.toolbar.left, isReadOnly, 'left', 'pad') %>
           <% e.end_block(); %>
@@ -100,7 +117,8 @@
            translation keys. We deliberately do NOT put data-l10n-id on
            the icon buttons — that would set their textContent, which on
            a buttonicon glyph would draw the localized string on screen. -->
-      <div id="history-controls" class="history-controls" role="toolbar" hidden>
+      <div id="history-controls" class="history-controls" role="toolbar"
+           aria-labelledby="editbar-history-label" hidden>
         <button type="button" id="history-playpause" class="buttonicon buttonicon-play"
                 aria-pressed="false"></button>
         <button type="button" id="history-leftstep" class="buttonicon buttonicon-step-backward">
@@ -137,12 +155,14 @@
           <option value="1000" data-l10n-id="timeslider.settings.playbackSpeed.1000ms">1000 ms</option>
         </select>
       </div>
-      <ul class="menu_right" role="toolbar">
+      <ul class="menu_right" role="toolbar" aria-labelledby="editbar-actions-label">
           <% e.begin_block("editbarMenuRight"); %>
           <%- toolbar.menu(settings.toolbar.right, isReadOnly, 'right', 'pad') %>
           <% e.end_block(); %>
       </ul>
-      <button type="button" class="show-more-icon-btn" aria-label="Show more toolbar buttons" aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->
+      <button type="button" class="show-more-icon-btn"
+              aria-labelledby="editbar-showmore-label"
+              aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->
   </div>
 
   <% e.begin_block("afterEditbar"); %><% e.end_block(); %>
diff --git a/src/tests/frontend-new/specs/a11y_dialogs.spec.ts b/src/tests/frontend-new/specs/a11y_dialogs.spec.ts
index 2eeb856b4..e9389fdfb 100644
--- a/src/tests/frontend-new/specs/a11y_dialogs.spec.ts
+++ b/src/tests/frontend-new/specs/a11y_dialogs.spec.ts
@@ -139,14 +139,97 @@ test('otherusers region has aria-live and aria-label (no aria-role typo)', async
   expect(ariaRole).toBeNull();
 });
 
-test('show-more toolbar button has aria-label and aria-expanded', async ({page}) => {
+test('show-more toolbar button has an accessible name and aria-expanded', async ({page}) => {
   const btn = page.locator('.show-more-icon-btn');
   const tag = await btn.evaluate((el) => el.tagName.toLowerCase());
   expect(tag).toBe('button');
-  await expect(btn).toHaveAttribute('aria-label', 'Show more toolbar buttons');
+  // The accessible name is supplied by aria-labelledby pointing at a hidden
+  // localized span (so html10n can translate it). Verify the relationship
+  // resolves and produces the expected English string with locale=en-US.
+  await expect(btn).toHaveAttribute('aria-labelledby', 'editbar-showmore-label');
+  await expect(page.locator('#editbar-showmore-label')).toHaveText('Show more toolbar buttons');
   await expect(btn).toHaveAttribute('aria-expanded', 'false');
 });
 
+test('editbar toolbars have role=toolbar with accessible names (#7255)', async ({page}) => {
+  // Lighthouse + AT tooling (firefox a11y inspector) flagged both <ul> toolbars
+  // as unnamed in the 2026-05-16 follow-up on #7255. Each toolbar role now
+  // points to a hidden localized span via aria-labelledby; if either span is
+  // ever removed, getAttribute returns an id with no matching element and the
+  // toolbar becomes unnamed again — so assert the resolved string, not just
+  // the attribute wiring.
+  const cases: Array<[string, string, string]> = [
+    ['.menu_left', 'editbar-formatting-label', 'Formatting toolbar'],
+    ['.menu_right', 'editbar-actions-label', 'Pad actions toolbar'],
+    // History toolbar reuses pad.historyMode.controlsLabel (already
+    // translated in multiple locales) instead of a new English-only key.
+    ['#history-controls', 'editbar-history-label', 'Pad history controls'],
+  ];
+  for (const [sel, labelId, expected] of cases) {
+    const toolbar = page.locator(sel);
+    await expect(toolbar).toHaveAttribute('role', 'toolbar');
+    await expect(toolbar).toHaveAttribute('aria-labelledby', labelId);
+    await expect(page.locator(`#${labelId}`)).toHaveText(expected);
+  }
+});
+
+test('toolbar <li>/<a> wrappers are presentational (Lighthouse listitem rule, #7255)', async ({page}) => {
+  // Lighthouse / axe-core's `listitem` rule flags <li> children of any
+  // element whose role isn't `list` — and role="toolbar" on the <ul>
+  // overrides the implicit list role. Murphy's #7255 follow-up included
+  // the Lighthouse screenshot of this exact failure. role="presentation"
+  // tells axe-core the <li>+<a> wrappers are layout scaffolding, while
+  // the inner <button> retains button semantics for AT.
+  const listItems = page.locator('.menu_left > li, .menu_right > li');
+  const count = await listItems.count();
+  expect(count).toBeGreaterThan(0);
+  for (let i = 0; i < count; i++) {
+    await expect(listItems.nth(i)).toHaveAttribute('role', 'presentation');
+  }
+  // Core's toolbar.ts emits items as <li><a><button>...</button></a></li>;
+  // for those, the wrapping <a> is presentational so AT focus lands on the
+  // <button>, not the empty link. Plugins may emit anchors with their own
+  // role (e.g. ep_subscript_and_superscript renders <a role="button">), so
+  // scope this assertion to core's button-wrappers only — `:has(> button)`
+  // matches the <a> that contain a <button> child, which is what core emits.
+  const anchors = page.locator(
+      '.menu_left > li:not(.separator) > a:has(> button), ' +
+      '.menu_right > li:not(.separator) > a:has(> button)');
+  const aCount = await anchors.count();
+  expect(aCount).toBeGreaterThan(0);
+  for (let i = 0; i < aCount; i++) {
+    await expect(anchors.nth(i)).toHaveAttribute('role', 'presentation');
+  }
+});
+
+test('online_count badge has a localized accessible label (#7255)', async ({page}) => {
+  // The user-count badge in the showusers toolbar button used to expose a
+  // bare digit ("5") to AT, with no clue it was a user count. Now the badge
+  // carries an aria-label generated from pad.userlist.onlineCount that
+  // updates whenever the count changes. role=status + aria-live=polite
+  // means AT announces the change without the user having to refocus.
+  const badge = page.locator('#online_count');
+  await expect(badge).toHaveAttribute('role', 'status');
+  await expect(badge).toHaveAttribute('aria-live', 'polite');
+  // toHaveText / toHaveAttribute poll so the assertions survive the
+  // initial userlist init() pass (which appends the span and then sets
+  // its aria-label asynchronously after html10n + setMyUserInfo land).
+  await expect(badge).toHaveText(/^\d+$/);
+  // English plural form contains "connected user" — covers both singular
+  // and plural without baking the exact count into the test.
+  await expect(badge).toHaveAttribute('aria-label', /connected user/);
+});
+
+test('linemetricsdiv is hidden from screen readers (#7255)', async ({page}) => {
+  // The "Ether X" announcement in the issue's a11y-inspector screenshot was
+  // the outer iframe (titled "Ether") plus a single "x" text leaf from
+  // ace.ts's linemetricsdiv. linemetricsdiv is a measurement-only node — it
+  // holds a single "x" so the renderer can read its computed line height —
+  // and must stay out of the AT tree.
+  const outerFrame = page.frameLocator('iframe[name="ace_outer"]');
+  await expect(outerFrame.locator('#linemetricsdiv')).toHaveAttribute('aria-hidden', 'true');
+});
+
 test('skip-to-content link bypasses toolbar (WCAG 2.4.1, #7255)', async ({page}) => {
   const skip = page.locator('#skip-to-content');
   // It exists in the DOM and is hidden from sighted users by default —