Repository navigation
Create expense - Keyboard navigation is broken when user add a currency - #104313
Conversation
|
@ikevin127 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-19.at.14.26.46.mov |
| onClose={hidePickerModal} | ||
| onModalHide={hidePickerModal} | ||
| shouldEnableNewFocusManagement | ||
| shouldReturnFocus={!!launcherRef} |
There was a problem hiding this comment.
src/pages/iou/request/step/IOURequestStepCurrencyModal.tsx:57
🟢 shouldReturnFocus used to be false on this modal and is now true, so focus also comes back after a mouse or touch selection. The currency button's onMouseDown calls preventDefault, so if the amount input had focus, the input is still document.activeElement when the trap activates.
The active element takes priority over launcherRef, so focus goes back to the amount input after a currency is picked. On Android mWeb, that programmatic focus can pop the soft keyboard back up right after the picker closes.
Can you check this on Android and iOS mWeb and add videos ? Every platform section in the PR is empty except MacOS.
There was a problem hiding this comment.
@thelullabyy 🟠 This thread got resolved without a reply, and the Android mWeb section in the PR is still empty.
Can you add an Android mWeb video for the touch path: focus the amount input, tap the currency button, pick a currency?
- If focus goes back to the amount input and the keyboard pops up,
shouldReturnFocusneeds to stay off for non-keyboard selections. - If it lands on the currency button with no keyboard, just reply here with the video.
👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
I'm requesting changes for the code-comments and the item below. Working on the checklist in the meantime. ✅ Reviewer checklist completed - tests well. Tag me once addressed 🙌
General
🟢 On the issue, a contributor showed the same lost-focus bug on category, tag, the date dropdown, the date clear (X) button, tax rate, tax amount, distance rate and attendees. You commented that you'd check those.
Please say in the PR whether they'll be fixed here or in a follow-up issue, so this PR doesn't close #96614 while those fields are still broken.
|
Hi @ikevin127, I added the fix, updated PR description to reflect the scope, add new recording,... Kindly help check again |
ikevin127
left a comment
There was a problem hiding this comment.
The code side is close, the PR description still needs work. I posted 2 comments below.
PR description
🟠 Tests and QA step 5, "Verify that User is back and being in the currency field", is unclear and QA will run it word for word. Please change it to "Verify focus returns to the currency button, and pressing Tab moves to the next field".
🟡 State the scope explicitly: currency only, with a follow-up issue for the other fields. Otherwise this closes #96614 while they are still broken.
@thelullabyy Verdict: Ready after these 4 items
- Android mWeb touch-path video, and a reply on the
shouldReturnFocusthread. - Fix Tests and QA step 5.
- State the follow-up for the other fields. This is my call with the internal engineer.
- Type the test mocks from production props.
| onClose={hidePickerModal} | ||
| onModalHide={hidePickerModal} | ||
| shouldEnableNewFocusManagement | ||
| shouldReturnFocus={!!launcherRef} |
There was a problem hiding this comment.
@thelullabyy 🟠 This thread got resolved without a reply, and the Android mWeb section in the PR is still empty.
Can you add an Android mWeb video for the touch path: focus the amount input, tap the currency button, pick a currency?
- If focus goes back to the amount input and the keyboard pops up,
shouldReturnFocusneeds to stay off for non-keyboard selections. - If it lands on the currency button with no keyboard, just reply here with the video.
👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
🟢 LGTM - Thanks for addressing the comments
@thelullabyy Looks like Oxfmt CI is 🔴
lakchote
left a comment
There was a problem hiding this comment.
Thanks @thelullabyy, looks good.
|
🚧 lakchote 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. |
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.5.27-0 🚀
|
|
No help site update needed. This PR is a keyboard-accessibility bug fix that returns focus to the currency button after you pick a currency, which is what users already expect, and no help article describes this focus behavior. |
|
@ikevin127 @thelullabyy This PR is failing because of a regression issue #96614 (comment) |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.27-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Scope: this PR fixes focus return for the currency field only (#96614). Closing the currency picker now returns focus to the currency button instead of dropping it at the top of the form.
The other fields on the confirmation page that have the same focus-return problem are out of scope here and will be handled in a follow-up issue.
Fixed Issues
$ #96614
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.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
Screen.Recording.2026-10-28.at.23.49.44.mov
MacOS: Chrome / Safari
Screen.Recording.2026-10-18.at.20.14.47.mov