[CP staging] follow-up: Screen Reader: Many Pages: There is no dialog role and title announced#87292
Conversation
|
@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] |
|
@mkhutornyi @amyevans One of you needs to 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: 9df8884c29
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Code Review — PR #87292 Overall the approach is solid: extracting 1. Test ordering dependency (medium) The test file relies on module-level 2. Module-level listeners never removed (low) The 3. Removing Everything else looks clean. The |
|
Bug? After refresh page, go to another tab and back, back button is focused Screen.Recording.2026-04-08.at.9.04.31.am.mov |
|
Please check last 2 comments. Otherwise looks good |
|
This PR prevents #87304 because of the Before this PR: Every time a new screen pushed a label (inner navigation like amount → confirm), the focus claim was reset, so After this PR: The updated test at |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari87292.mov |
|
🤖 Code Review The core fix (removing 1. Listener leak in Suggested fix: Track the listener in the 2. PR description mentions 3. Minor: named exports only consumed by tests Everything else — the modality detection ( |
|
@TaduJR please address #87292 (comment) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b22a3df1a
ℹ️ 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".
|
🤖 Code Review The PR is well-structured — the extraction of Minor: The selector Nit: Missing test for disabled-element edge case ( A test verifying that a disabled first element is skipped (or that stale attributes don't persist) would strengthen the suite. Everything else looks good — the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5b8132d4c
ℹ️ 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".
mkhutornyi
left a comment
There was a problem hiding this comment.
Please check minor comments above.
Otherwise looks good
Done. |
|
🚧 @amyevans has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
…ere-is-no-dialog-role-and-title-announced [CP staging] follow-up: Screen Reader: Many Pages: There is no dialog role and title announced (cherry picked from commit 9e3c1b5) (cherry-picked to staging by roryabraham)
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.3.54-4 🚀
|
|
No help site changes required. This PR modifies internal focus management logic ( The help site articles under |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.54-7 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.3.55-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. The changes are purely internal accessibility/focus-management fixes (focus ring suppression, keyboard navigation, deploy blocker fix for Enter key on confirm pages). They don't affect any documented user flows, feature names, settings labels, or UI workflows covered by help site articles under |
|
Test 9 of this PR failing because of the issue #87469 |
Explanation of Change
Follow-up to #85221. Fixes deploy blocker #87304 (Enter on confirm page activates Back button instead of submitting) and fixes focus ring showing on page load/refresh.
What's changed
DialogLabelContext- RemovedinitialFocusClaimedRefreset frompushLabel. Focus is now only claimed once per dialog lifecycle (when the RHP first opens), not on every inner screen transition. This prevents the Back button from stealing focus on screens like the expense confirm page.useDialogContainerFocus- Extracted focus logic intofocusFirstInteractiveElement()for unit testability. Added input-modality-aware focus ring handling:data-programmatic-focus+outline: none. RegistersonFirstTabhandler that intercepts the first Tab key and re-focuses with a visible ring, preventing Tab from skipping the silently focused element. Suppression attributes survive browser tab-switch blur/re-focus cycles (no blur cleanup).keydownlistener detects keyboard usage before the hook mounts).mousedownlistener resets keyboard flag so suppression is consistently restored after switching from keyboard to mouse.onFirstTablistener registered without{once: true}so non-Tab keys don't consume it. Listener cleaned up via returned cleanup function called inuseEffectteardown to prevent leaks on dialog unmount.FOCUSABLE_SELECTOR- Addedinput, textarea, selectfor more robust first-focusable-element discovery.Unit tests - 21 tests for
focusFirstInteractiveElement(guards, suppression, onFirstTab, cleanup function, non-Tab persistence, focus-moved-away, keyboard path, mousedown reset, tab-switch resilience, aria-hidden, selectors). Each describe block sets its own modality explicitly to avoid test ordering dependencies. 10 tests forDialogLabelContext(updated focus claim test to verify pushLabel does not reset claim).Fixed Issues
$ #76944
$ #87304
PROPOSAL: #76944 (comment)
Tests
Test 1: No focus ring on page load
staging.new.expensify.com/settings/about/app-download-linksin the URL bar and press Enter) or go to App Download Links RHP and refresh the pageTest 2: Tab after page load lands on Back button
Test 3: Keyboard navigation shows focus ring immediately
Test 4: Mouse click then Tab lands on Back button
Test 5: Repeated mouse clicks work consistently
Test 6: Auto-focused input is not stolen
Test 7: Enter on confirm page does not go back (deploy blocker #87304)
Test 8: Tab switch does not show focus ring
staging.new.expensify.com/settings/about/app-download-linksin the URL bar)Test 9: Inner navigation does not re-focus 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
Mac-Chrome.mp4