Remove `setTimeout` usage in AttachmentPicker/index.native.js #2719

kidroca · 2021-05-06T17:55:37Z

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

Expected Result:

The Attachment Picker modal should work correctly without the need of setTimeout

Actual Result:

setTimeout is needed because on iOS the Gallery/Camera is immediately dismissed when the Attachment bottom docked modal closes

Details

This is a follow up on #2656 (comment)
It can potentially be resolved by updating the attachment related libraries handled in this ticket: #2531

Action Performed:

Open a chat
Select (+) button to add an attachment
Select gallery
Sometimes the gallery would open and immediately close or not open at all

Workaround:

Added a brief timeout so that the Camera/Gallery is only opened a 1-2 frames after the Attachment Picker modal closes

Platform:

Where is this issue occurring?

Web
✅ iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.38-1
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

The text was updated successfully, but these errors were encountered:

mallenexpensify · 2021-05-25T17:29:31Z

@kidroca sorry for the delay here. (we've recently improved our process so new issues will get reviewed very quickly now)
I tried to reproduce on three chats in iOS (and iOS mWeb Safari) and wasn't able to. Can you see if you can reproduce and, if so, provide more details include a screen video recording?
Thanks,
Matt

kidroca · 2021-05-25T18:00:08Z

@mallenexpensify
There's no bug to reproduce here
This is a refactor remainder ticket so that we can update the code that uses setTimeout and remove the setTimeout usage
It can be done right now as this will introduce a bug

mallenexpensify · 2021-05-26T17:13:58Z

Thanks for the context @kidroca. I'm not an engineer so will loop one in here to review.

MelvinBot · 2021-05-26T17:14:13Z

Triggered auto assignment to @bondydaa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

bondydaa · 2021-05-26T20:02:55Z

Cool yep I agree we should not rely on a setTimeout as mentioned in the PR.

I think this could probably be worthwhile for an external contributor, I'm guessing @kidroca might have an idea of how to do this even?

I'm not sure it needs to be a daily, weekly is probably good for now since the sooner we get it cleaned up the less likely we will need to put more setTimeouts in the code to push things further back in the call stack.

bondydaa · 2021-05-26T20:04:15Z

I don't exactly have the time right now to manage external contributors so I'm not going to apply the external label but @mallenexpensify if you want to so we can get the job up on upwork go for it, i'll ask in engineering-chat as well.

kidroca · 2021-05-26T20:45:15Z

Hey, guys I just want to clarify, the setTimeout is needed right now to prevent the bug. We had a similar issue where I've exausted many paths to try and get rid of it. It's possible it won't be needed if we update the image picker library to an up to date version - there is a ticket about it linked in the description.
There is no issue at the moment and the task can be postponed. At least after the other ticket is resolved

Christinadobrzyn · 2021-05-27T06:51:55Z

External Job posted in Upwork - https://www.upwork.com/jobs/~012fc94068d907e4dd
Internal Job posted in Upwork - https://www.upwork.com/ab/job-post/1328455843768528896/1397807119242944512

Invited @kidroca to the Upwork project to work on it as needed

kidroca · 2021-05-27T12:08:33Z

Sorry but there's some big misunderstanding here. There's nothing that can be done for this ticket ATM. And there's no bug that needs fixing - I was asked to open this task as a reminder to remove the setTimeout call at some point in the future when that's possible

#2656 (comment)

@tgolen
Could you please create a follow-up GH issue to remove the setTimeout once the libraries are updated and this is no longer a problem?

Currently it's not possible it will lead to a bug preventing image picking I don't see any way to address this right now.
The best thing to do IMO is to wait the other ticket that updates the image picker library and see if the setTimeout can be removed then

@Christinadobrzyn I can't accept the Upwork invite as I don't see a way to complete this task ATM

tgolen · 2021-05-27T15:40:44Z

Yeah, let's put this on HOLD until that library is updated. Good call.

Christinadobrzyn · 2021-05-28T00:21:12Z

Ah sorry for the confusion! Thanks for clarifying! Removed the Upwork posting and added a HOLD label to this issue.

kidroca · 2021-06-11T09:20:41Z

@tgolen
Unfortunately updating the libraries did not resolve the issue, and setTimeout is still required: https://github.com/Expensify/Expensify.cash/pull/3420/files#r649441003

I've spent quite some time searching for a fix and trying different things and the only possible fix was
to leave the modal (Create Menu: Take Photo | Gallery | Documents) open and only close it after image/document
selection, but this causes an unpleasant experience of seeing the modal after selecting an image: #2580 so it was decided to close before opening the camera/gallery

Linking the original issue that dealt with setTimeout: #1913

kidroca · 2021-06-11T09:25:46Z

I can try to dig deeper and trace this to the core, but it could take me days and I might still not be able to solve it
If you still want me to try, please offer me a job through Upwork, but I want to make it clear that I may not be able to solve this issue

kadiealexander · 2021-10-08T02:41:35Z

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

MelvinBot · 2021-10-11T19:00:14Z

This issue has not been updated in over 15 days. @kidroca 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!

kidroca · 2021-10-14T17:31:32Z

This ticket is related as we also had to use setTimeout to make the Create Menu modal appear: #5303

The common things are:

it's a modal not being visible issue
iOS only
setTimeout fixes the issue
- in the end it was decided that a small delay is needed anyway for a better UX (for that particular new user flow)

Another interesting thing is it's somehow related to the Sidebar/Drawer.

<Modal isVisible={true}>{/* some sample content */}</Modal>

Adding code like this to the Sidebar would not render the modal (only setting visible with delay works), but adding that same piece of code elsewhere (like the Report Screen) would render a modal over it.
It's possible the places where we use the AttachmentPicker (composer, avatar picker) have a predisposition for the same problem

kidroca · 2021-10-24T20:14:53Z

Checked to see if any updates since last time we tried fixed the issue but they don't
Removing the setTimeout still results in a bug where camera/gallery/documents disappear

Starting a separate project to isolate a reproducible behavior and find out exactly what's causing this

kidroca · 2021-10-25T13:25:45Z

Before starting a separate project I've also tried upgrading the following libs and retesting, but the issue was still present:

diff --git a/package.json b/package.json
@@ -51,10 +51,10 @@
     "@react-native-firebase/perf": "^12.3.0",
     "@react-native-masked-view/masked-view": "^0.2.4",
     "@react-native-picker/picker": "^1.9.11",
-    "@react-navigation/compat": "^5.3.15",
-    "@react-navigation/drawer": "6.1.4",
-    "@react-navigation/native": "6.0.2",
-    "@react-navigation/stack": "6.0.7",
+    "@react-navigation/compat": "^5.3.20",
+    "@react-navigation/drawer": "^6.1.8",
+    "@react-navigation/native": "^6.0.6",
+    "@react-navigation/stack": "^6.0.11",
     "babel-plugin-transform-remove-console": "^6.9.4",
     "dotenv": "^8.2.0",
     "electron-context-menu": "^2.3.0",
@@ -75,27 +75,27 @@
     "react": "^17.0.2",
     "react-collapse": "^5.1.0",
     "react-dom": "^17.0.2",
-    "react-native": "0.64.1",
+    "react-native": "0.66.1",
     "react-native-bootsplash": "^3.2.0",
     "react-native-collapsible": "^1.6.0",
     "react-native-config": "^1.4.0",
-    "react-native-document-picker": "^5.1.0",
-    "react-native-gesture-handler": "1.9.0",
+    "react-native-document-picker": "^7.1.1",
+    "react-native-gesture-handler": "^1.10.3",
     "react-native-google-places-autocomplete": "^2.4.1",
     "react-native-image-pan-zoom": "^2.1.12",
-    "react-native-image-picker": "^4.0.3",
+    "react-native-image-picker": "^4.1.2",
     "react-native-keyboard-spacer": "^0.4.1",
-    "react-native-modal": "^11.10.0",
+    "react-native-modal": "^13.0.0",
     "react-native-onyx": "git+https://github.com/Expensify/react-native-onyx.git#6eadd7f73fca87e6551e43607ec78f645e17ef50",
     "react-native-pdf": "^6.2.2",
     "react-native-performance": "^2.0.0",
     "react-native-permissions": "^3.0.1",
     "react-native-picker-select": "8.0.4",
     "react-native-plaid-link-sdk": "^7.1.0",
-    "react-native-reanimated": "^2.3.0-alpha.1",
+    "react-native-reanimated": "^2.2.3",
     "react-native-render-html": "6.0.0-beta.8",
-    "react-native-safe-area-context": "^3.1.4",
-    "react-native-screens": "^3.0.0",
+    "react-native-safe-area-context": "^3.3.2",
+    "react-native-screens": "^3.8.0",
     "react-native-svg": "^12.1.0",
     "react-native-unimodules": "^0.13.3",
     "react-native-web": "0.15.7",

kidroca · 2021-10-25T14:40:26Z

This is the issue reported on react-native-image-picker: react-native-image-picker/react-native-image-picker#1456

The important parts

react-native-image-picker/react-native-image-picker#1456 (comment)

The only other possible thing I can think of is I show a modal selector from react-native-modal-selector, and then open the camera or the image library, based on the selection. However, like I mentioned it always opens on the first time, but never after that.

react-native-image-picker/react-native-image-picker#1456 (comment)

Attempt to present <UIImagePickerController: 0x10b03dc00> on <RCTModalHostViewController: 0x113f8e0f0> (from <RCTModalHostViewController: 0x113f8e0f0>) whose view is not in the window hierarchy.
This is exactly what happens to us, if we remove the setTimeout

react-native-image-picker/react-native-image-picker#1456 (comment)

When using react-native modal or derivate libraries to show the options and closing them, Make sure the Modal is closed completely before calling the library methods. Because the library opens a View which attaches to previous view, since the previous view is closed the launchImageLibrary or launchCamera cannot attach to any view and the error happens. This is a known in issue in react-native when dealing with Closing and opening a iOS modal simultaneously.

The error we're getting in xCode when we select "Choose from gallery" or "Choose documents" is

xCode logs when we press "Choose from gallery" button. (iOS 15.0.2 iPhone 11 Pro Physical Device)

2021-10-25 17:26:24.857524+0300 New Expensify[713:73404] [Presentation] Attempt to present <PHPickerViewController: 0x121156fe0> on <RCTModalHostViewController: 0x125af5de0> (from <RCTModalHostViewController: 0x125af5de0>) whose view is not in the window hierarchy.

same as other users experienced in the linked issue above

xCode logs when we press "Choose document" button. (iOS 15.0.2 iPhone 11 Pro Physical Device)

2021-10-25 17:28:17.968877+0300 New Expensify[713:73404] [Assert] Setting modalPresentationStyle once presentationController has been accessed will have no effect until <UIDocumentPickerViewController: 0x11ff3a740> is presented, dismissed, and presented again.

From the error message it seems the View that the gallery/documents picker attaches to is the one of the Modal we've just closed.
Opening the gallery/documents picker with a delay (setTimeout) seems to be attaching it to a different view (not the Modal) and they work correctly

So far only 2 things have fixed the issue:

using setTimeout -> but we're trying to get rid of it as it's a dirty solution
not closing the modal until we're done picking an image (or document) -> this causes bad UX where users would see the Attachments popup (camera/gallery/document) for a brief moment after they select an image

I'm working on recreating the issue in a minimal reproduction repository and link it to issues on:

react-native-modal -> could be raising the onModalHide a bit too early
react-native-image-picker -> this is the linked issue
react-native-document-picker

kidroca · 2021-10-25T14:46:06Z

Removing the [Hold] as it was decided to continue working on this here: https://expensify.slack.com/archives/C01GTK53T8Q/p1633990542157900

kidroca · 2021-10-27T13:38:40Z

I've created a standalone repository where I've partially recreated the issue

recrated the problem for RN document picker
couldn't recreate the problem for RN image picker

standalone app showing the problem

Screen.Recording.2021-10-27.at.15.42.24.mov

It looks like onModalHide is called at a time where the resources are not completely released and the next modal/picker/alert would attach to the closing modal
I think launchGallery starts slower than pickDocument and so in my standalone example it doesn't suffer the bug

There's a now closed ticket about onModalHide not firing late enough: react-native-modal/react-native-modal#534
And here's a related ticket: react-native-modal/react-native-modal#484 where it was suggested to use InteractionManager. runAfterInteractions. I've tried that but it didn't solve the problem
I've posted some notes on the issues there

kidroca · 2021-10-27T13:40:13Z

Here's what works so far:

setTimeout - we're looking for a way to get rid of it
don't close the modal but launch gallery/document picker over it, and close it at the end - this works but creates bad UX where you see the attachment options for a moment after you've picked something (we decided to use setTimeout to close the modal first and avoid the bad UX)
replace setTimeout with requestAnimationFrame - this works, it again feels as a workaround, but at least it does not involve magic numbers, e.g. delay of 200 might work on most devices but still fail on some, while requestAnimationFrame should run consistently when the device has breathing space to execute the code

What didn't work

InteractionManager.runAfterInteractions - would sometimes fix the problem but not always and it had to be executed on onModalWillHide
Completely unmounting the modal like !this.shouldShowGallery && <Popup /> and then calling launchGallery. This still fails to display the gallery on iOS
updating all relevant packages to their latest versions - react-native, the @navigation/* packages, screens, reanimated, gesture handler, the image and the document pickers and other libraries
changing navigation configuration - tweaks like
- removing mode="modal" from RootStack.Navigator
- removing the drawer navigation altogether and just render the ReportScreen

tgolen · 2021-10-27T16:57:39Z

Thanks for your continued work on this. I am following along! 🍿

MelvinBot · 2021-11-19T19:00:30Z

This issue has not been updated in over 15 days. @kidroca 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!

kidroca · 2021-11-19T21:02:48Z

There has been no development on the external tickets I've shared and I don't have definitive evidence that this is a problem with RN Modal and the onModalHide being called a bit too soon

I need to debug this through xCode in order to pause the debugger when onModalHide executes the native code and see whether the modal view/controller is still attached. When I'm done with my other tickets I'll return here

MelvinBot · 2022-02-14T19:21:37Z

@kidroca, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

MelvinBot · 2022-02-14T19:21:37Z

@kidroca, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

MelvinBot · 2022-02-14T19:21:38Z

@kidroca, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

kidroca · 2022-02-14T19:43:29Z

No updates on the library issues I've commented on and tracking
I had a plan to debug this, when I'm not working on higher priority stuff

kidroca mentioned this issue May 6, 2021

Fix: Attachment Picker briefly displayed after taking a picture #2656

Merged

5 tasks

mallenexpensify assigned kidroca May 25, 2021

mallenexpensify added the Engineering label May 26, 2021

MelvinBot assigned bondydaa May 26, 2021

mallenexpensify added the Daily KSv2 label May 26, 2021

bondydaa added Weekly KSv2 and removed Daily KSv2 labels May 26, 2021

bondydaa removed their assignment May 26, 2021

iwiznia added the External Added to denote the issue can be worked on by a contributor label May 26, 2021

MelvinBot assigned Christinadobrzyn May 26, 2021

MelvinBot added the Daily KSv2 label May 26, 2021

Expensify deleted a comment from MelvinBot May 26, 2021

iwiznia removed the Daily KSv2 label May 26, 2021

iwiznia unassigned Christinadobrzyn May 26, 2021

Christinadobrzyn changed the title ~~Remove setTimeout usage in AttachmentPicker/index.native.js~~ [HOLD] Remove setTimeout usage in AttachmentPicker/index.native.js May 28, 2021

kadiealexander added the n6-hold label Sep 16, 2021

MelvinBot added Monthly KSv2 and removed Weekly KSv2 labels Oct 11, 2021

MelvinBot removed the Overdue label Oct 11, 2021

mallenexpensify added Weekly KSv2 and removed Monthly KSv2 labels Oct 11, 2021

kidroca changed the title ~~[HOLD] Remove setTimeout usage in AttachmentPicker/index.native.js~~ Remove setTimeout usage in AttachmentPicker/index.native.js Oct 25, 2021

mallenexpensify removed the n6-hold label Nov 16, 2021

MelvinBot added Overdue and removed Weekly KSv2 labels Nov 16, 2021

MelvinBot added Monthly KSv2 and removed Overdue labels Nov 19, 2021

MelvinBot closed this as completed Feb 14, 2022

MelvinBot added the Not a priority label Feb 14, 2022

koko57 mentioned this issue Apr 18, 2023

[$1000] IOS - App freezes when user uploaded messages and image while offline #16591

Closed

6 tasks

priyeshshah11 mentioned this issue May 1, 2023

Fix keyboard issues in add attachment flow #15337

Closed

55 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `setTimeout` usage in AttachmentPicker/index.native.js #2719

Remove `setTimeout` usage in AttachmentPicker/index.native.js #2719

kidroca commented May 6, 2021 •

edited

Loading

mallenexpensify commented May 25, 2021

kidroca commented May 25, 2021

mallenexpensify commented May 26, 2021

MelvinBot commented May 26, 2021

bondydaa commented May 26, 2021

bondydaa commented May 26, 2021

kidroca commented May 26, 2021

Christinadobrzyn commented May 27, 2021 •

edited

Loading

kidroca commented May 27, 2021 •

edited

Loading

tgolen commented May 27, 2021

Christinadobrzyn commented May 28, 2021

kidroca commented Jun 11, 2021 •

edited

Loading

kidroca commented Jun 11, 2021

kadiealexander commented Oct 8, 2021

MelvinBot commented Oct 11, 2021

kidroca commented Oct 14, 2021 •

edited

Loading

kidroca commented Oct 24, 2021

kidroca commented Oct 25, 2021 •

edited

Loading

kidroca commented Oct 25, 2021

kidroca commented Oct 25, 2021

kidroca commented Oct 27, 2021

kidroca commented Oct 27, 2021 •

edited

Loading

tgolen commented Oct 27, 2021

MelvinBot commented Nov 19, 2021

kidroca commented Nov 19, 2021

MelvinBot commented Feb 14, 2022

MelvinBot commented Feb 14, 2022

MelvinBot commented Feb 14, 2022

kidroca commented Feb 14, 2022

Remove setTimeout usage in AttachmentPicker/index.native.js #2719

Remove setTimeout usage in AttachmentPicker/index.native.js #2719

Comments

kidroca commented May 6, 2021 • edited Loading

Expected Result:

Actual Result:

Details

Action Performed:

Workaround:

Platform:

mallenexpensify commented May 25, 2021

kidroca commented May 25, 2021

mallenexpensify commented May 26, 2021

MelvinBot commented May 26, 2021

bondydaa commented May 26, 2021

bondydaa commented May 26, 2021

kidroca commented May 26, 2021

Christinadobrzyn commented May 27, 2021 • edited Loading

kidroca commented May 27, 2021 • edited Loading

tgolen commented May 27, 2021

Christinadobrzyn commented May 28, 2021

kidroca commented Jun 11, 2021 • edited Loading

kidroca commented Jun 11, 2021

kadiealexander commented Oct 8, 2021

MelvinBot commented Oct 11, 2021

kidroca commented Oct 14, 2021 • edited Loading

kidroca commented Oct 24, 2021

kidroca commented Oct 25, 2021 • edited Loading

kidroca commented Oct 25, 2021

The important parts

xCode logs when we press "Choose from gallery" button. (iOS 15.0.2 iPhone 11 Pro Physical Device)

xCode logs when we press "Choose document" button. (iOS 15.0.2 iPhone 11 Pro Physical Device)

kidroca commented Oct 25, 2021

kidroca commented Oct 27, 2021

standalone app showing the problem

kidroca commented Oct 27, 2021 • edited Loading

tgolen commented Oct 27, 2021

MelvinBot commented Nov 19, 2021

kidroca commented Nov 19, 2021

MelvinBot commented Feb 14, 2022

MelvinBot commented Feb 14, 2022

MelvinBot commented Feb 14, 2022

kidroca commented Feb 14, 2022

Remove `setTimeout` usage in AttachmentPicker/index.native.js #2719

Remove `setTimeout` usage in AttachmentPicker/index.native.js #2719

kidroca commented May 6, 2021 •

edited

Loading

Christinadobrzyn commented May 27, 2021 •

edited

Loading

kidroca commented May 27, 2021 •

edited

Loading

kidroca commented Jun 11, 2021 •

edited

Loading

kidroca commented Oct 14, 2021 •

edited

Loading

kidroca commented Oct 25, 2021 •

edited

Loading

kidroca commented Oct 27, 2021 •

edited

Loading