[$500] Chat - Scrolling down on edit composer with multi lines scrolls up the chat history

[kbecciv]

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: 1.3.96.0
Reproducible in staging?: y
Reproducible in production?: y
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: Applause - Internal Team
Slack conversation:

Issue found when executing PR #30857

Action Performed:

1. Navigate to staging.new.expensify.com.
2. Open chat with chat history that is enough to fill a page.
3. Send a message with 8 multilines.
4. Hover the cursor over the edit composer.
5. Scroll down on the edit composer (swipe down on the touchpad).
6. Keep scrolling down.

Expected Result:

When scrolling down on the edit composer, the chat view will not be scrolled.

Actual Result:

When scrolling down on the edit composer, the chat view scrolls up. When scrolled to the top of the chat, user is unable to reach the latest message with the edit view as it is scrolling back up.

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6267210_1699356584644.20231107_131212.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014595e5e6d6d7b727
• Upwork Job ID: 1721862626579214336
• Last Price Increase: 2023-11-07

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[bernhardoj]

What a coincidence that I just reported this scroll issue a few hours ago on the slack channel 😄

Proposal

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

Scrolling on an edit composer will reverse-scroll the chat pages.

What is the root cause of that problem?

This is a regression from #27429. In that PR, we want to fix a [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event warning.

App/src/components/Composer/index.js

Line 388 in 8d6fc37

textInput.current.addEventListener('wheel', handleWheel, supportsPassive ? {passive: true} : false);

The warning told us to pass passive as true to the event listener. Before that PR, scrolling inside an edit composer won't scroll the chat pages, even when it reaches the end by handling the scroll manually and call event.preventDefault.

App/src/components/Composer/index.js

Lines 334 to 345 in 8d6fc37

	/**
	* Manually scrolls the text input, then prevents the event from being passed up to the parent.
	* @param {Object} event native Event
	*/
	const handleWheel = useCallback((event) => {
	if (event.target !== document.activeElement) {
	return;
	}
	textInput.current.scrollTop += event.deltaY;
	event.stopPropagation();
	}, []);

Notice the comment, and that the event.preventDefault is not there anymore. It's removed because when we pass passive as true, we shouldn't call preventDefault anymore.

For some reason, passing the passive as true and calling event.stopPropagation reverse-scrolls the chat pages when scrolling inside an edit composer.

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

Because we already remove the event.preventDefault, we don't need the manual wheel/scroll handle anymore (handleWheel).

What alternative solutions did you explore? (Optional)

If we want the
scrolling inside an edit composer won't scroll the chat pages, even when it reaches the end
feature back, then we should remove the passive from the event listener and add back event.preventDefault. However, this will bring back the [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event` warning

[akinwale]

@bernhardoj I tested your proposed solution but the composer exhibits the same behaviour in reverse. Scrolling down to the end doesn't scroll the report, but scrolling up does (even when the composer content is scrollable, ie. 15+ lines or more).

[suneox]

Proposal

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

Chat - Scrolling down on edit composer with multi lines scrolls up the chat history

What is the root cause of that problem?

The input handle wheel to correct scroll top but when hover on input and start scroll over input the parent also receive an event

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

To avoid scroll over text input we can add styles.overscrollBehaviorContain into inputStyleMemo to make sure only scroll inside the input

    const inputStyleMemo = useMemo(
        () => [
            // We are hiding the scrollbar to prevent it from reducing the text input width,
            // so we can get the correct scroll height while calculating the number of lines.
            numberOfLines < maxLines ? styles.overflowHidden : {},

            StyleSheet.flatten([style, {outline: 'none'}]),
            StyleUtils.getComposeTextAreaPadding(numberOfLines, isComposerFullSize),
            Browser.isMobileSafari() || Browser.isSafari() ? styles.rtlTextRenderForSafari : {},
+           styles.overscrollBehaviorContain,
        ],
        [style, maxLines, numberOfLines, isComposerFullSize],
    );

and disable handleWheel when numberOfLines < maxLines at due to text area not scrollable so func handleWheel will not working

+   const hasScroll = useMemo(() => numberOfLines > maxLines, [numberOfLines, maxLines]);

    useEffect(() => {
        ......
        if (textInput.current) {
            document.addEventListener('paste', handlePaste);
+           if (hasScroll) { 
                textInput.current.addEventListener('wheel', handleWheel, supportsPassive ? {passive: true} : false);
+           }
        }
        ......
    }, [hasScroll]);

POC

30987.mp4

What alternative solutions did you explore? (Optional)

Add styles.overscrollBehaviorContain and remove handleWheel

[HardikChoudhary24]

Proposal

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

• Scrolling down on edit composer with multi lines scrolls up the chat history

What is the root cause of that problem?

• The root cause of this issue is that we are using passive:true and thus we are not calling preventDefault which leads to the opposite scrolling behavior.

App/src/components/Composer/index.js

Line 388 in 0cb9969

textInput.current.addEventListener('wheel', handleWheel, supportsPassive ? {passive: true} : false);

App/src/components/Composer/index.js

Lines 338 to 345 in 0cb9969

	const handleWheel = useCallback((event) => {
	if (event.target !== document.activeElement) {
	return;
	}
	textInput.current.scrollTop += event.deltaY;
	event.stopPropagation();
	}, []);

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

• To address this problem, we can make passive:false and then call prevent.Default

  const handleWheel = useCallback((event) => {
        if (event.target !== document.activeElement) {
            return;
        }

        event.preventDefault();

        textInput.current.scrollTop += event.deltaY;
        event.stopPropagation();
    }, []);

   textInput.current.addEventListener('wheel', handleWheel,{passive: false});
   

Result

new.mov

What alternative solutions did you explore? (Optional)
N/A

[bernhardoj]

@akinwale it works fine on me.

Screen.Recording.2023-11-09.at.08.26.49.mov

For my main solution, you need to remove the wheel listener

App/src/components/Composer/index.js

Line 388 in dee9b31

textInput.current.addEventListener('wheel', handleWheel, supportsPassive ? {passive: true} : false);

[akinwale]

@bernhardoj Thanks. I retested and it works as expected.

@bernhardoj's proposal correctly identifies the root cause and the proposed solution is simple and adequate. Since it is the first proposal that fixes the problem, we can move forward with it.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[bernhardoj]

PR is ready
cc: @eVoloshchak

[akinwale]

@zanyrenney Automation seems to have missed this issue.

The PR was deployed to production on 2023-11-22, so payment should have been due on 2023-11-29. It's now overdue by 1 week.

Thanks.

[tgolen]

bump @zanyrenney for the above ^

[zanyrenney]

payment summary

$0 reporting bonus - done by applause
$500 - c @bernhardoj [CAN YOU PLEASE APPLY TO UPWORK JOB FOR PAYOUT]
$500 - c+ @akinwale PAID

[bernhardoj]

@zanyrenney the job is no longer available

[bernhardoj]

@zanyrenney hi, can you please check the upwork job?

[zanyrenney]

Ugh yep now its closed. I'll recreate for you @bernhardoj please apply ASAP, I am OOO from tomorrow.

[zanyrenney]

@bernhardoj

offer is here, please accept.

[bernhardoj]

@zanyrenney accepted. Thanks!

[zanyrenney]

$500 - c @bernhardoj PAID