[HOLD for payment 2024-08-09] [$250] Distance - Page is not scrollable after adding multiple stops

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.75-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: https://expensify.testrail.io/index.php?/tests/view/4572868
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Click on FAB > Submit expense > Distance tab
2. Add a start waypoint, a finish waypoint, and multiple stops
3. Try to Scroll the page
4. Do step 1 - 3 on mWeb

Expected Result:

Page becomes scrollable to view the whole content after adding multiple waypoints (Same behavior as in mWeb)

Actual Result:

Page is not scrollable to view the whole content after adding multiple waypoints which is a different behavior compared to mWeb.

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

Bug6490254_1716535967530.Screen_Recording_20240524_102844_New_Expensify.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014d91998739178ee2
• Upwork Job ID: 1795722148596056064
• Last Price Increase: 2024-06-05
• Automatic offers:
• Krishna2323 | Contributor | 102695130

Issue Owner

Current Issue Owner: @stephanieelliott

[melvin-bot]

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

[izarutskaya]

We think this issue might be related to the #collect project.

[Krishna2323]

Proposal

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

Distance - Page is not scrollable after adding multiple stops

What is the root cause of that problem?

The footer (map) for android is rendered separately from the DraggableFlatList (not as a footer of the list) and DraggableFlatList and ListFooterComponent is wrapped in a View instead of ScrollView.

App/src/components/DraggableList/index.android.tsx

Lines 11 to 21 in 0cb2dc0

	<View style={styles.flex1}>
	<DraggableFlatList
	ref={ref}
	containerStyle={ListFooterComponent ? undefined : styles.flex1}
	contentContainerStyle={ListFooterComponent ? undefined : styles.flexGrow1}
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...viewProps}
	/>
	{React.isValidElement(ListFooterComponent) && <View style={styles.flexGrow1}>{ListFooterComponent}</View>}
	</View>
	);

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

We can use NestableScrollContainer & NestableDraggableFlatList.

        <NestableScrollContainer
            ref={ref}
            contentContainerStyle={styles.flexGrow1}
        >
            <NestableDraggableFlatList
                contentContainerStyle={styles.flex1}
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...viewProps}
            />
            {React.isValidElement(ListFooterComponent) && <View style={styles.flexGrow1}>{ListFooterComponent}</View>}
        </NestableScrollContainer>

There will be a console error when we will use NestableDraggableFlatList.tsx and thats because of this code using findNodeHandle(scrollableRef.current); instead of passing scrollableRef.current directly to ontainerRef.current.measureLayout. It is also mentioned here. We can open an issue/PR to update that in react-native-draggable-flatlist.

Result

scroll_issue_distance_page.mp4

Alternatives

What alternative solutions did you explore? (Optional)

1. Remove the view and wrap

            <ScrollView style={styles.flex1}>
                <DraggableList
                    data={waypointsList}
                    keyExtractor={(item) => item}
                    shouldUsePortal
                    onDragEnd={updateWaypoints}
                    ref={scrollViewRef}
                    renderItem={renderItem}
                    ListFooterComponent={
                        <DistanceRequestFooter
                            waypoints={waypoints}
                            navigateToWaypointEditPage={navigateToWaypointEditPage}
                            transaction={transaction}
                        />
                    }
                />
            </ScrollView>

2. This only happens on android, so we can in the android file only, We can't use the ListFooterComponent because it was removed intentionally to solve [HOLD for payment 2024-05-20] CRITICAL: [P2P Distance] [$500] Enable immediate slide scrolling without tap #40211.

App/src/components/DraggableList/index.android.tsx

Lines 11 to 20 in 525ad6f

	<View style={styles.flex1}>
	<DraggableFlatList
	ref={ref}
	containerStyle={ListFooterComponent ? undefined : styles.flex1}
	contentContainerStyle={ListFooterComponent ? undefined : styles.flexGrow1}
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...viewProps}
	/>
	{React.isValidElement(ListFooterComponent) && <View style={styles.flexGrow1}>{ListFooterComponent}</View>}
	</View>

To:

        <ScrollView contentContainerStyle={{flexGrow: 1}}>
            <DraggableFlatList
                ref={ref}
                containerStyle={ListFooterComponent ? undefined : styles.flex1}
                contentContainerStyle={ListFooterComponent ? undefined : styles.flexGrow1}
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...viewProps}
            />
            {React.isValidElement(ListFooterComponent) && <View style={styles.flexGrow1}>{ListFooterComponent}</View>}
        </ScrollView>

We can wrap both elements in ScrollView and disable the scroll functionality of DraggableFlatList.

        <ScrollView contentContainerStyle={styles.flexGrow1}>
            <DraggableFlatList
                ref={ref}
                containerStyle={ListFooterComponent ? undefined : styles.flex1}
                contentContainerStyle={ListFooterComponent ? undefined : styles.flexGrow1}
                scrollEnabled={false}
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...viewProps}
            />
            {React.isValidElement(ListFooterComponent) && <View style={styles.flexGrow1}>{ListFooterComponent}</View>}
        </ScrollView>

Result

distance_rate_scroll_issue.mp4

[allgandalf]

Proposal

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

Only for Android, Page is not scrollabe when we add multiple stops for distance request.

What is the root cause of that problem?

This is bug is for android specific platform because in the Native file, we have wrapped the DraggableList in a View:

App/src/components/DraggableList/index.android.tsx

Lines 11 to 12 in 525ad6f

	<View style={styles.flex1}>
	<DraggableFlatList

This was because we also display the component ListFooterComponent:

App/src/components/DraggableList/index.android.tsx

Lines 19 to 20 in 525ad6f

	{React.isValidElement(ListFooterComponent) && <View style={styles.flexGrow1}>{ListFooterComponent}</View>}
	</View>

Due to which we are unable to scroll on Android.

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

We need to make a function for ListFooterComponent and pass it as a prop ListFooterComponent to DraggableFlatList:

diff --git a/src/components/DraggableList/index.android.tsx b/src/components/DraggableList/index.android.tsx
index 30bf7c927b..70b2557e8b 100644
--- a/src/components/DraggableList/index.android.tsx
+++ b/src/components/DraggableList/index.android.tsx
@@ -7,17 +7,20 @@ import type {DraggableListProps} from './types';
 
 function DraggableList<T>({renderClone, shouldUsePortal, ListFooterComponent, ...viewProps}: DraggableListProps<T>, ref: React.ForwardedRef<FlatList<T>>) {
     const styles = useThemeStyles();
+
+    const FooterComponent = () => {
+        return <View style={styles.flexGrow1}>{React.isValidElement(ListFooterComponent) && ListFooterComponent}</View>;
+    };
+
     return (
-        <View style={styles.flex1}>
-            <DraggableFlatList
-                ref={ref}
-                containerStyle={ListFooterComponent ? undefined : styles.flex1}
-                contentContainerStyle={ListFooterComponent ? undefined : styles.flexGrow1}
-                // eslint-disable-next-line react/jsx-props-no-spreading
-                {...viewProps}
-            />
-            {React.isValidElement(ListFooterComponent) && <View style={styles.flexGrow1}>{ListFooterComponent}</View>}
-        </View>
+        <DraggableFlatList
+            ref={ref}
+            containerStyle={ListFooterComponent ? undefined : styles.flex1}
+            contentContainerStyle={ListFooterComponent ? undefined : styles.flexGrow1}
+            // eslint-disable-next-line react/jsx-props-no-spreading
+            {...viewProps}
+            ListFooterComponent={FooterComponent}
+        />
     );
 }
 

Result Video

Screen.Recording.2024-05-25.at.2.45.49.PM.mov

[Krishna2323]

Proposal updated

• Added alternative option 2

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[stephanieelliott]

Hey @mollfpr, a few proposals here to review when you get a chance!

[mollfpr]

Wrap the content in a ScrollView and remove the View.

@Krishna2323 What's the difference between your main and the alternative solution? Also, I tried the above step but it did not work. In the alternative solution, wrapping the DraggableList with <ScrollView style={styles.flex1}> seems to fix the issue, so I don't understand the purpose of the second step.

@GandalfGwaihir As mentioned before in the @Krishna2323 proposal, we can't use the ListFooterComponent to avoid the issue #40211 arising again.

[Krishna2323]

@mollfpr, I think we only need to add the ScrollView on in android file, so we can go with the second option in my alternative solution, <ScrollView style={styles.flexGrow1}>, we will use flexGrow1 instead of flex1 because the map won't take full available height if we use flex1.

[allgandalf]

we can't use the ListFooterComponent to avoid the issue #40211 arising again.

I wasn't able to test on physical device as i don't have one, but when i tested my solution out on emulator, it worked fine even with the issue linked, are you sure that if we implement my proposal, that bug will occur again @mollfpr ?

[mollfpr]

@GandalfGwaihir I'm pretty sure the bug will occur again. The PR #41603 is also intentionally moved to the android file to avoid using ListFooterComponent.

@Krishna2323 Could you update your proposal?

[allgandalf]

oh cool, I will think of something else here, also @Krishna2323 , don’t wrap inside scrollview, We can’t wrap a virtual list inside scroll view, we will get console error for that

[Krishna2323]

@mollfpr, proposal updated.

• Resolved the consoled error issue mentioned by @GandalfGwaihir above
• Added recording of the result

[melvin-bot]

@mollfpr, @stephanieelliott Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mollfpr]

@Krishna2323 The result looks good, but I think the expected result is similar to what we have in iOS where the scroll can be used above the waypoint list instead of just space beside the add stop button.

Simulator.Screen.Recording.-.iPhone.15.Pro.17.2.-.2024-06-04.at.02.57.04.mov

[allgandalf]

I have a spare android device where i can test my proposal and see if the mentioned issue occurs again, should i do that? or it would be a waste of time? @mollfpr

[melvin-bot]

📣 @allgandalf! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[Krishna2323]

@mollfpr, Proposal Updated .

• Updated main solution to use NestableScrollContainer & NestableDraggableFlatList, also added result video.

[melvin-bot]

Triggered auto assignment to @lschurr (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[stephanieelliott]

Applying the New Feature label to get another BZ member on this while I am OOO til July 10. To catch you up on where we are @lschurr, we have a PR and it's being actively reviewed: #43810

Thanks for watching over this, I'll grab it back from you when I return!

[stephanieelliott]

This is held on an upstream fix #43810 (comment)

[stephanieelliott]

Upstream issue we are blocked on is here: computerjazz/react-native-draggable-flatlist#543

[melvin-bot]

This issue has not been updated in over 15 days. @mollfpr, @stephanieelliott, @stitesExpensify, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.15-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Distance - Page is not scrollable after adding multiple stops. #43810

If no regressions arise, payment will be issued on 2024-08-09. 🎊

For reference, here are some details about the assignees on this issue:

• @mollfpr requires payment through NewDot Manual Requests
• @Krishna2323 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
• [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@mollfpr] Determine if we should create a regression test for this bug.
• [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[stephanieelliott]

Summarizing payment on this issue:

• Contributor: @Krishna2323 $250 - PAID via Upwork
• Contributor+: @mollfpr $250 - please request via ND

Upwork job is here: https://www.upwork.com/jobs/~1737876297360310272

[stephanieelliott]

@mollfpr can you please complete the BZ checklist for this issue?

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

I couldn't find the offending PR.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be good.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Click on FAB > Submit expense > Distance tab
2. Add a start waypoint, a finish waypoint, and multiple stops
3. Verify page is scrollable after the content overflows
4. 👍 or 👎

[JmillsExpensify]

$250 approved for @mollfpr

[melvin-bot]

@mollfpr, @stephanieelliott, @stitesExpensify, @Krishna2323 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[stephanieelliott]

Test case issue created: https://github.com/Expensify/Expensify/issues/420216

All done here, thanks!