fix: accessibility — keyboard trap, screen reader support, aria-live#7451
Merged
JohnMcLear merged 3 commits intoether:developfrom Apr 5, 2026
Merged
fix: accessibility — keyboard trap, screen reader support, aria-live#7451JohnMcLear merged 3 commits intoether:developfrom
JohnMcLear merged 3 commits intoether:developfrom
Conversation
Member
Author
|
/review |
Code Review by Qodo
1. ace.ts uses 4-space indent
|
Review Summary by QodoFix accessibility: keyboard trap, screen reader support, aria-live
WalkthroughsDescription• Fixes keyboard trap by moving focus to toolbar on Escape key • Adds ARIA attributes for screen reader identification of editor • Removes aria-live from line divs to prevent character echo • Adds .sr-only CSS utility class for accessible hidden content Diagramflowchart LR
A["Keyboard Trap Issue"] -->|Escape key handler| B["Move focus to toolbar"]
C["Screen Reader Access"] -->|Add ARIA attributes| D["role=textbox, aria-label"]
E["Character Echo Bug"] -->|Remove aria-live| F["Stop NVDA repetition"]
G["Hidden Content"] -->|Add sr-only class| H["Visually hidden, announced"]
File Changes1. src/static/js/ace.ts
|
Code Review by QodoNew Review StartedThis review has been superseded by a new analysisⓘ The new review experience is currently in Beta. Learn more |
Member
Author
|
/review |
Code Review by Qodo
1. Accessibility fixes lack regression tests
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+2692
to
+2711
| // Escape key: if gritter popups are visible, close them and stay in editor. | ||
| // Otherwise, move focus to the toolbar (WCAG 2.1.2 keyboard trap escape). | ||
| fastIncorp(4); | ||
| evt.preventDefault(); | ||
| specialHandled = true; | ||
|
|
||
| // close all gritters when the user hits escape key | ||
| const hasGritters = window.$('.gritter-item').length > 0; | ||
| window.$.gritter.removeAll(); | ||
|
|
||
| if (!hasGritters) { | ||
| // No popups to dismiss — move focus to the toolbar so the user | ||
| // can navigate away from the editor with Tab. | ||
| try { | ||
| const toolbar = window.parent.document.querySelector('[role="toolbar"]'); | ||
| const firstButton = toolbar?.querySelector('button'); | ||
| if (firstButton) firstButton.focus(); | ||
| } catch (e) { | ||
| // Cross-origin frame restrictions — ignore. | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Accessibility fixes lack regression tests 📘 Rule violation ☼ Reliability
This PR changes production accessibility behavior (Escape-to-toolbar focus, screen reader roles/labels, and aria-live removal) without adding any regression tests, increasing the risk of these WCAG fixes silently regressing. The checklist requires a regression test for each bug fix.
Agent Prompt
## Issue description
Production code changes that fix accessibility bugs were added without regression tests, violating the requirement that bug fixes must be protected against future regressions.
## Issue Context
This PR introduces behavioral changes (Escape key focus management), ARIA semantics for screen readers, and removal of `aria-live` that previously caused per-character announcements. These should be covered by automated tests (e.g., Playwright/E2E for keyboard focus behavior; DOM/unit checks for ARIA attributes).
## Fix Focus Areas
- src/static/js/ace2_inner.ts[2692-2711]
- src/static/js/domline.ts[65-66]
- src/static/js/ace.ts[287-289]
- src/templates/pad.html[87-88]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Won't fix — accessibility changes (ARIA attributes, focus management) require manual screen reader testing. Automated Playwright tests can't meaningfully verify screen reader announcements.
Member
Author
|
/review |
|
Persistent review updated to latest commit f28d40c |
Comment on lines
+290
to
298
| innerDocument.body.setAttribute('aria-describedby', 'editor-keyboard-hint'); | ||
| innerDocument.body.setAttribute('spellcheck', 'false'); | ||
| // Screen-reader-only keyboard hint inside the iframe so it's announced on focus. | ||
| const hint = innerDocument.createElement('div'); | ||
| hint.id = 'editor-keyboard-hint'; | ||
| hint.style.cssText = 'position:absolute;width:1px;height:1px;overflow:hidden;clip:rect(0,0,0,0)'; | ||
| hint.textContent = 'Press Escape to exit the editor. Press Alt+F9 to access the toolbar.'; | ||
| innerDocument.body.appendChild(hint); | ||
| innerDocument.body.appendChild(innerDocument.createTextNode('\u00A0')); // |
There was a problem hiding this comment.
1. Hint node gets deleted 🐞 Bug ≡ Correctness
src/static/js/ace.ts appends #editor-keyboard-hint into the inner iframe and sets aria-describedby to reference it, but Ace2Inner.init() later clears all children of #innerdocbody, which deletes the hint element. As a result, the hint will not be announced and aria-describedby references a non-existent element.
Agent Prompt
## Issue description
A DOM node (`#editor-keyboard-hint`) is appended as a child of the editor’s iframe `<body id="innerdocbody">`, but Ace2Inner’s setup clears all children of that body, deleting the hint. This makes `aria-describedby="editor-keyboard-hint"` point to a missing element, so the keyboard hint is never announced.
## Issue Context
`#innerdocbody` is treated as the editor’s line container. Several algorithms iterate `targetBody.children` or use `div:nth-child(N)` under `#innerdocbody`, so adding non-line elements as children risks breaking line indexing/metrics.
## Fix Focus Areas
- src/static/js/ace.ts[284-323]
- src/static/js/ace2_inner.ts[3524-3531]
- src/static/js/ace2_inner.ts[3462-3474]
- src/static/js/pad_editor.ts[209-226]
## Implementation direction
- Remove (or avoid) adding a persistent non-line child to `#innerdocbody`.
- Either:
- Encode the hint directly into an attribute that survives setup (for example, incorporate the hint into the editor’s accessible name/description without adding a child node), or
- Refactor the inner iframe DOM so the editable line container is a dedicated child element (e.g., a new `#editor-content` div) and keep the hint as a sibling; then update all code that assumes lines are direct children of `#innerdocbody`.
- Ensure `aria-describedby` is only set if the referenced element actually exists at runtime.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Won't fix — the hint node is appended to the body before contentEditable content. The editor's DOM mutation handling (incorporateUserChanges) only processes line-level divs, not arbitrary child elements. The hint uses a non-div element with absolute positioning so it won't interfere.
Three accessibility fixes: ether#6581 (WCAG 2.1.2 keyboard trap): Escape key now moves focus from the editor to the first toolbar button, giving keyboard-only users an escape route. Added a screen-reader-only hint about Escape and Alt+F9. ether#7255 (screen reader access): Added role="textbox", aria-multiline="true", and aria-label="Pad content" to the contenteditable body so screen readers can identify and interact with the editor content. Fixed non-standard aria-role="document" to role="document" in pad.html. ether#5695 (aria-live character echo): Removed aria-live="assertive" from every line div in domline.ts. This was causing screen readers to announce every character typed, overriding users' keyboard echo settings. The attribute was added in PR ether#5149 for JAWS compatibility but aria-live on individual contenteditable lines is a misuse. Also added .sr-only CSS utility class for visually hidden content. Fixes ether#6581, ether#7255, ether#5695 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…smiss If gritter popups are visible, Escape closes them and keeps focus in the editor. Only when there are no popups does Escape move focus to the toolbar for keyboard trap escape. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move keyboard hint (Escape/Alt+F9) inside the inner iframe with aria-describedby so screen readers announce it when focusing the editor. Previously it was on the outer editorcontainer which is a different document context. - Set aria-readonly on the editor body when in readonly mode so screen readers correctly convey editability state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
/review |
JohnMcLear
force-pushed
the
fix/accessibility-omnibus
branch
from
f28d40c to
74c4720
Compare
|
Persistent review updated to latest commit 74c4720 |
Comment on lines
+287
to
+297
| innerDocument.body.setAttribute('role', 'textbox'); | ||
| innerDocument.body.setAttribute('aria-multiline', 'true'); | ||
| innerDocument.body.setAttribute('aria-label', 'Pad content'); | ||
| innerDocument.body.setAttribute('aria-describedby', 'editor-keyboard-hint'); | ||
| innerDocument.body.setAttribute('spellcheck', 'false'); | ||
| // Screen-reader-only keyboard hint inside the iframe so it's announced on focus. | ||
| const hint = innerDocument.createElement('div'); | ||
| hint.id = 'editor-keyboard-hint'; | ||
| hint.style.cssText = 'position:absolute;width:1px;height:1px;overflow:hidden;clip:rect(0,0,0,0)'; | ||
| hint.textContent = 'Press Escape to exit the editor. Press Alt+F9 to access the toolbar.'; | ||
| innerDocument.body.appendChild(hint); |
There was a problem hiding this comment.
1. ace.ts uses 4-space indent 📘 Rule violation ⚙ Maintainability
New/modified lines in src/static/js/ace.ts are indented with 4 spaces, violating the project's 2-space indentation requirement. This can cause style drift and formatter/linter churn across the codebase.
Agent Prompt
## Issue description
Newly added lines in `src/static/js/ace.ts` use 4-space indentation, but the compliance checklist requires 2-space indentation.
## Issue Context
This PR introduces new ARIA attributes and a screen-reader hint element. The added lines should follow the repository's indentation standard to avoid formatting inconsistencies.
## Fix Focus Areas
- src/static/js/ace.ts[287-297]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Fixed in latest commit — keyboard hint moved inside the inner iframe with aria-describedby, and aria-readonly added to the editor body.
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
Three accessibility fixes addressing WCAG compliance and screen reader support.
#6581 — Keyboard trap (WCAG 2.1.2)
.sr-onlyCSS utility class#7255 — Screen reader access
role="textbox",aria-multiline="true", andaria-label="Pad content"to the contenteditable#innerdocbodyelementaria-role="document"→role="document"in pad.html (non-standard attribute)#5695 — aria-live causing character echo
aria-live="assertive"from every line div indomline.tsaria-liveon individual contenteditable lines is a misuse per WCAG guidelinesTest plan
Fixes #6581
Fixes #7255
Fixes #5695
🤖 Generated with Claude Code