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.58k stars 2.92k forks source link

[$250] mWeb/Chrome - Chat - Edition composer is cutted when editing multiline message near the top of screen #52138

Open lanitochka17 opened 3 weeks ago

lanitochka17 commented 3 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.58-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5184687&group_by=cases:section_id&group_order=asc&group_id=292106 Issue reported by: Applause - Internal Team

Action Performed:

  1. Open the staging.new.expensify.com website
  2. Open any chat with long history
  3. Long tap on a multiline message displayed near the top of the screen
  4. Select "Edit Message"
  5. Verify that the chat is scrolled up and edition composer is fully visible

Expected Result:

When trying to edit a multiline message displayed near the top of the screen, the chat should be scrolled up to make the composer fully visible

Actual Result:

When trying to edit a multiline message displayed near the top of the screen, the chat is not scrolled up when the keyboard is opened and the edition composer gets cutted off by the chat header

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/b77258f0-424b-4807-88dd-153853c50266

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021857151760601711832
  • Upwork Job ID: 1857151760601711832
  • Last Price Increase: 2024-11-28
Issue OwnerCurrent Issue Owner: @thesahindia
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @abekkala (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.

melvin-bot[bot] commented 2 weeks ago

@abekkala Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 weeks ago

@abekkala 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

abekkala commented 2 weeks ago

confirmed the composer should not be partially hidden by the header

melvin-bot[bot] commented 1 week ago

@abekkala, @thesahindia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

abekkala commented 1 week ago

Waiting for proposal

melvin-bot[bot] commented 1 week ago

@abekkala @thesahindia this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 week ago

@abekkala, @thesahindia Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

@abekkala, @thesahindia Still overdue 6 days?! Let's take care of this!

wildan-m commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-11-28 16:03:30 UTC.

Proposal

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

The mWeb/Chrome Chat Edition composer is cut off when editing a multiline message near the top of the screen.

What is the root cause of that problem?

The scroll pushed by soft input keyboard and in some cases it will cut the editor.

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

I think we need to scroll to the edited report action before we perform editing. This way we can ensure that the edited message is near the top of the soft input keyboard

In src/pages/home/report/ReportActionItem.tsx modify this use effect to add scrollToIndex


    useEffect(() => {
        if (prevDraftMessage !== undefined || draftMessage === undefined) {
            return;
        }

        InteractionManager.runAfterInteractions(() => {
            requestAnimationFrame(() => {
                reportScrollManager.scrollToIndex(index);
            });
        })

        focusComposerWithDelay(textInputRef.current)(true);
    }, [prevDraftMessage, draftMessage]);

Optional: Limit to Browser.isMobileChrome or Browser.isMobile or canUseTouchScreen if we don't want this behavior on other platforms

in useReportScrollManager make animated as param with default to true


    /**
     * Scroll to the provided index. On non-native implementations we do not want to scroll when we are scrolling because
     */
    const scrollToIndex = useCallback(
        (index: number, isEditing?: boolean, animated: boolean = true) => {
            if (!flatListRef?.current || isEditing) {
                return;
            }

            flatListRef.current.scrollToIndex({index, animated});
        },
        [flatListRef],
    );

And use false value in the use effect


    useEffect(() => {
        if (prevDraftMessage !== undefined || draftMessage === undefined) {
            return;
        }

        InteractionManager.runAfterInteractions(() => {
            requestAnimationFrame(() => {
                reportScrollManager.scrollToIndex(index, false, false);
            });
        })

        focusComposerWithDelay(textInputRef.current)(true);
    }, [prevDraftMessage, draftMessage]);

Branch for this solution

What alternative solutions did you explore? (Optional)

We can modify ReportActionItemMessageEdit to also scroll the reportActionItem for mWeb

src/pages/home/report/ReportActionItemMessageEdit.tsx


                            onFocus={() => {
                                setIsFocused(true);
                                if (textInputRef.current) {
                                    ReportActionComposeFocusManager.editComposerRef.current = textInputRef.current;
                                }
                               const shouldScrollOnEdit = canUseTouchScreen() ? isFocused && prevIsFocused === isFocused : true;
                                InteractionManager.runAfterInteractions(() => {
                                    requestAnimationFrame(() => {
                                        reportScrollManager.scrollToIndex(index, shouldScrollOnEdit );
                                    });
                                });

The shouldScrollOnEdit can be one of these:

Logic Behavior
canUseTouchScreen() ? isFocused && prevIsFocused === isFocused : true; it scroll when it first time receiving focus, it will not only work when pressing edit but also tapping to other opened ReportActionItemMessageEdit
!canUseTouchScreen() will be enabled for mweb

canUseTouchScreen can be altered with isSmallScreen / Browser.isMobile / Browser.isMobileChrome

wildan-m commented 1 week ago

Proposal Updated

melvin-bot[bot] commented 5 days ago

@abekkala, @thesahindia 10 days overdue. Is anyone even seeing these? Hello?

abekkala commented 5 days ago

@thesahindia can you take a look at the updated proposal from @wildan-m

thesahindia commented 4 days ago

I couldn’t get to it today. I’ll review it first thing when I get to work.

thesahindia commented 3 days ago

@wildan-m, it doesn't look good.

https://github.com/user-attachments/assets/8ce20ac5-d6c3-4bc9-b79f-7ebe857af58b

melvin-bot[bot] commented 3 days ago

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

wildan-m commented 3 days ago

Proposal Updated

@thesahindia what about disabling the scroll animation?

thesahindia commented 3 days ago

Could you please share a video? I am still getting the same behaviour.

wildan-m commented 2 days ago

Proposal Updated

it doesn't look good.

@thesahindia if we check this PR https://github.com/Expensify/App/pull/2320 and this code that behavior is actually implemented but only for native. I think we should also implement the behavior for mweb