Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.53k stars 2.88k forks source link

[$250] Pressing up arrow to edit the previous message, there is a bad overlap with the recipients local time. #51431

Open m-natarajan opened 2 weeks ago

m-natarajan commented 2 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.53-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @tgolen Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1729781934730029

Action Performed:

  1. Go to staging.new.expensify.com
  2. DM any user and send multiple messages
  3. Click the up arrow on keyboard to edit the last message sent

    Expected Result:

    User able to edit last message sent and no visual issues

    Actual Result:

    Selected message box overlaps with the recipients local time

    Workaround:

    Unknown

    Platforms:

    Which of our officially supported platforms is this issue occurring on?

    • [ ] Android: Standalone
    • [ ] Android: HybridApp
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Standalone
    • [ ] iOS: HybridApp
    • [ ] iOS: mWeb Safari
    • [x] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence ![image (3)](https://github.com/user-attachments/assets/5092447f-dc44-421b-bcba-d87f83ca2b75) ![Google Chrome 2024-10-24 at 10 49 46](https://github.com/user-attachments/assets/d38d1d65-d120-4a0b-bf89-fc1a42377492) ![Snip - (55) New Expensify - Google Chrome (2)](https://github.com/user-attachments/assets/9c8fb52b-4973-4252-8614-1ae0281d7f9f)

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021851399834739628625
  • Upwork Job ID: 1851399834739628625
  • Last Price Increase: 2024-11-05
Issue OwnerCurrent Issue Owner: @Ollyws
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

shahinyan11 commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-10-24 19:35:48 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Pressing up arrow to edit the previous message, there is a bad overlap with the recipients local time.

What is the root cause of that problem?

The message edit mode container is large and the scroll position remains the same after expanding the content.

What changes do you think we should make in order to solve the problem?

Add following line const reportScrollManager = useReportScrollManager() in ComposerWithSuggestions component and add bellow code after this line

InteractionManager.runAfterInteractions(()=>{
    reportScrollManager.scrollToBottom()
})

What alternative solutions did you explore? (Optional)

Add bellow useEffect in ReportActionItem component

useEffect(() => {
    if(!draftMessage) return

    InteractionManager.runAfterInteractions(()=>{
        reportScrollManager.scrollToIndex(index)
    })
}, [draftMessage, index]);

What alternative solutions did you explore? (Optional)

Add bellow useEffect in ReportActionItemMessageEdit component

useEffect(() => {    
    InteractionManager.runAfterInteractions(()=>{
        reportScrollManager.scrollToIndex(index)
    })
}, [index]);

We can also use setTimeout(()=>{}, 0) instead of InteractionManager.runAfterInteractions everywhere

bernhardoj commented 2 weeks ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Pressing the up arrow to edit the last message doesn't show it fully.

What is the root cause of that problem?

In our custom MVCPFlatList, we enable the maintainVisibleContentPosition to maintain the position of the message when the content size changes by scrolling it by the offset of the size differences. But, we also have the mvcpAutoscrollToTopThresholdRef which should auto-scroll to the bottom if it's within the threshold (250). https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/components/FlatList/index.tsx#L124-L131

The issue in this case is, the scroll to the bottom (0) doesn't work and looks like it's conflicting with the edit composer focus. For the scroll to bottom, we set the scroll animated to true (2nd param), which will set the behavior to smooth scrolling. So, the edit composer focus interrupt the scrolling. https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/components/FlatList/index.tsx#L127-L129

If we remove auto-focus from the edit composer, then it works fine, but we can't do that.

What changes do you think we should make in order to solve the problem?

If the mutation is an addition of an edit composer, then don't scroll with animation.

First, adjustForMaintainVisibleContentPosition will accept a new props to decide whether to animate the scroll or not and pass it to the scroll function. https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/components/FlatList/index.tsx#L111 https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/components/FlatList/index.tsx#L129

const adjustForMaintainVisibleContentPosition = useCallback((animated = true) => {
    ...
    scrollToOffset(0, animated, false);

Then, we check if the mutation list contains an added nodes of the edit composer. https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/components/FlatList/index.tsx#L142-L166

let isEditComposerAdded = false;
mutations.forEach((mutation) => {
    ...

    mutation.addedNodes.forEach((node) => {
        if (node.nodeType === Node.ELEMENT_NODE && node.dataset.isEditing === "true") {
            isEditComposerAdded = true;
            return;
        }
    });
});

...

adjustForMaintainVisibleContentPosition(!isEditComposerAdded);
prepareForMaintainVisibleContentPosition();

If an edit composer action is added, then we pass the animated as false. We check whether it's an edit composer action by checking the node.dataset.isEditing === "true" instead of recursively checking the children to make it simpler (let me know if we want to recursively checking the children).

Last, we need to add the isEditing dataset to the element here. https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/pages/home/report/ReportActionItem.tsx#L966

<View style={highlightedBackgroundColorIfNeeded} dataSet={{isEditing: !!draftMessage}}>

What alternative solutions did you explore? (Optional)

We have a few alternatives here.

1. Delay the auto scroll to bottom here using InteractionManager, let the focus happens first. https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/components/FlatList/index.tsx#L128-L130

2. Delay the focus using InteractionManager, https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/libs/focusComposerWithDelay/index.ts#L30-L38

OR we can wait for the scroll event to ends, but the scroll end event actually just use setTimeout 250ms after there is no onScroll event received anymore. https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/components/InvertedFlatList/index.tsx#L75-L83

3. We can manually scroll when the edit composer is focused, but reportScrollManager.scrollToIndex is currently only made for native because in web, we don't want to scroll the edit composer to bottom when editing it. https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/pages/home/report/ReportActionItemMessageEdit.tsx#L521-L530 https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/hooks/useReportScrollManager/index.ts#L11-L18

So, what we can do is to only scroll if it's the last action.

reportScrollManager.scrollToIndex(index, isLastVisibleReportAction);

We can get the last visible report action inside getFirstVisibleReportActionID. We will rename it to getFirstAndLastVisibleReportActionID https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/pages/home/report/ReportActionsList.tsx#L452

function getFirstAndLastVisibleReportActionID(sortedReportActions: ReportAction[] = [], isOffline = false): [string, string] {
    if (!Array.isArray(sortedReportActions)) {
        return ['', ''];
    }
    const sortedFilterReportActions = sortedReportActions.filter((action) => !isDeletedAction(action) || (action?.childVisibleActionCount ?? 0) > 0 || isOffline);
    return sortedFilterReportActions.length > 1 ? [
        sortedFilterReportActions.at(sortedFilterReportActions.length - 2)?.reportActionID ?? '-1',
        sortedFilterReportActions.at(0)?.reportActionID ?? '-1',
    ] : ['', ''];
}

Then, pass isLastVisibleReportAction just like isFirstVisibleReportAction https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/pages/home/report/ReportActionsList.tsx#L531-L532

If we want to always scroll the edit composer to bottom (specifically to put it above the main composer), then we can remove the isEditing here. https://github.com/Expensify/App/blob/66112d13bd47ed3d9b029e77b8d494e13808a594/src/hooks/useReportScrollManager/index.ts#L11-L17

adelekennedy commented 1 week ago

I think this issue may be a dupe (thank you @shahinyan11!) closing in favor if this one as it already has a proposal

raza-ak commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Pressing up arrow to edit the previous message, there is a bad overlap with the recipients local time.

What is the root cause of that problem?

Enabling edit mode triggers a slight upward movement of the scrollbar, which shifts the layout and results in an overlap between the message editing area and the recipient's local time display.

What changes do you think we should make in order to solve the problem?

A useEffect hook was implemented to automatically scroll to the bottom after the component finishes rendering in edit mode. This ensures that the edit section is fully visible and prevents any overlap with the recipient’s local time. Following changes will fix the issue:

image

Video:

https://github.com/user-attachments/assets/41995382-6b30-41f2-9dc3-c9e530241ed9

melvin-bot[bot] commented 1 week ago

@strepanier03 Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~021851399834739628625

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Ollyws (External)

strepanier03 commented 1 week ago

Repro'd pretty easily. This is mostly just a polish project as it doesn't impact the customer's ability to use the product nor drive forward other projects.

image
melvin-bot[bot] commented 3 days ago

@strepanier03, @Ollyws Huh... This is 4 days overdue. Who can take care of this?

Ollyws commented 3 days ago

Will get to this one tomorrow morning.

melvin-bot[bot] commented 2 days ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Ollyws commented 2 days ago

@bernhardoj's proposal LGTM. 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 days ago

Triggered auto assignment to @rlinoz, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

bernhardoj commented 2 days ago

PR is ready

cc: @Ollyws