[CP Staging] fix: prevent focus-return side effects on confirmation-field save and SelectionList RHP close - #90869
Conversation
…oesn't scroll-jump SelectionList
|
@shubham1206agra 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 |
![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: ec1506c27a
ℹ️ 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❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…ore so keyboard Tab still syncs cursor
…ing only the scroll
|
Double checked it, and addressed it. Thanks! |
|
@codex review |
PR ReviewOverall: This is a clean, well-structured fix for three deploy blockers sharing one root cause. The approach of exposing What's good
One design question (non-blocking)
Minor test coverage note (non-blocking)No test verifies jest.spyOn(trigger, 'focus').mockImplementation(() => { throw new Error('boom'); });
expect(() => restoreTriggerForRoute('route-a')).toThrow();
expect(isFocusRestoreInProgress()).toBe(false);No blocking issues found. The fix is minimal, well-tested, and correctly addresses all three deploy blockers. |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ 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". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb Safarimweb.movMacOS: Chrome / Safariweb.mov |
|
Blue frame shows when close RHP using Esc key. This doesn't happen on production. blue.frame.mov |
|
@MelvinBot investigate this. Repro step:
Why doesn't this happen on other tabs like Spend > Reports, Spend > Drafts, etc but only Spend > Expenses tab? 
|
|
I reviewed that. It's safe minor refactor. @MelvinBot double confirm |
|
I reviewed commit What it does: Extracts the duplicated "set focused index + suppress scroll" pattern into a shared helper Why it's safe:
No logic changes, no new behavior paths, single file touched. Confirmed safe. 👍 |
MonilBhavsar
changed the title
Beamanator
left a comment
Beamanator
left a comment
There was a problem hiding this comment.
looking pretty good here!
|
🚧 @MonilBhavsar 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. |
…ges-The-focus-is-lost-and-does-not-return-to-triggering-element [CP Staging] fix: prevent focus-return side effects on confirmation-field save and SelectionList RHP close (cherry picked from commit 161d0ba) (cherry-picked to staging by roryabraham)
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.3.75-7 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. The changes are purely internal UI bug fixes for keyboard focus-return behavior (preventing scroll jumps, row highlights, and Enter key conflicts after back navigation). No user-facing features, workflows, settings, or terminology are affected. |
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.3.75-10 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This PR fixes internal keyboard focus/scroll side effects ( |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.75-14 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.3.76-0 🚀
|
|
I reviewed all the changes in this PR. The modifications are purely internal bug fixes to keyboard focus-return behavior across
None of these changes affect user-facing features, workflows, settings names, or terminology that would be documented in the help site. No changes are required to Expensify's help site files under |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.77-3 🚀
|
7 participants
Explanation of Change
#87852 restores keyboard focus to the element you came from on back navigation. That programmatic
.focus()is the right thing for accessibility, but three places were mistaking it for the user actually navigating there by keyboard — three deploy blockers, one root cause.To tell a real focus from the restore,
NavigationFocusReturnnow exposesisFocusRestoreInProgress(), true only for the synchronous momentrestoreTriggerForRoutecalls.focus(). Lists check it in theironFocushandlers.#90838 — Merchant: pressing Enter again re-opens Merchant instead of creating the expense.
Saving the field with Enter returns to the confirm page and focus lands back on the Merchant row. Because that row is a button,
Button.shouldDisableEnterShortcutswitches off the page's "Create expense" Enter shortcut, so the next Enter just re-opens Merchant. Fix: a one-shotskipNextFocusRestore()that the merchant screen sets right before its save-and-go-back. Back and Esc don't set it, so normal dismissals still restore focus (WCAG intact).#90839 — Categories: the list scroll-jumps after closing the details RHP.
The restored row's
onFocusmoved the SelectionList's arrow cursor, which scrolled the list to it. Fix: on a focus-return restore the cursor still tracks the restored row (so keyboard nav continues from the right place), but the scroll that move would trigger is suppressed.#90845 — Spend: the expense stays highlighted after closing the RHP.
Same
onFocuspath in the Search list, where the cursor is the row highlight — so the opened expense stayed lit up and scrolled into view. Fix: the Search list ignores the restore for cursor purposes; focus is still restored for screen readers, it just doesn't paint the keyboard highlight.Fixed Issues
$ #90838
$ #90839
$ #90845
PROPOSAL:
Tests
[#90838] Confirmation field save → Enter creates the expense
[#90839] Category details RHP close → no scroll jump
[#90845] Spend expense RHP close → row not highlighted
All steps verified on web (dev); the three blockers are fixed with no regression to focus-return, keyboard arrow/Tab navigation, or mouse interaction.
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