-
-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(a11y): dialog semantics, focus management, icon labels, html lang #7584
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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
5074bf7
aa1a0ed
aa5dd2f
8432643
515ba19
68a0761
c89444c
f8548b2
c2062fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,8 +66,22 @@ class ToolbarItem { | |
| bind(callback) { | ||
| if (this.isButton()) { | ||
| this.$el.on('click', (event) => { | ||
| // Stash the clicked button as the focus-restore target BEFORE we | ||
| // blur :focus — but only for dropdown-opening buttons. Non-dropdown | ||
| // commands (list toggles, bold, etc.) return focus to the ace editor | ||
| // and should not touch _lastTrigger (it would retain a stale | ||
| // reference and mess with later popup Esc-close focus handling). | ||
| const cmd = this.getCommand(); | ||
| // @ts-ignore — padeditbar is the exported singleton defined below | ||
| const isDropdownTrigger = exports.padeditbar.dropdowns.indexOf(cmd) !== -1; | ||
| if (isDropdownTrigger) { | ||
| const trigger = (this.$el.find('button')[0] as HTMLElement | undefined) || | ||
| (this.$el[0] as HTMLElement); | ||
| // @ts-ignore | ||
| if (trigger) exports.padeditbar._lastTrigger = trigger; | ||
| } | ||
| $(':focus').trigger('blur'); | ||
| callback(this.getCommand(), this); | ||
| callback(cmd, this); | ||
| event.preventDefault(); | ||
| }); | ||
| } else if (this.isSelect()) { | ||
|
|
@@ -128,6 +142,7 @@ exports.padeditbar = new class { | |
| this._editbarPosition = 0; | ||
| this.commands = {}; | ||
| this.dropdowns = []; | ||
| this._lastTrigger = null; | ||
| } | ||
|
|
||
| init() { | ||
|
|
@@ -145,7 +160,8 @@ exports.padeditbar = new class { | |
| }); | ||
|
|
||
| $('.show-more-icon-btn').on('click', () => { | ||
| $('.toolbar').toggleClass('full-icons'); | ||
| const expanded = $('.toolbar').toggleClass('full-icons').hasClass('full-icons'); | ||
| $('.show-more-icon-btn').attr('aria-expanded', String(expanded)); | ||
| }); | ||
| this.checkAllIconsAreDisplayedInToolbar(); | ||
| $(window).on('resize', _.debounce(() => this.checkAllIconsAreDisplayedInToolbar(), 100)); | ||
|
|
@@ -208,6 +224,19 @@ exports.padeditbar = new class { | |
| $('.nice-select').removeClass('open'); | ||
| $('.toolbar-popup').removeClass('popup-show'); | ||
|
|
||
| // Remember the trigger so we can restore focus when the dialog closes. | ||
| // The Button click handler pre-sets `_lastTrigger` before calling blur(), | ||
| // because blur would make document.activeElement === <body>. For other | ||
| // paths (keyboard shortcut, programmatic open) fall back to whatever has | ||
| // focus right now. | ||
| const wasAnyOpen = $('.popup.popup-show').length > 0; | ||
| if (!wasAnyOpen && moduleName !== 'none' && !this._lastTrigger) { | ||
| const active = document.activeElement; | ||
| if (active && active !== document.body) this._lastTrigger = active; | ||
| } | ||
|
|
||
| let openedModule = null; | ||
|
|
||
| // hide all modules and remove highlighting of all buttons | ||
| if (moduleName === 'none') { | ||
| for (const thisModuleName of this.dropdowns) { | ||
|
|
@@ -236,9 +265,40 @@ exports.padeditbar = new class { | |
| } else if (thisModuleName === moduleName) { | ||
| $(`li[data-key=${thisModuleName}] > a`).addClass('selected'); | ||
| module.addClass('popup-show'); | ||
| openedModule = module; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (openedModule) { | ||
| // Move focus into the now-visible popup so keyboard users land inside the dialog. | ||
| // Skip if a command handler already placed focus inside this popup — the Embed | ||
| // command focuses #linkinput deliberately, which is different from the first | ||
| // tabbable element (a readonly checkbox) and should win. | ||
| // Fallback: if no focusable descendant exists (e.g. #users where the only | ||
| // input is disabled), focus the popup div itself so keydown events fire on | ||
| // the outer document instead of being trapped in the ace editor iframe. | ||
| const target = openedModule; | ||
| requestAnimationFrame(() => { | ||
| // If a command handler already placed focus inside the popup (e.g. | ||
| // the Embed command focuses #linkinput, showusers focuses | ||
| // #myusernameedit), honour that. | ||
| if (target[0].contains(document.activeElement)) return; | ||
| // Otherwise focus the popup container itself. This keeps keydown | ||
| // events on the outer document (so Esc always dismisses the popup, | ||
| // even when the popup has no directly-focusable descendants like | ||
| // #users does), and it works uniformly across browsers without | ||
| // getting tripped up by `visibility: hidden` nested popups. | ||
| // Keyboard users can Tab from here into the popup's controls. | ||
| if (!target.attr('tabindex')) target.attr('tabindex', '-1'); | ||
| target[0].focus(); | ||
| }); | ||
|
Comment on lines
+273
to
+295
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Embed focus overridden toggleDropDown() now focuses the first focusable element in the opened popup on the next animation frame, which overrides existing command-specific focus logic. In the Embed popup this steals focus from #linkinput (which the embed command intentionally focuses/selects) and moves it to the readonly checkbox instead. Agent Prompt
|
||
| } else if ($('.popup.popup-show').length === 0 && this._lastTrigger) { | ||
| // All popups closed — restore focus to the element that opened the first one. | ||
| const trigger = this._lastTrigger; | ||
| this._lastTrigger = null; | ||
| if (document.body.contains(trigger)) trigger.focus(); | ||
| } | ||
|
Comment on lines
+273
to
+301
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Stale focus restoration padeditbar.toggleDropDown('none') now restores focus to this._lastTrigger whenever no popups are
open, even if no popup was previously open. Because _lastTrigger is set on every toolbar button
click, background calls to toggleDropDown('none') (e.g., connection-state handling) can unexpectedly
move focus from the editor to a stale toolbar button.
Agent Prompt
|
||
| } catch (err) { | ||
| cbErr = err || new Error(err); | ||
| } finally { | ||
|
|
@@ -289,6 +349,23 @@ exports.padeditbar = new class { | |
| } | ||
|
|
||
| _bodyKeyEvent(evt) { | ||
| // Escape while any popup is open: close it. We don't restrict to | ||
| // `:focus inside popup` because some popups (e.g. #users) have no | ||
| // focusable content on open — focus stays in the ace editor iframe — | ||
| // but Esc should still dismiss them for keyboard users. | ||
| if (evt.keyCode === 27 && $('.popup.popup-show').length > 0) { | ||
| // `toggleDropDown('none')` intentionally skips the users popup so | ||
| // switching between other popups doesn't hide the user list. For | ||
| // Escape we want the users popup to close too (unless pinned). | ||
| const openPopup = $('.popup.popup-show').first(); | ||
| if (openPopup.attr('id') === 'users' && !openPopup.hasClass('stickyUsers')) { | ||
| openPopup.removeClass('popup-show'); | ||
| $('li[data-key=users] > a').removeClass('selected'); | ||
| } | ||
| this.toggleDropDown('none'); | ||
| evt.preventDefault(); | ||
| return; | ||
| } | ||
|
Comment on lines
+352
to
+368
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Escape won't close colorpicker padeditbar._bodyKeyEvent() intercepts Escape whenever any .popup has .popup-show, but it only
closes dropdown popups via toggleDropDown('none') (and special-cases #users). Popups that are
opened outside toggleDropDown (such as #mycolorpicker) keep .popup-show, so Escape becomes a
no-op while still calling preventDefault() and returning early.
Agent Prompt
|
||
| // If the event is Alt F9 or Escape & we're already in the editbar menu | ||
| // Send the users focus back to the pad | ||
| if ((evt.keyCode === 120 && evt.altKey) || evt.keyCode === 27) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -662,9 +662,15 @@ export class Html10n { | |
| if (node.children.length === 0 || prop != 'textContent') { | ||
| // @ts-ignore | ||
| node[prop] = str.str! | ||
| node.setAttribute("aria-label", str.str!); // Sets the aria-label | ||
| // The idea of the above is that we always have an aria value | ||
| // This might be a bit of an abrupt solution but let's see how it goes | ||
| // Populate aria-label from the translation so screen readers always get | ||
| // a localized accessible name, but do not overwrite an explicit | ||
| // aria-label that an author has already set. This lets templates use | ||
| // static English aria-labels for icon-only controls (export links, | ||
| // chat icon, close/pin buttons) without losing them at localization | ||
| // time. See PR #7584 review feedback. | ||
| if (!node.hasAttribute('aria-label')) { | ||
| node.setAttribute('aria-label', str.str!); | ||
| } | ||
|
Comment on lines
+665
to
+673
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Stale aria-label after relocalize html10n.translateNode() now refuses to overwrite an existing aria-label, but html10n itself sets aria-label during the first localization pass, so later html10n.localize() calls (triggered by language changes) will not update accessible names. This can leave screen readers announcing labels in the old language even though the visible text has been translated, because aria-label overrides text content for the accessible name. Agent Prompt
|
||
| } else { | ||
| let children = node.childNodes, | ||
| found = false | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. Focus restore captures wrong trigger
🐞 Bug≡ CorrectnessAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools