Conversation
This fixes Issue Expensify#76921 where focus was lost after navigating back, causing screen readers to announce incorrect elements. The root cause was PR Expensify#49240 which set `setReturnFocus: false` in FocusTrapForScreen to fix Issue Expensify#46109 (blue frame on new tab). This was an over-correction that broke focus restoration during in-app navigation. The fix implements context-aware focus restoration: - Initial page load: Don't restore focus (guards against Expensify#46109) - Navigation back: Restore focus to the previously focused element Changes: - Capture `previouslyFocusedElement` in `onActivate` callback - Track `wasActivatedViaNavigation` based on activeElement state - Implement intelligent `setReturnFocus` that distinguishes scenarios - Add `isElementFocusable` helper for robust element validation - Add comprehensive unit tests (18 test cases) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
@srikarparsi 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] |
mavrickdeveloper
deleted the
fix/76921-focus-restoration-back-navigation
branch
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9681ab168b
ℹ️ 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".
| // Capture the currently focused element BEFORE blurring | ||
| // Only capture if it's a meaningful element (not body) | ||
| if (activeElement && activeElement !== document.body) { | ||
| previouslyFocusedElement.current = activeElement; | ||
| wasActivatedViaNavigation.current = true; | ||
| } else { |
There was a problem hiding this comment.
The navigation detection assumes any document.activeElement that isn’t body represents the element that triggered navigation. On screens that auto-focus an input during mount, focus can move to that input before onActivate runs, so previouslyFocusedElement ends up pointing to an element inside the new screen (not the navigation trigger). When the screen deactivates, setReturnFocus will try to restore focus to that now-unmounted element (or the same triggerElement), so focus won’t return to the original trigger. This breaks the intended “back navigation restores focus” behavior specifically for screens with autoFocus fields; consider ignoring elements inside the trap when capturing, or deriving the trigger from the focus-trap callback instead of document.activeElement.
Useful? React with 👍 / 👎.
mavrickdeveloper
changed the title
1 participant
to be deleted