follow-up-v2: Screen Reader: Many Pages: There is no dialog role and title announced#87538
Conversation
…Reader-Many-Pages-There-is-no-dialog-role-and-title-announced
|
@mkhutornyi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 321fe4bf49
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…Reader-Many-Pages-There-is-no-dialog-role-and-title-announced
|
Great cleanup! ✨ |
|
Friendly bump @mkhutornyi When you have a moment. Thanks |
Code ReviewNice cleanup — replacing 40+ lines of workaround code ( What looks good:
One minor note:
No blocking issues found. LGTM. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 682fd09d33
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const pushLabel = (text: string): number => { | ||
| const id = nextIdRef.current++; | ||
| labelStackRef.current = [...labelStackRef.current, {id, text}]; | ||
| initialFocusClaimedRef.current = false; |
There was a problem hiding this comment.
Avoid resetting initial focus claim on each label push
Resetting initialFocusClaimedRef inside pushLabel makes focus eligibility reopen on any label re-registration, not just real screen transitions. useDialogLabelRegistration re-runs its effect whenever pushLabel/popLabel identities change (src/hooks/useDialogLabelRegistration.ts:12-18), which happens when DialogLabelProvider re-renders; that causes a new pushLabel() call, flips this flag back to false, and allows useDialogContainerFocus in Header (src/components/Header.tsx:54-56) to re-focus dialog controls during unrelated re-renders (for example responsive/layout updates), leading to unexpected focus jumps.
Useful? React with 👍 / 👎.
trjExpensify
left a comment
There was a problem hiding this comment.
Accessibility project PR 👍
|
@mkhutornyi can you please prioritize this PR, it's holding 2 other issues |
|
On it now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Explanation of Change
Follow-up to #85221. Fixes deploy blocker #87304 and improves focus ring behavior.
What's changed
useDialogContainerFocus— ReplacedhadKeyboardEventwithhadTabNavigationto distinguish Tab navigation from form interaction. Tab sets the flag, typing keys clear it, Enter/Space preserve it, mousedown clears it. UsesfocusVisiblebrowser API (target.focus({focusVisible: hadTabNavigation})) to control the focus ring at the source instead of fighting the browser's:focus-visibleheuristic with workarounds.focusVisible: true— ring shows on Back buttonfocusVisible: false— Back button focused silently, prevents deploy blocker [Due for payment 2026-04-17] [Due for payment 2026-04-16] Web - Expense - Enter on confirm page returns to amount input page #87304focusVisible: false— no ringfocusVisible: false— no ringDialogLabelContext—pushLabelresetsinitialFocusClaimedRefso each inner navigation screen can claim focus. Required by WCAG 2.4.3 and [HOLD] [$500] Keyboard Navigation: Many Pages: The focus is lost and does not return to triggering element #76921.FOCUSABLE_SELECTOR— Addedinput, textarea, selectand skips:disabled/aria-disabled="true"elements.Removed 40+ lines of workaround code —
data-programmatic-focusattribute,outline: noneinline style,onFirstTabTab interception, cleanup functions, and listener leak management. All replaced by thefocusVisibleAPI.Unit tests — 21 tests for
focusFirstInteractiveElement(guards, Tab/typing/mouse/Enter/Space modality, aria-hidden, disabled, selectors). 10 tests forDialogLabelContext. Each describe block sets modality explicitly viabeforeEach.Fixed Issues
$ #76944
$ #87304
PROPOSAL: #76944 (comment)
Tests
Test 1: Deploy blocker #87304 - Type amount + Enter does not activate Back button
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-04-09.at.11.31.17.at.night.mov