Skip to content

fix(a11y): localized aria-label + title on export-as links#7697

Merged
JohnMcLear merged 2 commits intoether:developfrom
JohnMcLear:fix/a11y-export-aria-labels
May 7, 2026
Merged

fix(a11y): localized aria-label + title on export-as links#7697
JohnMcLear merged 2 commits intoether:developfrom
JohnMcLear:fix/a11y-export-aria-labels

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

Summary

The six export anchors in the import/export dialog (#exportetherpada, #exporthtmla, #exportplaina, #exportworda, #exportpdfa, #exportopena) had no aria-label or title. Their accessible name fell back to the inner icon span's translated text — just the format name (e.g. "Etherpad", "PDF"), with no "export" verb context. The inner span also doubles as the visible glyph, so screen readers picked up the same string twice in some setups.

This PR adds data-l10n-id="pad.importExport.export<format>a.title" to each anchor. html10n populates both the title attribute and aria-label from the same key (the same pattern already used by #chaticon, #titlesticky, .show-more-icon-btn), so:

  • Screen readers announce e.g. "Export as Etherpad" instead of "Etherpad"
  • Sighted users get a localized tooltip on hover
  • Inner icon spans get aria-hidden="true" so they aren't read separately

Six new English strings are added to src/locales/en.json (pad.importExport.exportetherpada.titleexportopena.title). All other locales fall back to English until translatewiki picks up the new keys.

This is the only piece of the original a11y batch (the rest of which already landed via PR #7584 etc.) that hadn't made it into develop.

Test plan

  • pnpm run ts-check passes
  • Strengthened tests/frontend-new/specs/a11y_dialogs.spec.ts "export links" test: asserts each rendered anchor has an aria-label starting with "Export ", title === aria-label, and the inner span has aria-hidden="true". Skips Word/PDF/ODF anchors when pad_impexp.ts strips them (no soffice).
  • Verified end-to-end against a local etherpad: html10n produces aria-label="Export as Etherpad" / title="Export as Etherpad" etc. for the rendered anchors; aria-hidden="true" is on the inner icon spans.
  • CI: existing Playwright suite still green

The six export anchors in the import/export dialog had no aria-label or
title; their accessible name relied on the inner icon span's translated
text (e.g. "Etherpad"). That announces just the format name with no
"export" verb context, and the icon span doubles as the visible glyph
which screen readers also pick up.

Add `data-l10n-id="pad.importExport.export<format>a.title"` to each
anchor — html10n populates both the `title` attribute and `aria-label`
from the same key, so screen readers announce e.g. "Export as Etherpad"
and sighted users get a tooltip. Mark the inner icon span
`aria-hidden="true"` so SR doesn't double-read the format name.

Strengthens the existing `a11y_dialogs.spec.ts` "export links" test to
assert aria-label/title are populated and inner spans are aria-hidden,
and only checks the three formats that are present without soffice.
@qodo-code-review
Copy link
Copy Markdown

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Add localized aria-label and title to export links

🐞 Bug fix 🧪 Tests

Grey Divider

Walkthroughs

Description
• Add localized aria-label and title to six export links
• Mark inner icon spans with aria-hidden="true" to prevent double announcement
• Strengthen a11y test to verify aria-label, title, and aria-hidden attributes
• Add six new English translation strings for export link labels
Diagram
flowchart LR
  A["Export anchors<br/>no aria-label/title"] -->|Add data-l10n-id| B["Anchors with<br/>localized labels"]
  A -->|Add aria-hidden| C["Inner spans<br/>hidden from SR"]
  B -->|html10n expands| D["aria-label +<br/>title populated"]
  E["Test assertions<br/>strengthened"] -->|Verify| D
  E -->|Verify| C
Loading

Grey Divider

File Changes

1. src/templates/pad.html Accessibility enhancement +12/-12

Add localization IDs and aria-hidden to export links

• Add data-l10n-id="pad.importExport.export<format>a.title" to all six export anchor elements
• Add aria-hidden="true" to all inner icon span elements within export links
• Reorder attributes for consistency (data-l10n-id before target)

src/templates/pad.html


2. src/locales/en.json Localization +6/-0

Add English translations for export link labels

• Add six new translation keys for export link titles with "Export as" prefix
• Keys follow pattern pad.importExport.export<format>a.title
• Strings include full format names (e.g., "Export as Etherpad", "Export as Microsoft Word")

src/locales/en.json


3. src/tests/frontend-new/specs/a11y_dialogs.spec.ts 🧪 Tests +12/-8

Strengthen export links accessibility test assertions

• Update test name to reflect new aria-label and title assertions
• Replace innerText check with aria-label validation (must start with "Export ")
• Add assertion that title attribute matches aria-label
• Add assertion that inner span has aria-hidden="true" attribute
• Update test comments to explain html10n expansion and aria-hidden purpose

src/tests/frontend-new/specs/a11y_dialogs.spec.ts


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented May 7, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. English-only aria-label assertion ✓ Resolved 🐞 Bug ☼ Reliability
Description
a11y_dialogs.spec.ts asserts aria-label starts with the English string "Export ", which will
fail once these new localization keys are translated (or if test locale changes to a translated
language). This makes the Playwright suite brittle even though the underlying UI behavior is
correct.
Code

src/tests/frontend-new/specs/a11y_dialogs.spec.ts[R88-91]

+    const ariaLabel = (await locator.getAttribute('aria-label')) ?? '';
+    expect(ariaLabel.startsWith('Export ')).toBe(true);
+    const title = await locator.getAttribute('title');
+    expect(title).toBe(ariaLabel);
Evidence
The test hard-codes an English prefix, but html10n chooses language from document.cookie or
navigator.language (with 'en' as fallback) and will use a translated value whenever the locale
provides one. The Playwright config does not pin locale, and this same spec file already treats
localized aria-labels as non-empty rather than matching English strings, highlighting the
inconsistency.

src/tests/frontend-new/specs/a11y_dialogs.spec.ts[68-95]
src/static/js/l10n.ts[4-13]
src/playwright.config.ts[45-69]
src/tests/frontend-new/specs/a11y_dialogs.spec.ts[97-107]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The export-links a11y test asserts an English-only `aria-label` prefix (`"Export "`). Once non-English translations for `pad.importExport.export<format>a.title` are added, the `aria-label` will be localized and the test will fail.
### Issue Context
`html10n.localize([cookieLang, navigator.language, 'en'])` can select a non-English language, and Playwright configuration does not pin the browser locale.
### Fix Focus Areas
- src/tests/frontend-new/specs/a11y_dialogs.spec.ts[68-95]
- src/playwright.config.ts[45-69]
- src/static/js/l10n.ts[4-13]
### Suggested fix
Pick one:
1) **Locale-agnostic assertions**: replace the `startsWith('Export ')` check with `expect(ariaLabel.length).toBeGreaterThan(0)` (and keep `title === ariaLabel` + `aria-hidden=true`), matching the existing pattern used for `#chaticon`.
2) **Pin test locale**: set Playwright context `locale: 'en-US'` globally or for this test file (`test.use({locale: 'en-US'})`) so the English prefix assertion remains stable.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Missing noopener on blank links ✓ Resolved 🐞 Bug ⛨ Security
Description
The export links in pad.html open in a new tab via target="_blank" but still omit
rel="noopener", leaving window.opener exposed to the opened page. Because this PR touched these
anchors, it’s a good opportunity to fix the reverse-tabnabbing hardening gap.
Code

src/templates/pad.html[R326-342]

+              <a id="exportetherpada" data-l10n-id="pad.importExport.exportetherpada.title" target="_blank" class="exportlink">
+                <span class="exporttype buttonicon buttonicon-file-powerpoint" id="exportetherpad" data-l10n-id="pad.importExport.exportetherpad" aria-hidden="true"></span>
            </a>
-              <a id="exporthtmla" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-code" id="exporthtml" data-l10n-id="pad.importExport.exporthtml"></span>
+              <a id="exporthtmla" data-l10n-id="pad.importExport.exporthtmla.title" target="_blank" class="exportlink">
+                <span class="exporttype buttonicon buttonicon-file-code" id="exporthtml" data-l10n-id="pad.importExport.exporthtml" aria-hidden="true"></span>
            </a>
-              <a id="exportplaina" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file" id="exportplain" data-l10n-id="pad.importExport.exportplain"></span>
+              <a id="exportplaina" data-l10n-id="pad.importExport.exportplaina.title" target="_blank" class="exportlink">
+                <span class="exporttype buttonicon buttonicon-file" id="exportplain" data-l10n-id="pad.importExport.exportplain" aria-hidden="true"></span>
            </a>
-              <a id="exportworda" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-word" id="exportword" data-l10n-id="pad.importExport.exportword"></span>
+              <a id="exportworda" data-l10n-id="pad.importExport.exportworda.title" target="_blank" class="exportlink">
+                <span class="exporttype buttonicon buttonicon-file-word" id="exportword" data-l10n-id="pad.importExport.exportword" aria-hidden="true"></span>
            </a>
-              <a id="exportpdfa" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-pdf" id="exportpdf" data-l10n-id="pad.importExport.exportpdf"></span>
+              <a id="exportpdfa" data-l10n-id="pad.importExport.exportpdfa.title" target="_blank" class="exportlink">
+                <span class="exporttype buttonicon buttonicon-file-pdf" id="exportpdf" data-l10n-id="pad.importExport.exportpdf" aria-hidden="true"></span>
            </a>
-              <a id="exportopena" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-alt" id="exportopen" data-l10n-id="pad.importExport.exportopen"></span>
+              <a id="exportopena" data-l10n-id="pad.importExport.exportopena.title" target="_blank" class="exportlink">
+                <span class="exporttype buttonicon buttonicon-file-alt" id="exportopen" data-l10n-id="pad.importExport.exportopen" aria-hidden="true"></span>
Evidence
pad.html contains multiple export anchors with target="_blank" and no rel. The same template
already uses rel="noopener" on another target="_blank" link, showing the project expects this
hardening pattern.

src/templates/pad.html[323-343]
src/templates/pad.html[270-273]
Best Practice: OWASP Reverse Tabnabbing guidance

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Export links use `target="_blank"` without `rel="noopener"`, allowing the opened page to access `window.opener`.
### Issue Context
These anchors are modified in this PR, so adding `rel` is low-risk and consistent with other `target="_blank"` usage in the same template.
### Fix Focus Areas
- src/templates/pad.html[326-343]
### Suggested fix
Add `rel="noopener"` (or `rel="noopener noreferrer"`) to each export `<a ... target="_blank" ...>` in the export column.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

- Add rel="noopener" to each export anchor: closes the
  reverse-tabnabbing window-opener gap. Other target="_blank" links
  in pad.html (e.g. the "Powered by Etherpad" link) already follow
  this hardening pattern.
- Pin Playwright locale to en-US for the a11y_dialogs spec: this
  file already asserts specific English strings (e.g. "Close chat",
  "Active users on this pad"); without the pin, translatewiki
  updates would break those tests. Tighten the export-anchor
  assertions to exact-match the English aria-label/title now that
  the locale is stable, and assert rel="noopener" too.
@JohnMcLear JohnMcLear merged commit c7ac1cc into ether:develop May 7, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant