-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Report scroll adjustment #66240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Report scroll adjustment #66240
Conversation
…scroll-adjustment
…scroll-adjustment
|
@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] |
|
@shubham1206agra I replaced FlatList with our custom InvertedFlatList for better chat-like scrolling behavior. So now it uses the same scroll element across the fresh report and regular chat |
Screen.Recording.2025-07-29.at.5.44.03.PM.mov@perunt The comment is getting hidden behind menu. |
|
@shubham1206agra since @perunt is OOO right now, i'll try to give this PR a look within the next few days. |
…scroll-adjustment
|
I'm back on this. I see that this issue reproduces with the closed keyboard only. Now I see that this happens since we have messages positioned at the top and not at the bottom as in the rest of the app. Will fix it |
|
@shubham1206agra, now it works as expected |
Screen.Recording.2025-08-12.at.9.07.23.PM.mov@perunt BUG: Sometimes, I am unable to scroll to the bottom in mWeb Safari Edit - This is repro on main |
|
@chrispader the suggestion behaves correctly (e.g. always on top, aligned above the composer) |
|
@trjExpensify @mountiny @shubham1206agra in this case, are we good to continue with the review and merge of this PR? |
Screen.Recording.2025-11-03.at.7.17.26.PM.movThe problem with the PR is that the impact of changes goes beyond the flow we are trying to address, and now we can see such bugs, like missing keyboard animation, inconsistent padding when opening modal in different cases, etc. So, my suggestion is to revert #54764 safely before doing anything (I know it's difficult, but we should create an issue for this first and hold this PR on that) cc @mountiny |
|
I just think we should mimic how it works in Slack, that is sometimes the content also moves down when the keyboard closes under the bottom sheet modal which avoids the jumps in the video from Shubham |
|
I think it would make sense to try to revert the changes from the Kiryls Pr if we agree we would like to align more on the Slack keyboard behaviour which imho is very smooth and good UI/ UX |
|
cc @trjExpensify @Expensify/design for buddy check, not sure if this would need bigger slack post or no |
|
From my end (UX), the content underneath the modal shouldn't be altered in any way. The problem that I see is that RN doesn't give you the stuff that native gives you for free (which is what the Slack video shows). We have a lack of animations in the video above but the behavior in the video from a positioning standpoint isn't bad, it's just the animation and lag imo. |
I agree, I don't know or recall why that was done. 👍 |
|
Agreed. I don't recall why/when that was done, nor do I think the content underneath the modal should be altered anyway. |
|
I'm with all of you as well. |
|
Thanks for chiming in everyone, so the next steps would be that @chrispader would try to revert the PR from Kiryl. Then we could adjust this PR to the desired behaviour |
|
@chrispader are you actively working on that? What are the next steps? |
|
I'm working on that atm. Seems the next step would be to try to revert the original PR. I'll try to start tomorrow morning |
|
After reviewing Slack's behavior and comparing it with our current implementation, I think we should align with Slack's approach instead of introducing a separate inline TextField inside the message list. Right now, Expensify replaces the message with a second TextField when editing. That feels unintuitive, adds extra UI complexity, and results in odd interactions when the keyboard + composer overlap the editable area. Slack keeps the message list unchanged and simply reuses the main composer for editing. The edited message appears above the composer, and the content shifts only when editing is active. This flow feels simpler, more predictable, and avoids layout issues. If we want a consistent UX, I’d suggest we switch to the same strategy:
cc @chrispader ScreenRecording_11-17-2025.15-31-52_1.MP4 |
|
cc @Expensify/design |
|
If we want, we could still move the currently edited message into view. Slack doesn't do it, but i'm not sure what i personally prefer in terms of UX. Re-using the composer for editing would definitely make handling layout much easier and removes all sorts of bugs, like layout shifts or janky behavior, when the state for editing messages changes at different times for different parts of the UI. |
I agree, this is not really a good practice. I don't think there is a use-case where a user would need to edit multiple messages at once, rather than consecutively @dannymcclain if we go for this approach, do you think you could sketch some designs for the composer in the new "editing message" state? |
I feel like we could make it look identical to the current editing style, right? Just instead of being inline with the message you're editing, it would be at the bottom (where the composer always lives). |
Totally fair. I'm good with this approach as well. 👍 |
Yes, i see that they do the inline message editing on web/desktop, where we usually have more screen real estate. On native, it feels more intuitive to re-use the composer for editing messages. I think ideally, we would have inline message editing on web/desktop and on native we would edit a message through the composer, but i understand that this would add additional complexity and would require a major refactor of the composer component. Personally, i would prefer just using the regular composer for all, which makes all the scrolling/positioning easier and also prevents the issue with multiple active 'editing inputs' mentioned by @dannymcclain |
|
I am afraid this has been extensively discussed in the past and I believe we wanted to keep the multiple edit composers in place. I dont think we should just change this without P/S and broader discussion in slack also given the cross paltform philosophy so it should behave same on web and on native. What is holding us back from reverting that original PR behaviour and proceeding with the UX we agreed on without changing this, it feels like separate topic to me and slows us down |
|
@chrispader @Kureev Can we close this PR? Are we proceeding in different branch? |
@mountiny yes, i think this can be closed as we're continuing to work on this in #76741 |


Explanation of Change
Fixed Issues
$ #64085 (comment)
PROPOSAL:
Tests
Offline tests
QA Steps
Precondition:
// TODO: These must be filled out, or the issue title must include "[No QA]."
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.npm run compress-svg)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-07-16.at.14.38.09.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop