Skip to content

fix: accessibility — keyboard trap, screen reader support, aria-live #7451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JohnMcLear merged 3 commits into from
Apr 5, 2026
Merged

fix: accessibility — keyboard trap, screen reader support, aria-live #7451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4ca4c2
fix: accessibility — keyboard trap, screen reader support, aria-live
JohnMcLear Apr 4, 2026
46101a3
fix: Escape closes gritters first, only exits editor if nothing to di…
JohnMcLear Apr 4, 2026
74c4720
fix: address Qodo review — keyboard hint in iframe, aria-readonly
JohnMcLear Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/static/css/pad.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,19 @@
@import url("pad/loadingbox.css");
@import url("pad/form.css");

/* Screen reader only — visually hidden but announced by assistive technology */
.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border: 0;
}

html {
font-size: 15px;
color: #3e3e3e;
Expand Down
10 changes: 10 additions & 0 deletions src/static/js/ace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,17 @@ const Ace2Editor = function () {
// <body> tag
innerDocument.body.id = 'innerdocbody';
innerDocument.body.classList.add('innerdocbody');
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);
Comment on lines +287 to +297

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Apr 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

@JohnMcLear JohnMcLear Apr 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in latest commit — keyboard hint moved inside the inner iframe with aria-describedby, and aria-readonly added to the editor body.

innerDocument.body.appendChild(innerDocument.createTextNode('\u00A0')); // &nbsp;
Comment on lines +290 to 298

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Apr 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

@JohnMcLear JohnMcLear Apr 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/*
debugLog('Ace2Editor.init() waiting for require kernel load');
Expand Down
20 changes: 16 additions & 4 deletions src/static/js/ace2_inner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,7 @@ function Ace2Inner(editorInfo, cssManagers) {
const setEditable = (newVal) => {
isEditable = newVal;
targetBody.contentEditable = isEditable ? 'true' : 'false';
targetBody.setAttribute('aria-readonly', isEditable ? 'false' : 'true');
targetBody.classList.toggle('static', !isEditable);
};

Expand Down Expand Up @@ -2689,15 +2690,26 @@ function Ace2Inner(editorInfo, cssManagers) {
if (!specialHandled && isTypeForSpecialKey &&
keyCode === 27 &&
padShortcutEnabled.esc) {
// prevent esc key;
// in mozilla versions 14-19 avoid reconnecting pad.

// 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();
Comment thread
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
} catch (e) {
// Cross-origin frame restrictions — ignore.
}
}
Comment on lines +2693 to +2712

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Apr 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

@JohnMcLear JohnMcLear Apr 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix — accessibility changes (ARIA attributes, focus management) require manual screen reader testing. Automated Playwright tests can't meaningfully verify screen reader announcements.

}
if (!specialHandled && isTypeForCmdKey &&
/* Do a saved revision on ctrl S */
Expand Down
2 changes: 0 additions & 2 deletions src/static/js/domline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ domline.createDomLine = (nonEmpty, doesWrap, optBrowser, optDocument) => {

if (document) {
result.node = document.createElement('div');
// JAWS and NVDA screen reader compatibility. Only needed if in a real browser.
result.node.setAttribute('aria-live', 'assertive');
} else {
result.node = {
innerHTML: '',
Expand Down
4 changes: 2 additions & 2 deletions src/templates/pad.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
<!--- PAD EDITOR (in iframe) -->
<!----------------------------->

<div id="editorcontainer" class="editorcontainer"></div>
<div id="editorcontainer" class="editorcontainer" aria-label="Document editor"></div>

<div id="editorloadingbox">
<% e.begin_block("permissionDenied"); %>
Expand Down Expand Up @@ -363,7 +363,7 @@ <h2 data-l10n-id="pad.share.emebdcode"></h2>
<input type="text" id="myusernameedit" disabled="disabled" data-l10n-id="pad.userlist.entername">
</div>
</div>
<div id="otherusers" aria-role="document">
<div id="otherusers" role="document">
<table id="otheruserstable" cellspacing="0" cellpadding="0" border="0">
<tr><td></td></tr>
</table>
Expand Down
Loading