[HOLD for payment 2024-05-20] CRITICAL: [P2P Distance] [$500] Enable immediate slide scrolling without tap

[m-natarajan]

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.62-6 Reproducible in staging?: y Reproducible in production?: 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: @quinthar Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712975397589869

Action Performed:

1. Tap green FAB button
2. Select 'track expense'
3. Tap distance tab
4. Touch the screen and pause ever so briefly after touching the screen.
5. Touch the screen and immediately move

   Expected Result:

   Able to move the map with fingers when touching the screen

   Actual Result:

   I need to "tap and then slide" (where my finger needs to hold still for maybe 100ms before moving) rather than just "slide" (ie, where my finger can move immediately after touching the map, like in Google Maps).

   Workaround:

   unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/1793014e-2d15-49d1-b581-b5fe8ef5594e

https://github.com/Expensify/App/assets/38435837/ebcc67b6-c966-4827-b231-4ed349cd39c5

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013a0e30e9fd5243ce
• Upwork Job ID: 1780282719880835072
• Last Price Increase: 2024-05-02
• Automatic offers:
• ikevin127 | Contributor | 0
• suneox | Contributor | 0

Issue Owner

Current Issue Owner: @zanyrenney

[melvin-bot[bot]]

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

[zanyrenney]

reviewed, assigining external.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[suneox]

Proposal

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

MapView conflict event with another component when scrolling on Android

What is the root cause of that problem?

The PanResponder on MapView sometimes has been terminated by DraggableFlatList and another places using PanResponderCapture event at ScreenWrapper and TabNavigator

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

We will add onMoveShouldSetPanResponderCapture and onStartShouldSetPanResponderCapture to MapView to early prevent conflict event with another component, and separate MapView/index.android.tsx, DraggableList/index.android.tsx to specific platform to avoid regression then update ListFooterComponent to bellow DraggableFlatList

Tested branch

What alternative solutions did you explore? (Optional)

[shahinyan11]

Proposal

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

[P2P Distance] Enable immediate slide scrolling without tap

What is the root cause of that problem?

Рere we are rendering the DistanceRequestFooter ( It contains MapView inside ) as a ListFooterComponent of the DraggableList, which causes the gestures of the MapView and the DraggableList to conflict.

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

1. Add a dragHitSlop prop to the DraggableFlatList, where the bottom key equal to the height of MapView in ListFooterComponent, to disable drag gestures on the MapView area. For that we need to measure the height of MapView in DraggableFlatList component. To achieve this we should update DraggableFlatList component as below.

   function DraggableList<T>({renderClone, shouldUsePortal, ListFooterComponent, ...viewProps}: DraggableListProps<T>, ref: React.ForwardedRef<FlatList<T>>) {
   const styles = useThemeStyles();
   const [mapViewHeight, setMapViewHeight] = useState(0)
   
   const EnhancedListFooterComponent = ListFooterComponent && React.cloneElement(ListFooterComponent, {
       onLayout: (event) => {
           const { height } = event.nativeEvent.layout;
           setMapViewHeight(height);
       },
   })
   
   return (
       <DraggableFlatList
           ref={ref}
           containerStyle={styles.flex1}
           contentContainerStyle={styles.flexGrow1}
           ListFooterComponentStyle={styles.flex1}
           dragHitSlop={{top:0,left:0,right:0, bottom:-mapViewHeight}}
           ListFooterComponent={EnhancedListFooterComponent}
           // eslint-disable-next-line react/jsx-props-no-spreading
           {...viewProps}
       />
   );
   }

2. Step 3 also requires the following changes to work - Add ...props in props of DistanceRequestFooter component and {...props} on container of MapView

   
   function DistanceRequestFooter({waypoints, transaction, mapboxAccessToken, navigateToWaypointEditPage, ...props}
   ...

<View {...props} style={styles.mapViewContainer} >

### What alternative solutions did you explore? (Optional)
Or just use `ListFooterComponent` out of  `DraggableFlatList`

[mollfpr]

@suneox @shahinyan11 Are you able to reproduce the issue in the emulator iOS and Android?

[shahinyan11]

@suneox @shahinyan11 Are you able to reproduce the issue in the emulator iOS and Android?

I tested on a real Android device and reproduced it. I think this is a case that is better tested on a real device.

[suneox]

@suneox @shahinyan11 Are you able to reproduce the issue in the emulator iOS and Android?

I just tested on real device

[zanyrenney]

@mollfpr please can you review the comments above and see if they work wiht the proposals?

[suneox]

Proposal updated

Add alternative solution and Tested branch

[quinthar]

What are the next steps? Are we waiting on @mollfpr to pick a proposal? What's the ETA on that?

[mollfpr]

Sorry for the delay. I just got my phone set up to test the issue.

@shahinyan11 Your solution doesn't look good on the web.

[shahinyan11]

@shahinyan11 Your solution doesn't look good on the web.

@mollfpr Are you about UI or functionality. Are you talking about the user interface or functionality? If you mean the UI, then there is a question of a slight change in styles

[mollfpr]

@suneox I'm testing your branch but the issue is still there.

https://github.com/Expensify/App/assets/25520267/d2283ff6-e6b2-4c54-ba37-44e28e0441bf

@shahinyan11 Could you please update your proposal to make it work on all platforms?

[quinthar]

@shahinyan11 Can you please give an ETA? Thanks!

[suneox]

@suneox I'm testing your branch but the issue is still there.

@mollfpr Are you testing on real device? I have test solution work well on my phone, could you please share about your device test?

[mollfpr]

@suneox Yes, I'm testing it on SG S23 Ultra. You can see the tap in the video in frames 0.11 and 0.15 is moving but the map is not.

[shahinyan11]

@shahinyan11 Can you please give an ETA? Thanks!

I will give updates today

[melvin-bot[bot]]

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

[shahinyan11]

I have problems with android build. so it can take more time

[shahinyan11]

@mollfpr I have one question. I noticed that my change caused MapView to start capturing navigator gestures. Is it ok ?

[zanyrenney]

bump @mollfpr please get back to @shahinyan11 on their question. Thanks!

[quinthar]

@shahinyan11 can you please elaborate on what these "navigation gestures" are? Do you think it's a good thing?

[shahinyan11]

@quinthar Sometimes navigation between tabs happens when we swipe from left to right on map

[melvin-bot[bot]]

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[neil-marcellini]

Assigning @ikevin127 as C+ from Slack here.

[suneox]

Update Tested branch based on my proposal

[ikevin127]

♻️ Proposals review in progress!

[shahinyan11]

Proposal

Updated

[shahinyan11]

Proposal

Updated. I did complex change to desired result. I think it works good now

[ikevin127]

@suneox I tested both your solutions and neither of them work in fixing the issue, even though there might be some small improvement to the capture of the swipe event on the map - if we swipe quick, the issue is still present:

Video

https://github.com/Expensify/App/assets/56457735/8a8c136e-bc6d-41c6-94a6-ac960b792dd6

[ikevin127]

@shahinyan11 Your solution seems to work and given that it's only changing the index.native.tsx version of the DraggableFlatList it is a good thing because we're not changing logic on other platforms where the issue does not occur.

Video

https://github.com/Expensify/App/assets/56457735/e6adc129-d158-427a-8fa1-dcc476d9a337

This can be done even more specifically targeted for Android only by splitting the index.native.tsx in 2, having index.ios.tsx (unchanged) and index.android.tsx since this issue is only occuring in Android and we don't want to change the logic on iOS unnecessarily.

There are a few issues with the solution that I need addressed before we can move on:

1. We need some explanation as to what the solution is doing exactly to fix the issue. I'm asking for this because you simply provided the solution code block, without any explanation as to what the solution is doing.
2. I noticed that the {...responder.panHandlers} line from your solution is not required for the solution to work. I removed it because your solution did not provide the source of this responder prop. Please also explain this part, whether or not it's actually required and why.
3. I noticed that you used a setTimeout in your solution, and according to Code patterns to avoid:

   1. Make sure any setTimeouts are absolutely necessary, not just a workaround for a situation that isn’t fully understood (e.g. a race condition).

4. Additionally, I noticed that with your solution applied -> if we close and re-open the app while in debug mode, the app crashes when navigating to the Distance tab, with the following error which leads to the onLayout={onFooterLayout} line of your solution:

Screenshot (crash)


If you can clear up these things and update your proposal to reflect these clarifications, I'll do a final review to confirm everything works as expected then I'll recommend for assignment.

[melvin-bot[bot]]

@neil-marcellini @ikevin127 @zanyrenney 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!

[ikevin127]

Based on latest #expensify-open-source Slack 🧵 (conversation): we're currently waiting on @shahinyan11 to clarify their solution and get it to a working state as currently the app crashes when closing and re-opening it while the solution is applied.

Otherwise we're still open to proposals, preferably one that would work as expected based on the above Slack 🧵 conversation. Also, if this is really urgent I'd suggest increasing the bounty! 💰

cc @quinthar @neil-marcellini @zanyrenney

[shahinyan11]

I will have updates untill tomorrow

[suneox]

@suneox I tested both your solutions and neither of them work in fixing the issue, even though there might be some small improvement to the capture of the swipe event on the map - if we swipe quick, the issue is still present:

Video proposal-1.mp4

@ikevin127 Could you please try my alternative solution at this branch? I just sync and test on the latest main it still works well

https://github.com/Expensify/App/assets/11959869/ad583d7a-fadd-40f4-87dd-51845516b96c

For the latest change, we can intercept the pan responder early to notify onMapInteraction at MapView

[shahinyan11]

Proposal

Updated. @ikevin127 I updated my solution because it had a bottleneck and destroyed the navigation in one particular case

[shahinyan11]

@neil-marcellini @ikevin127 Would you mind if I ask for increased compensation here ?

[ikevin127]

♻️ Will review the updated proposals again today!

[ikevin127]

@ikevin127 Could you please try my alternative solution at this branch? I just sync and test on the latest main it still works well

@suneox I checked out your test branch and as mentioned in my last comment, the capture seems to be improved but the issue is still present if you swipe really fast. The reason why it might look / seem fixed on your side is because you're testing on an emulator using cursor to swipe and because of this you can't replicate really fast swipes as you would on an actual physical device with touch.

I tested this on a physical OnePlus 7T (Android 14) and here's the video of the behaviour (I activated tap visibility), and I tested your branch fix behaviour vs google maps (end of video) and here are the results:

Android: Native (OnePlus 7T - Android 14)

https://github.com/Expensify/App/assets/56457735/fe4d2cd5-0d28-4eb8-83da-cfb0773efe4b

[ikevin127]

https://github.com/Expensify/App/issues/40211#issuecomment-2059760924. @ikevin127 I updated my solution because it had a bottleneck and destroyed the navigation in one particular case

@shahinyan11 I tested your updated proposal, looks good, it doesn't crash the app anymore upon restart. There are some details related to the solution which I would change but that can be handled during PR.

✅ Here's how the behaviour looks on a physical device compared to google maps:

Android: Native (OnePlus 7T - Android 14)

https://github.com/Expensify/App/assets/56457735/78ba058a-058d-4e13-8ae7-a4ba23b399d1

[!warning] There's only 1 issue left with your current solution:

1. It breaks the swipe navigation behaviour the first time you mount the Distance tab page.

What happens exactly is because of your solution, if we try to swipe right to get to the Scan tab, precisely swiping in between the bottom of the map and the Next button -> doesn't work anymore. The user needs to actually tap on the Scan tab, then swipe back to the Distance tab and only then the swipe navigation would work again from the Distance tab. Here's a video to get some visuals in order to better understand the issue:

Note: Pay attention to the tap indicator when I swipe.

Android: Native (Nav Swipe Issue)

https://github.com/Expensify/App/assets/56457735/ce698576-b7fb-4c08-8638-b1787249d2ec

Once this issue is fixed I think we're good for assignment here!

[ikevin127]

No fully working proposal so far, still looking for a proposal that fixes the issue without creating regressions.

[melvin-bot[bot]]

Upwork job price has been updated to $500

[shahinyan11]

@ikevin127 @suneox After a lot of research and testing many approaches, I came to the conclusion that it is better to leave everything as is or disable the swipe navigation for the distance tab.

[ikevin127]

Bumped in #expensify-open-source Slack 🧵, hopefully we get more eyes on this!

[suneox]

Hi @ikevin127 Could you please help me quick check on your device at the new branch? I have tested it on a real device and it works well on sony experia 1 II and sync latest main

https://github.com/Expensify/App/assets/11959869/21def188-980f-4407-84b5-bac6758a5edd

I will remove the debugging log and explain in detail after you test it works well

[suneox]

@ikevin127 @suneox After a lot of research and testing many approaches, I came to the conclusion that it is better to leave everything as is or disable the swipe navigation for the distance tab.

I will remove the debugging log and explain in detail after you test it works well

@ikevin127 After my research, the PanResponder on MapView sometimes has been terminated by DraggableFlatList and keyboardDissmissPanResponder use onMoveShouldSetPanResponderCapture at ScreenWraper so we have to use onStartShouldSetPanResponderCapture and onMoveShouldSetPanResponderCapture for PanResponder on MapView and separate the custom MapView for android to avoid regression

[ikevin127]

Could you please help me quick check on your device at the new branch?

I will test and let you know!

@ikevin127 After my research, the PanResponder on MapView sometimes has been terminated by DraggableFlatList and keyboardDissmissPanResponder use onMoveShouldSetPanResponderCapture at ScreenWraper so we have to use onStartShouldSetPanResponderCapture and onMoveShouldSetPanResponderCapture for PanResponder on MapView and separate the custom MapView for android to avoid regression.

It makes sense, I was looking forward to a proposal that would separate logic into index.android.tsx. Will test and get back with an answer in 1-2 hours!

[ikevin127]

@suneox Your new test branch tests well 🎉 It fixes our issue as per the expected result (see video below).

Android: Native (OnePlus 7T - Android 14)

https://github.com/Expensify/App/assets/56457735/e00d995b-baff-4fc0-9017-4c1f11167386

Please update your proposal detailing the WHY of the proposed changes so we can clearly understand the root cause and how the solution works in fixing our issue.