Fix/65746 Enable scroll for per diem when keyboard is open#66747
Fix/65746 Enable scroll for per diem when keyboard is open#66747marcochavezf merged 10 commits intoExpensify:mainfrom
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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-hybrid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios-hybrid.mp4iOS: mWeb Safariios-mweb.mp4MacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
| if (!isScreenTouched) { | ||
| return; | ||
| } | ||
|
|
||
| if (!shouldHideKeyboardOnScroll) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
These two can be combined (and comment adjusted):
| if (!isScreenTouched) { | |
| return; | |
| } | |
| if (!shouldHideKeyboardOnScroll) { | |
| return; | |
| } | |
| // Only dismiss the keyboard whenever the user scrolls the screen or `shouldHideKeyboardOnScroll` is true | |
| if (!isScreenTouched || !shouldHideKeyboardOnScroll) { | |
| return; | |
| } |
|
@Eskalifer1 🔴 I found a blocker issue while testing on Android / iOS: mWeb where because we hardcode I think we have to somehow dynamically set
🟡 I completed the reviewer checklist but won't be able to approve until the blocker issue is resolved which I'll have to retest. Make sure to test the fix on both native / mweb before deciding to push it, to ensure it doesn't break other logic. |
|
Almost forgot: for the PR description testing section step: |
|
Thanks, I'll take a look on Monday |
|
@ikevin127 By the way, I looked and this bug is reproducible without my changes in the code, here's a video of the staging 2025-07-19.23.59.59.mov |
|
I propose to fix this bug by adding Keyboard.dismiss() in this function, what do you think about it? App/src/pages/iou/request/IOURequestStartPage.tsx Lines 116 to 131 in 71e91c6 cc: @ikevin127 |
That's true, thanks for letting me know. I guess adding Once the change is applied, I'll Approve 🟢 |
|
@Eskalifer1 Thanks for applying the keyboard fix! Looks like there are some failing workflows, let me know once all checks pass so I can review and Approve. |
ikevin127
left a comment
There was a problem hiding this comment.
LGTM ![]()
(failing spell check not related to our PR changes)
|
@ikevin127 Thank you, I think we need to open another task to close the keyboard, because Keyboard.dismiss on onTabSelected is triggered too late for me, so if you quickly type a few characters, the bug will appear. We can leave Keyboard.dismiss as a temporary solution, but it would be nice if this is solved properly. If it's worth it to fix it, of course. |
|
I noticed it's a bit slow and the keyboard is closed after navigation to the other tab is completed, but it's still better than not closing at all. IMO that's fine, as the issue existed previously on staging / production so it shouldn't be reported as a regression of this PR. cc @marcochavezf for context |
|
@ikevin127 Wait a second, I found the regression, I'm working on a fix. On IOS native strage behavior due to shouldEnableKeyboardAvoidingView -.-2025-07-21-.-12.20.26.mp4 |
|
You can edit the PR title and add either |
|
By the way, I noticed another bug, on IOS native when switching between tabs (via the tab button) on the PerDiem page, the keyboard closes as soon as it opens, and this is not related to my changes, I saw the same behavior on the latest main 2025-07-22.14.13.57.movI can also fix it |
|
Let me know what you think, thanks) cc: @ikevin127 |
I think we're good on this one, way out of scope.
That was quite the read, appreciate the effort. I looked into your solution with Strengths: Potential Issues/Improvements:
if (swipeStartIndexRef.current === 3 && currentIndex === 2) {This hardcoded index comparison (3→2 for PerDiem→Distance) is fragile and could break if tab order changes.
@Eskalifer1 To streamline the process and avoid going too much out of scope, given that this mWeb issue is proving to be difficult to address / align with native - I'd say let's simply disable tab navigator swipe on mWeb only for the Per Diem page, this way we avoid the keyboard issue, while maintaining the current logic on native - even though the behaviour will be slightly different. In import { isMobile } from '@libs/Browser';
// We're disabling swipe on mWeb fo the Per Diem tab because the keyboard will hang on the other tab after switching
disableSwipe={(isMultiScanEnabled && selectedTab === CONST.TAB_REQUEST.SCAN) || (selectedTab === CONST.TAB_REQUEST.PER_DIEM && isMobile())}making sure to add that comment for context, this way we avoid the issue of keyboard hanging on the other tab, and if tab is switched by pressing on the button the keyboard is dismissed right away. @marcochavezf If you agree with the fix, I think we can address the issue separately if needed to align the behaviour with native when it comes to this already existing issue on staging / production. ♻️ Once this and the above changes are addressed by the author, and all workflows are passing - I think you can remove the |
|
@ikevin127 Thank you for your update. Just to confirm, we are simply disabling swiping on the Per-diem page, as well as on mobile devices. This means that we are getting rid of the swipe logic between tabs on native devices, which in turn will remove the keyboard bug, and I can report the task of hiding the keyboard on iOS, and it will be a separate task. Correct me if I'm wrong, thank you. |
|
Oh, wait, I think I understand. We don't completely disable swiping on mobile devices, only for the PerDiem page. I think we should correct the condition to disableSwipe={(isMultiScanEnabled && selectedTab === CONST.TAB_REQUEST.SCAN) || (selectedTab === CONST.TAB_REQUEST.SCAN && isMobile())} |
|
Just fo information:
I think that we can search not only by index, but also by tab name, so it's not a big problem.
Yes, i agree
Yes, this is the only problem that remains, because it works differently on native devices and browsers, and this is an internal implementation that we cannot influence. |
|
@Eskalifer1 My bad, just edited the comment, I think it should be: disableSwipe={(isMultiScanEnabled && selectedTab === CONST.TAB_REQUEST.SCAN) || (selectedTab === CONST.TAB_REQUEST.PER_DIEM && isMobile())}to only disable horizontal swipe on the Per Diem page and when isMobile, meaning on mWeb only while on Per Diem page. |
|
I think we are waiting for confirmation regarding the PR, correct? Or Can I start working on the changes now? |
|
I think you're good to move forward and apply this change, don't forget about adding the comment above // We're disabling swipe on mWeb fo the Per Diem tab because the keyboard will hang on the other tab after switchingand change this while you're at it, then once changes are applied CME can take a look and decide whether we will move forward with merge given the context mentioned here. |
|
Okay, thank you, I'll update the PR tomorrow morning. |
|
PR is updated |
|
Thanks, you can remove [HOLD] from PR title now that its ready for review & merge. |
ikevin127
left a comment
There was a problem hiding this comment.
LGTM ![]()
cc @marcochavezf please check out this comment before during review and merge
|
Thanks guys! For the comment regarding disabling the tab navigator swipe on mWeb, could you please bring it to the open-source channel and tag us? 🙏🏽 I'm not sure about this solution, and probably there could be someone else who already had this problem and can point us in the right direction |
|
Hi, i have this situation first time, should i do this or C+? |
|
Posted in Slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1753445102295139 |
|
Thanks for bringing this up in the open-source channel. Probably this is the first time we're experiencing this issue, so let's go with the current solution |
|
✋ 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/marcochavezf in version: 9.1.88-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.1.88-3 🚀
|
| <ScreenWrapper | ||
| includePaddingTop={false} | ||
| keyboardVerticalOffset={variables.contentHeaderHeight + top + variables.tabSelectorButtonHeight + variables.tabSelectorButtonPadding} | ||
| testID={`${IOURequestStepDestination.displayName}-container`} |
There was a problem hiding this comment.
Coming from this issue #72833, we’re wrapping the StepScreenWrapper with ScreenWrapper. However, since StepScreenWrapper has its own offline indicator and ScreenWrapper does too, the offline indicator was appearing twice. We’ve fixed that here: #72833 (comment)
Explanation of Change
Fixed Issues
$#65746
PROPOSAL:#65746 (comment)
Tests
Launch app
Go to workspace settings - more features - enable permission diem
Upload perdiem file attached below
PerDiem-2025-07-19.18_35_01.550.csv
Open workpace chat
Tap plus icon from compose box - create expense - per diem
Enter some text in search box like - sa
Try to scroll the results
Make sure that the result is scrolled and the keyboard is not hidden during scrolling
Offline tests
Precondition: have imported per diem to workspace
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))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
65746-android.mp4
Android: mWeb Chrome
65746-android-web.mp4
iOS: Native
65746-ios.mp4
iOS: mWeb Safari
65746-ios-web.mp4
MacOS: Chrome / Safari
65746-web.mp4
MacOS: Desktop
65746-desktop.mp4