feat: add accessibility ARIA attributes in Ace2Editor.init()#7350
Closed
deepshekhardas wants to merge 2 commits intoether:developfrom
Closed
feat: add accessibility ARIA attributes in Ace2Editor.init()#7350deepshekhardas wants to merge 2 commits intoether:developfrom
deepshekhardas wants to merge 2 commits intoether:developfrom
Conversation
Review Summary by QodoAdd accessibility ARIA attributes to improve screenreader support
WalkthroughsDescription• Add ARIA attributes to outer iframe for screenreader support • Hide line numbers from assistive technology with aria-hidden • Add ARIA attributes to inner iframe and content area • Fix code formatting and indentation throughout file Diagramflowchart LR
A["Outer iframe"] -->|role=application<br/>aria-label| B["Etherpad editor"]
C["Line numbers div"] -->|aria-hidden=true| D["Hidden from screenreaders"]
E["Inner iframe"] -->|role=document<br/>aria-label| F["Pad content"]
G["Content body"] -->|role=textbox<br/>aria-multiline| H["Pad content"]
File Changes1. src/static/js/ace.ts
|
Code Review by Qodo
1. accessibilityAttributes default true
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
src/static/js/ace.ts
Outdated
Comment on lines
189
to
193
| outerFrame.setAttribute('role', 'application'); | ||
| outerFrame.setAttribute('aria-label', 'Etherpad editor'); | ||
| // Some browsers do strange things unless the iframe has a src or srcdoc property: | ||
| // - Firefox replaces the frame's contentWindow.document object with a different object after | ||
| // the frame is created. This can be worked around by waiting for the window's load event |
There was a problem hiding this comment.
1. ace2editor.init() missing aria test 📘 Rule violation ⛯ Reliability
This PR changes editor accessibility behavior by adding ARIA attributes but does not add a regression test that would fail if the fix were reverted. Without automated coverage, the screen reader fix can regress unnoticed.
Agent Prompt
## Issue description
This PR fixes a screen reader accessibility bug by adding ARIA attributes, but it does not include a regression test to ensure the ARIA attributes remain present.
## Issue Context
Per compliance requirements, bug fixes must include a test that fails if the fix is reverted.
## Fix Focus Areas
- src/static/js/ace.ts[185-296]
- src/tests/frontend-new/specs[1-999999]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
There ye go @deepshekhardas - our slop machine is fighting yours ;) Please take action based on the feedback |
Member
|
/review |
|
Persistent review updated to latest commit ef0264e |
Comment on lines
407
to
+410
| alwaysShowChat: false, | ||
| chatAndUsers: false, | ||
| lang: null, | ||
| accessibilityAttributes: true, |
There was a problem hiding this comment.
1. accessibilityattributes default true 📘 Rule violation ⛯ Reliability
The new accessibility behavior is enabled by default, changing runtime behavior without an opt-in flag defaulting to disabled. Additionally, some ARIA behavior (aria-hidden on #sidediv) is applied even when the flag is disabled.
Agent Prompt
## Issue description
The PR introduces a new `accessibilityAttributes` behavior but enables it by default and also applies at least one ARIA-related attribute unconditionally, which violates the requirement that new features must be feature-flagged and disabled by default.
## Issue Context
A new config option (`padOptions.accessibilityAttributes`) is added and used to add ARIA attributes, but the defaults/template set it to `true` and `#sidediv` is always marked `aria-hidden="true"`.
## Fix Focus Areas
- src/node/utils/Settings.ts[407-410]
- settings.json.template[264-266]
- src/static/js/ace.ts[229-233]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
187
to
196
| outerFrame.name = 'ace_outer'; | ||
| outerFrame.frameBorder = 0; // for IE | ||
| outerFrame.title = 'Ether'; | ||
| if (clientVars.padOptions.accessibilityAttributes) { | ||
| outerFrame.setAttribute('role', 'application'); | ||
| outerFrame.setAttribute('aria-label', html10n.get('pad.aria.editor')); | ||
| } | ||
| // Some browsers do strange things unless the iframe has a src or srcdoc property: | ||
| // - Firefox replaces the frame's contentWindow.document object with a different object after | ||
| // the frame is created. This can be worked around by waiting for the window's load event |
There was a problem hiding this comment.
2. No skip-to-content link 📎 Requirement gap ✓ Correctness
The PR adds ARIA roles/labels but does not add a discoverable skip-to-content mechanism that moves focus to the main editor/content area. Screen reader and keyboard users still lack a direct way to bypass surrounding UI to reach the pad content.
Agent Prompt
## Issue description
There is no skip-to-content mechanism for screen reader/keyboard users to jump directly to the main pad content.
## Issue Context
The PR adds ARIA roles/labels to the editor iframes and content body but does not introduce a skip link or equivalent focus-jump mechanism.
## Fix Focus Areas
- src/static/js/ace.ts[186-233]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+90
to
+91
| "pad.aria.editor": "ARIA label for the outer iframe that contains the editor.", | ||
| "pad.aria.pad_content": "ARIA label for the inner iframe and the content body of the pad.", |
There was a problem hiding this comment.
3. Tabs added in qqq.json 📘 Rule violation ✓ Correctness
New localization entries are indented with tab characters, violating the no-tabs / 2-space indentation style requirement. This can cause inconsistent formatting and diff churn.
Agent Prompt
## Issue description
Tabs were introduced in `src/locales/qqq.json`, violating the repository style requirement.
## Issue Context
The new `pad.aria.*` localization documentation strings are tab-indented.
## Fix Focus Areas
- src/locales/qqq.json[90-91]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+3
to
+9
| test.describe('Accessibility ARIA attributes', () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| // Navigate to a new pad | ||
| await page.goto('http://localhost:9001/p/accessibility-test'); | ||
| // Wait for the pad to load | ||
| await expect(page.locator('#editorcontainer')).toHaveClass(/initialized/); | ||
| }); |
There was a problem hiding this comment.
4. Tests use 4-space indent 📘 Rule violation ✓ Correctness
New test files use 4-space indentation instead of the required 2-space indentation. This violates the code style compliance rule.
Agent Prompt
## Issue description
The new accessibility tests are formatted with 4-space indentation instead of the required 2-space indentation.
## Issue Context
Both the Playwright test and the legacy frontend test added in this PR use 4-space indentation.
## Fix Focus Areas
- src/tests/frontend-new/specs/accessibility.spec.ts[3-29]
- src/tests/frontend/specs/accessibility.js[3-28]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+190
to
+193
| if (clientVars.padOptions.accessibilityAttributes) { | ||
| outerFrame.setAttribute('role', 'application'); | ||
| outerFrame.setAttribute('aria-label', html10n.get('pad.aria.editor')); | ||
| } |
There was a problem hiding this comment.
5. Html10n import interop 🐞 Bug ✓ Correctness
ace.ts uses require('.../vendors/html10n') and then calls html10n.get(...), but html10n is a
default export so require() will typically return an object with a .default property. This can
throw at runtime when ARIA attributes are enabled, breaking editor initialization.
Agent Prompt
### Issue description
`src/static/js/ace.ts` uses a bare CommonJS `require()` to import `vendors/html10n`, but the module is a TypeScript **default export**. As a result, `html10n.get` may be undefined at runtime, causing `Ace2Editor.init()` to throw when ARIA attributes are enabled.
### Issue Context
- `src/static/js/vendors/html10n.ts` exports `default`.
- Elsewhere in the repo, default exports imported via `require()` use `.default`.
### Fix Focus Areas
- src/static/js/ace.ts[28-36]
- src/static/js/ace.ts[186-193]
- src/static/js/ace.ts[243-249]
- src/static/js/ace.ts[295-302]
### Suggested change
- Change the import to one of:
- `const html10n = require('ep_etherpad-lite/static/js/vendors/html10n').default;`
- or `import html10n from './vendors/html10n';` (if relative import works with your bundling)
- When setting ARIA labels, consider:
- `const label = html10n.get('pad.aria.editor') || 'Etherpad editor';`
- similarly for `pad.aria.pad_content`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4
to
+9
| test.beforeEach(async ({ page }) => { | ||
| // Navigate to a new pad | ||
| await page.goto('http://localhost:9001/p/accessibility-test'); | ||
| // Wait for the pad to load | ||
| await expect(page.locator('#editorcontainer')).toHaveClass(/initialized/); | ||
| }); |
There was a problem hiding this comment.
6. Hardcoded playwright url 🐞 Bug ⛯ Reliability
The new Playwright test navigates to http://localhost:9001/... directly instead of using the shared helper/baseURL conventions used by other frontend-new specs. This is likely to fail in CI or in environments that run Etherpad on a different host/port.
Agent Prompt
### Issue description
The new Playwright spec hardcodes `http://localhost:9001`, which can break when the test runner uses a different baseURL/port.
### Issue Context
Other `src/tests/frontend-new/specs/*.spec.ts` files use `goToNewPad(page)` from `../helper/padHelper`.
### Fix Focus Areas
- src/tests/frontend-new/specs/accessibility.spec.ts[4-9]
### Suggested change
- Replace the hardcoded `page.goto('http://localhost:9001/...')` with either:
- `await goToNewPad(page);` (preferred, consistent with suite)
- or `await page.goto('/p/accessibility-test');` if baseURL is configured
- (Optional hardening) Instead of asserting exact English strings, assert `aria-label` is present and non-empty, or set the language cookie explicitly to ensure deterministic strings.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
@deepshekhardas are you going to be able to maintain these changes should they require input? |
Member
|
@deepshekhardas Because you are still spamming other repositories I believe you are not a real user. You do not reply to our suggestions. Please check our guidelines for using AI. |
Member
|
@deepshekhardas Because you are still spamming other repositories I believe you are not a real user. You do not reply to our suggestions. Please check our guidelines for using AI. |
Review Summary by QodoAdd ARIA accessibility attributes with feature flag and i18n support
WalkthroughsDescription• Add ARIA accessibility attributes to editor iframes and content areas • Implement feature flag accessibilityAttributes in settings (enabled by default) • Add i18n support with localized labels for screen readers • Include comprehensive test coverage for accessibility features Diagramflowchart LR
A["Settings Configuration"] -->|accessibilityAttributes flag| B["Ace2Editor.init()"]
B -->|Apply ARIA attributes| C["Outer iframe<br/>role=application"]
B -->|Apply ARIA attributes| D["Inner iframe<br/>role=document"]
B -->|Apply ARIA attributes| E["innerdocbody<br/>role=textbox"]
F["i18n Localization<br/>html10n"] -->|Provide labels| C
F -->|Provide labels| D
F -->|Provide labels| E
G["Test Suites"] -->|Verify attributes| H["Frontend Tests"]
File Changes1. src/node/utils/Settings.ts
|
Code Review by Qodo
1. Flag not fully respected
|
| const sideDiv = outerDocument.createElement('div'); | ||
| sideDiv.id = 'sidediv'; | ||
| sideDiv.classList.add('sidediv'); | ||
| sideDiv.setAttribute('aria-hidden', 'true'); |
There was a problem hiding this comment.
1. Flag not fully respected 🐞 Bug ✓ Correctness
The new accessibilityAttributes flag is meant to toggle ARIA accessibility attributes, but #sidediv is always set to aria-hidden="true" regardless of the flag. This makes the toggle incomplete and prevents fully disabling the accessibility behavior.
Agent Prompt
### Issue description
`padOptions.accessibilityAttributes` is intended to enable/disable ARIA accessibility attributes, but `#sidediv` is always marked `aria-hidden="true"`, even when the flag is disabled.
### Issue Context
This makes the setting ineffective for users who want to fully disable the ARIA changes.
### Fix Focus Areas
- src/static/js/ace.ts[226-233]
- settings.json.template[253-267]
### Suggested change
Wrap the `aria-hidden` assignment with the same `clientVars.padOptions.accessibilityAttributes` conditional used for the other ARIA attributes, and optionally remove the attribute when disabled to avoid stale state.
ⓘ 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
3 participants
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
Screen readers currently struggle with Etherpad because the editor iframes and content areas lack proper ARIA attributes. Users relying on assistive technology can only cycle through line numbers with no way to read or interact with the actual pad content.
This PR adds appropriate ARIA roles and labels to improve screenreader navigation.
Closes #7255
Changes
role="application"andaria-label="Etherpad editor"to the outer iframearia-hidden="true"to#sidediv(line numbers) to hide from screenreadersrole="document"andaria-label="Pad content"to the inner iframerole="textbox",aria-multiline="true", andaria-label="Pad content"to#innerdocbodyTesting
This is a focused split from #7341 as discussed with maintainers.