fix: Screen Reader: Global: Focus is lost when returning to the previous screen#91752
Conversation
|
@marufsharifi 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] |
|
cc @mkhutornyi |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
@MelvinBot review |
Code ReviewSummary: This PR implements native screen reader focus-return — when a user navigates back, TalkBack/VoiceOver refocuses the element that triggered the forward navigation. The architecture mirrors the existing web implementation in Issues to address1. New
The view-attachment check is genuinely needed to avoid sending focus events to detached views. However, is there an alternative approach that doesn't deepen the dependency on this deprecated API? If not, a code comment explaining why this usage is justified (and what would replace it when 2. In const composedRef = useMemo(() => mergeRefs(ref, internalRef), [ref]);
3. In the original web code (line 535 of the old NavigationFocusReturn.ts), the Suggestions (non-blocking)4. Android
5. In Next Steps: Reply with |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
Most are Invalid. But was initially working on findNodeHandle migration. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a2a3e8120
ℹ️ 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".
Done. |
There are sooooo many tests but are they really useful and able to catch regressions in the future? |
|
Yes There are most that I have added during testing to prevent future regressions. Also since we are orchestrating lib workarounds I am trying to simulate different cases and race conditions for future conditions. Hopefully react-native-screens and react-navigations would fix some of the bugs and states so we can remove unneeded diffs. I am planning to try to handle these on upstream in the future. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1ec067f9c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6c37412e6
ℹ️ 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".
|
|
3859725 to
a654a36
Compare
Ugghh, Sorry everyone. Please ignore this. I made a mistake during conflict resolution. Lost a all the commits. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ 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". |
mkhutornyi
left a comment
mkhutornyi
left a comment
There was a problem hiding this comment.
No blocker from my side
@roryabraham need your 👁️ on this big boy. 5k lines of code
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
Let's split this PR up a bit into smaller, easier-to-review chunks that each provide some incremental value.
PR 1 — Stable row identity
Replace title-derived and array-index React keys with stable, value-derived keys across a handful of pages and components. Pure React hygiene (prevents remount-on-edit bugs) with no accessibility dependency — can ship anytime.
Example: ProfilePage.tsx keys menu items by detail.testID instead of ${detail.title}_${index}, and SpendRuleMerchantsBase.tsx keys rows by rowId instead of a merchant-name-derived string.
PR 2 — Accessibility warm-cache
Rewrite the screen-reader/reduce-motion state cache to warm eagerly on load and refresh when the app resumes from background, fixing a boot-race and a stale-cache bug that affects every existing caller today. Purely additive — no new call sites required.
Example: the new makeWarmCache helper backs the rewritten subscribeScreenReader, replacing the old fire-and-forget AccessibilityInfo.addEventListener/isScreenReaderEnabled() pair.
PR 3 — Save-and-close skip-restore
Add an explicit "skip focus restore" option to back navigation, plus a reusable hook, so a submit-driven save-and-close doesn't re-focus the row that opened the form and hijack the next Enter press. This generalizes a pattern already live on one screen and rolls it out more broadly. No native dependency.
Example: the new shouldSkipFocusRestore option on GoBackOptions, consumed via the new useNavigateBackOnSave hook, which replaces an ad-hoc ref+effect pattern already in production in IOURequestStepMerchant.tsx.
PR 4 — Extract shared focus utilities
Pure refactor of the already-shipped web focus-return module: pull the navigation-state diffing, a small FIFO map, timing constants, and a visibility check out into standalone files, and reorganize the module into a directory so a native counterpart can be added later. Zero behavior change.
Example: navigationStateDiff.ts's diffNavigationState is lifted verbatim out of the old NavigationFocusReturn.ts, which now imports it back in at its new path.
PR 5 — TransitionTracker nav-kind fix
Teach the transition tracker to tell screen-navigation transitions apart from unrelated modal/keyboard ones, fixing a real ~1s stall that happens on back navigation today. Builds directly on PR 4's refactor.
Example: the new TransitionKind distinction, fed by ScreenLayout.tsx marking screen transitions as 'navigation'.
PR 6 — WCAG 2.4.7 initial focus + dialog labeling
Add proper focus-visible handling for initial focus when a screen opens (so screen readers announce it) and when a modal closes, sharing one "claim focus" helper between both. Web-only. Needs PR 2's screen-reader state and PR 4's timing constants.
Example: the new claimInitialFocus helper is shared by the new useScreenInitialFocus hook (screen open) and the modal-close path in FocusTrapForModal.
PR 7 — Native focus-return rescue (this PR)
The core new feature: the actual native (iOS/Android) mechanism that restores accessibility focus to the row a user pressed, once they navigate back to that screen. This is the biggest, riskiest chunk, and where the reviewer concerns about findNodeHandle, retry budgets, and race conditions apply. Needs PR 2, 4, and 5; works more reliably with PR 1's stable row identifiers but doesn't strictly require them.
Example: the new restoreTriggerForRoute/registerPressable registry in the new native module, fed by BaseGenericPressable.tsx registering every press.
Rollout / dependency graph
Critical path is only 3 PRs deep: PR4 → PR5 → PR7. Everything else is parallel.
flowchart TD
subgraph wave1 ["Wave 1 - parallel, no deps"]
PR1["PR 1: Stable row identity"]
PR2["PR 2: Accessibility warm-cache"]
PR3["PR 3: Save-and-close"]
PR4["PR 4: Extract shared utilities"]
end
subgraph wave2 ["Wave 2 - parallel to each other"]
PR5["PR 5: TransitionTracker fix"]
PR6["PR 6: WCAG 2.4.7 initial focus"]
end
subgraph wave3 ["Wave 3"]
PR7["PR 7: Native focus-return rescue"]
end
PR4 --> PR5
PR2 --> PR6
PR4 --> PR6
PR2 --> PR7
PR4 --> PR7
PR5 --> PR7
PR1 -.->|soft dep| PR7
Merge PR1-4 first, any order. Then PR5+PR6 in parallel. PR7 last.
|
Hi @roryabraham @rushatgabhane Is the splitting idea still around or are we going to continue on the PR due to the urgency of this issue? |
splitting idea is still around. Please work on all PRs as priority. |
|
…Reader-Global-Focus-is-lost-when-returning-to-the-previous-screen # Conflicts: # src/libs/Navigation/TransitionTracker.ts # src/pages/iou/request/step/IOURequestStepMerchant.tsx # src/pages/workspace/rules/SpendRules/SpendRuleCardPage.tsx
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ 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". |
6 participants
Explanation of Change
When a screen-reader user navigates back, focus now returns to the row they tapped to go forward (WCAG 2.4.3). Also fixes WCAG 2.4.7 (Focus Visible) on touch initial-focus and modal close.
Approach. On press, we record the element + its identifier against the screen the user is leaving. On back navigation, after the transition settles, we re-fire accessibility focus on that element. If
react-native-screensdetached the original view, a per-screen registry resolves the live re-mounted one by identifier. Bounded retry budget covers slow re-attach. Sighted users pay nothing the pipeline short-circuits when the screen reader is known-off.InteractionManagerontoTransitionTracker. Also fixes a latent ~1s back-nav stall.goBackskips restore so a re-focused row can't hijack the destination form's Save button.Fixed Issues
$ #77411
PROPOSAL:
Tests
1. Going back returns focus to the item you opened
2. Same behavior inside Workspace settings
3. Opening a screen announces it
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-Native-1.mp4
Android: mWeb Chrome
Android-mWeb-2.mp4
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Mac-Chrome.mp4