focus input on perDiem tab on tab change#71013
Conversation
|
@jayeshmangwani 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] |
| }; | ||
|
|
||
| type IOURequestStepDestinationRef = { |
There was a problem hiding this comment.
Please move this above the IOURequestStepDestinationProps, before its usage.
|
@Eskalifer1 , we previously faced a blocker #70904 because there was a delay in focusing on the amount input. After our PR, there’s now a delay in focusing on the per diem search input. I’m adding a video below to compare the delay between the current version and our PR version. Either we need to fix this delay, or we should explicitly mention in the QA section that the delay is expected, otherwise, we risk running into another deploy blocker. Current prod/staging behaviourstaging-per-diem.movPR behaviourdev-per-diem.mov |
|
@jayeshmangwani I think this is expected behavior, since all tabs that use this approach have this delay. For example, here is the page for creating a new chat: 2025-09-27.13.32.25.mov |
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.
|
|
@Eskalifer1 Please fix the autofocus on this page. When we have multiple per diem workspaces and need to select from the list, the autofocus doesn’t work. no-auto-focus.mov |
|
@jayeshmangwani Yes, thank you for noticing this bug. I suggest changing the approach, because when switching from the Workspace selection screen to the perDiem page itself, the onTabChange function is not called (since we remain on the same tab). Also, we use this component in another place, so if we leave the focus only when changing tabs, there will be no focus in another place. I suggest using the following approach: useFocusEffect(
useCallback(() => {
setTimeout(() => {
InteractionManager.runAfterInteractions(() => {
destinationSelectionListRef.current?.focusTextInput?.();
});
}, 1000);
}, []),
);The advantage of this approach is that regardless of where this component is used, we will focus the input. We can also move this to a separate hook that will add a delay only on iOS, while on all other environments the focus will occur instantly (since the bug we are trying to fix only affects iOS). PS: A 1-second delay is needed to focus the input immediately after changing the tab, because if we try to do it earlier, we will either lose focus or the application will crash. Please write what you think about this, and if this approach suits us, let me know what would be the best name for the hook, as well as which approach you think is better to use (pass a function to it or have it return a ref). |
|
@Eskalifer1 The |
|
I don't think the Also, I couldn't find a single place where we would use this approach for tabs, since the onTabSelect approach is used everywhere, but in this case it won't work for us. cc: @jayeshmangwani |
|
@Eskalifer1 , I think |
|
Hi @jayeshmangwani, Could you please describe in more detail what approach you mean? If it's this part of the code, then setTimeout is also used here, but its value is not suitable for us because it is too small: Lines 108 to 116 in bda97bb |
|
I meant to use the same approach as other pages where the |
|
As far as I know, there is currently no such approach in the project, since Tab usually uses the onTabSelect approach, for example on the NewChatPage page: App/src/pages/NewChatSelectorPage.tsx Lines 41 to 54 in bda97bb However, in this case, this approach is not suitable for us, and a delay of 300 ms will not be enough, since a complete transition between tabs takes more than 300 ms. |
|
I’m on another PR right now, but once I wrap it up, I’ll get back to this PR to move it forward in a few hours. |
|
Hey @Eskalifer1 , can you please update the code and add this change #71013 (comment)? I want to test it, and we can go with this for now. Please use any relevant name for the hook , we can finalize it later. |
|
Hi @jayeshmangwani PR updated! If this option is not suitable for some reason, we can pass here: App/src/components/DestinationPicker.tsx Lines 74 to 85 in 7585b7f
|
|
Thanks for updating the PR! |
|
Hey @Eskalifer1 , I’m testing your latest changes on the web, and I’ve noticed that sometimes the focus doesn’t seem to happen at all. It’s not consistent, it happens occasionally. Check out the video below where I compared the staging and dev environments. Could you try to reproduce this on your end and see if you face the same issue for an account where a few workspaces have per diem enabled? Please also try testing after a fresh login. web-bug.mov |
|
Hi @jayeshmangwani, I believe I understand the issue. This bug is most likely related to So I think we can just replace What do you think about this? Please check it out and let me know. Thank you! |
|
I’m still trying to test things with |
|
@Eskalifer1 @madmax330 , I think we should close this PR and look for new proposals for the issue #67907, since the suggested solution didn’t work during testing #71013 (comment). WDYT? |
|
Hi @jayeshmangwani i proposed solution here to use But if you think this approach is not good enough, then we can close this PR and you can try to find a better solution. |
|
@Eskalifer1 I haven’t seen any usage of |
|
As far as I know (I worked on another PR), a polyfill for In addition, as I mentioned above, As for the video, I can record how it looks if necessary. cc: @jayeshmangwani |
|
please mention the PR here so I can see too |
|
@Eskalifer1 , could you please mention the PR where the |
|
Hi @jayeshmangwani, i merged main, can you please retest if you can still reproduce the bug you wrote about earlier? Because I tried and I can't reproduce it now. |
|
@Eskalifer1 I’m not able to reproduce it on the latest merged main either. But since we’ve deprecated the Also, could you please share the PR where you worked on |
|
@jayeshmangwani I was working on a task where we initially wanted to use I can ask on Slack what to do in such situations if necessary. |
|
Thanks for the update, please tag me on Slack or share the discussion link here so I can follow along. |
|
Hi everyone, seems like we can HOLD this issue for now. Solutions are currently being sought for cases such as this, in the context of the project to remove |
|
Hi @jayeshmangwani, Can you review this PR again please? I updated it because it seems that the issue has been fixed and now we can use |
|
Hi @jayeshmangwani! Bump on review here, conflicts is fixed! |
|
Sorry, missed the ping here, I’ll re-test the PR later today. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid-1.movAndroid-2.movAndroid: mWeb Chromemweb-chrome-2.movmweb-chrome.moviOS: HybridAppios-1.movios-2.moviOS: mWeb Safarimweb-safari-2.movmweb-safari.movMacOS: Chrome / Safariweb-1.movweb-2.mov |
|
✋ 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/madmax330 in version: 9.3.5-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.5-7 🚀
|
Explanation of Change
This PR fixes a bug on iOS native where when switching between tabs while creating an expense in the input, the focus immediately disappears from the perDiem tab.
Fixed Issues
$#67907
PROPOSAL:#67907 (comment)
Tests
Preconditions: Have a workspace with Per Diem enabled.
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
67907-android-native.mov
Android: mWeb Chrome
67907-android-web.mov
iOS: Native
67907-ios-natibe.mov
iOS: mWeb Safari
67907-ios-web.mov
MacOS: Chrome / Safari
67907-web.mov