Fix: Search field not highlighted when auto-focused after switching from Scan to Manual#94514
Conversation
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 I looked into the failing checks — there's no code/test failure to fix here. The only failing check is PR Author Checklist / checklist, which fails because the human-only Every code-quality and functional check passed:
The Action needed from you: fill out the |
|
@MelvinBot Please merge main. After that replace all |
…ghlightSelectionList Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@thelullabyy 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] |
|
🤖 Done, @thelullabyy:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // even though the caret is in the field. Listening to the native focus/blur events directly | ||
| // on the element — plus reconciling against the current `activeElement` in case focus already | ||
| // landed before this effect attached its listeners — makes the visual state authoritative. | ||
| // See https://github.com/Expensify/App/issues/94233 |
There was a problem hiding this comment.
❌ PERF-14 (docs)
This new useEffect manually subscribes to an external store (the DOM element's native focus/blur events) and mirrors that external value directly into React state via setIsFocused(true)/setIsFocused(false). It follows the exact subscribe-in-setup / unsubscribe-in-cleanup / setState(externalValue)-in-listener shape that useSyncExternalStore is purpose-built for. The manual useEffect + setState approach is more error-prone (it misses tearing guarantees under concurrent rendering and adds an extra render pass) and the document.activeElement reconciliation on mount is exactly the kind of "read the current snapshot" logic getSnapshot handles for you.
Prefer reading this focus state through useSyncExternalStore, where subscribe registers the focus/blur listeners and getSnapshot reads the authoritative DOM state:
const subscribeFocus = useCallback(
(callback: () => void) => {
const inputElement = input.current;
if (!inputElement) {
return () => {};
}
inputElement.addEventListener('focus', callback);
inputElement.addEventListener('blur', callback);
return () => {
inputElement.removeEventListener('focus', callback);
inputElement.removeEventListener('blur', callback);
};
},
[],
);
const isDomFocused = useSyncExternalStore(
subscribeFocus,
() => typeof document !== 'undefined' && document.activeElement === input.current,
() => false,
);This makes the subscription contract explicit, removes the redundant isFocused mirror state, and handles the initial-snapshot reconciliation without a separate setState on mount.
Reviewed at: d767aea | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good call — applied in 3390ac3.
The manual useEffect + setState focus-mirroring (and the redundant setIsFocused calls in onFocus/onBlur) are now collapsed into a single useSyncExternalStore: subscribe registers the native focus/blur listeners and getSnapshot reads document.activeElement === input.current. This removes the mirror state and the extra render pass while keeping the original remount-race fix (the snapshot reconciles focus that landed before we subscribed). See BaseTextInput/implementation/index.tsx:142-167.
Note on React Compiler: reading input.current inside getSnapshot is fine here — it's a deferred callback, not a render-time ref read, so react-compiler-compliance-check shows no new violations (the file's pre-existing count is unchanged).
Verified locally: lint-changed, typecheck, react-compiler compliance (no regression), and the TextInputLabel + useSelectionListTextInput suites all pass. 👍
|
Please check this comment #94514 (comment) |
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@puneetlath 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] |
|
@thelullabyy let me know when you're ready for me on this one. |
|
@puneetlath I'm working on the rest fixes today... I think all of them can be done before Monday |
|
Awesome! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-27.at.01.50.15.movScreen.Recording.2026-06-27.at.01.52.02.mov |
|
@MelvinBot You are using the wrong template for PR description. Let's update to use this https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md Remember to replace all |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 puneetlath 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.25-0 🚀
|
|
🤖 No help site changes required. This PR is a purely internal frontend rendering fix. It changes how There is no change to any user-facing feature, workspace setting, UI label, navigation, or documented workflow, so nothing under Why this doesn't touch the help site
@thelullabyy, no linked help site PR was created because no documentation changes are needed for this fix. If you believe a help article should be updated for something related, let me know and I'll create one. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.25-2 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
On the Scan → Manual expense flow (for an account with no workspace), the participant search field ("Name, email, or phone number") receives the caret but shows no focus border and the label never floats, with the cursor overlapping the field title.
The field is focused imperatively via a ref (
SelectionList'ssetTimeout(focusTextInput, …)) while the start page is mid‑remount. The.focus()call lands on the input, but the ReactonFocussynthetic event isn't delivered during the remount, soBaseTextInput's internalisFocusedstate staysfalse. Since both the focus border (isFocused && styles.borderColorFocus) and the label float are gated entirely on that state, the field looks unfocused even though the caret is in it.This makes
isFocusedauthoritative instead of trusting that the ReactonFocus/onBlurevents always arrive:BaseTextInput(web) now listens to the nativefocus/blurDOM events directly on the input element, and reconciles againstdocument.activeElementon mount in case focus already landed before the listeners attached. The border and label-float then reconcile regardless of which focus path (theautoFocusprop or an imperative.focus()) was used, and regardless of the remount timing race.The change is web-only (the reported symptom is MacOS/Chrome), additive (it doesn't change the ref shape or remove the existing
onFocus/onBlurhandlers), and idempotent with the existing handlers.Fixed Issues
$ #94233
PROPOSAL: #94233 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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