-
-
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
Changes from 6 commits
5074bf7
aa1a0ed
aa5dd2f
8432643
515ba19
68a0761
c89444c
f8548b2
c2062fe
59b7ee3
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 |
|---|---|---|
|
|
@@ -128,6 +128,7 @@ exports.padeditbar = new class { | |
| this._editbarPosition = 0; | ||
| this.commands = {}; | ||
| this.dropdowns = []; | ||
| this._lastTrigger = null; | ||
| } | ||
|
|
||
| init() { | ||
|
|
@@ -145,7 +146,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 +210,15 @@ 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. | ||
| const wasAnyOpen = $('.popup.popup-show').length > 0; | ||
| if (!wasAnyOpen && moduleName !== 'none') { | ||
| 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 +247,27 @@ 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. | ||
| const target = openedModule; | ||
| requestAnimationFrame(() => { | ||
| const focusable = target.find( | ||
| 'button:visible, a[href]:visible, input:not([disabled]):visible, ' + | ||
| 'select:not([disabled]):visible, textarea:not([disabled]):visible, ' + | ||
| '[tabindex]:not([tabindex="-1"]):visible').first(); | ||
| if (focusable.length) focusable[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
+305
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 +318,13 @@ exports.padeditbar = new class { | |
| } | ||
|
|
||
| _bodyKeyEvent(evt) { | ||
| // Escape from inside any open popup: close the popup and let | ||
| // toggleDropDown('none') restore focus to the trigger. | ||
| if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) { | ||
| this.toggleDropDown('none'); | ||
| evt.preventDefault(); | ||
| return; | ||
| } | ||
|
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. Users escape close broken Pressing Escape inside the Users popup calls toggleDropDown('none'), but toggleDropDown('none')
explicitly skips the users module, so the Users dialog won’t close. Because the handler
preventsDefault() and returns, the existing Escape behavior won’t run either, leaving keyboard users
stuck in the Users popup.
Agent Prompt
Comment on lines
+356
to
+384
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) { | ||
|
|
||
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