Fix: Modal closes immediately after launch #2114

kidroca · 2021-03-26T19:32:42Z

Details

Converts the existing Attachment Picker implementation to a bottom docked modal. There's a sample video in the iOS section

Notable changes:

AttachmentPicker: Since this is a multi index component - to keep props interface uniform it was extracted to separate file
Converted native AttachmentPicker to a class based component, preserving existing render prop interface
Removed logic that causes a side effect that closes the attachment options modal
- Discussed here: iOS issue: Modal closes immediately after launch #1913 (comment)
Added error handling to let the user know when image picking fails for reasons other than permissions instead of leaving him guessing
The wrapped calls to RNImage picker with promises to keep a uniform and easy to follow interface

Fixed Issues

Fixes #1913 also fixes #2198

Tests

For web, mWeb and Desktop

Found a bug related to Safari, please use another browser to test (Chrome works): #2109

Open a chat
Press the (+) plus button to open the input menu
Select "Add Attachment"
The browser's file picker should open and you can select a file
When you select a file you should see File Preview modal
Pressing upload should upload the file and you should see the attachment/thumb inside the chat
Upon the File Preview closes (previous step) focus should be returned to the compose input field

iOS/Android

Open a chat
Press the (+) plus button to open the input menu
Select "Add Attachment"
A second modal should open and show options for: camera, gallery, document

Selecting "Take Photo"
- default flow: You shoot a photo, select "Use Photo" and continue to the next step (File Preview)
- other cases:
  - pressing "Cancel" (inside the camera) closes any modals and returns you to the "Chat".
  - not granting permission should display a helpful Alert message
  - other errors should display a general error message to let the user know something did go right. You can verify this by selecting "Take Photo" on a iOS simulator - you should see a message that taking photos on a simulator is not supported
Selecting "Choose from Gallery"
- default flow: You select a photo from the gallery and continue to the next step (File Preview)
- other cases:
  - pressing "Cancel" (inside the gallery) closes any modals and returns you to the "Chat".
  - there's error handling that should alert the user in case of an error but I don't know a way to trigger it. I tested it by manually forcing code execution through that flow
Selecting "Choose Document"
- default flow: You select a document and continue to the next step (File Preview)
- other cases: Same as in "Choose from Gallery"

File Preview

At this point you should see a preview of whatever was selected using the previous options
Pressing "Upload" should upload the file and you should see the attachment/thumb inside the chat
Pressing (x) close on the top right corner should close the preview and not upload anything
Upon the File Preview closes focus should be returned to the compose input field

Regarding field input focusing

the field is no longer focused when any modal closes
the field would autofocus at the end of the full attachment flow

QA Steps

Same as above

Note that the video sample is using the iOS simulator with the software keyboard toggled on:
#2114 (comment)

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS (Video)

2021-03-29.13-56-42.mp4

Android

github-actions · 2021-03-26T19:33:00Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

kidroca · 2021-03-26T20:06:41Z

I have read the CLA Document and I hereby sign the CLA

kidroca · 2021-03-26T20:07:03Z

recheck

NikkiWines · 2021-03-30T00:58:16Z

going to also add @Julesssss here as a reviewer since he's got context to for issue and proposed solution. Ran out of time to get to this today but will review it first thing tomorrow.

src/pages/home/report/ReportActionCompose.js

src/components/AttachmentPicker/propTypes.js

Julesssss · 2021-03-30T10:09:30Z

the field is no longer focused when any modal closes

This is a regression for Web & Desktop. We need to regain focus when the attachment picker closes (not for mobile though)

Also, it looks like we're not closing the AttachmentPicker menu when on item is selected, can we do this? IMO it's odd mobile UX to see the prior modal when cancelling from the gallery (for example) -- I would expect the modal to close as the gallery/camera/picker is launched

Julesssss · 2021-03-30T10:20:28Z

~~It tested well on Web and mobile, but mobile web (iOS) didn't work for me. I see the AttachmentModal, but tapping Add Attachment does nothing.~~ -- nevermind, just realised this is the same safari issue you'll fix elsewhere!

I also would expect the options here to match mobile?

kidroca · 2021-03-30T10:23:27Z

This is a regression for Web & Desktop. We need to regain focus when the attachment picker closes (not for mobile though)

I've also brought this up here: #2129 (comment)
Autofocus should be handled differently on mobile vs web/desktop but I don't think this can be address by the current PR

Also, it looks like we're not closing the AttachmentPicker menu when on item is selected, can we do this? IMO it's odd mobile UX to see the prior modal when cancelling from the gallery (for example) -- I would expect the modal to close as the gallery/camera/picker is launched

I wasn't sure about this, I can update it to close. My logic was that if I selected gallery by mistake I can press "Cancel" and return to select something else like documents

kidroca · 2021-03-30T10:48:29Z

I also would expect the options here to match mobile?

I don't think the options would match on mWeb, it would use the web component with the hidden input - that would bring whatever file picker the browser would display to you.

Something like this:

It's not even possible to test it right now as this issue is also affecting mobile Chrome: #2109:

It might present something like this:

Depending on what the <input accept="..." /> is set to

This is well documented here: https://developers.google.com/web/fundamentals/media/capturing-images
I'll make sure it works in #2109

kidroca · 2021-03-30T11:27:11Z

Updated

reverted changes regarding "This is a regression for Web & Desktop."
this also closes the 2nd modal when "Cancel" is picked inside the gallery/camera

Julesssss · 2021-03-30T14:06:00Z

Autofocus should be handled differently on mobile vs web/desktop but I don't think this can be address by the current PR.

Yeah, no problem. I just mentioned it in case it simplified things, clearly not :D

src/components/AttachmentPicker/index.native.js

kidroca · 2021-03-30T17:19:26Z

Updated

Included a fix for Android camera permissions - it appears on android the response.error is different and was not covered by the switch (and the original logic)

Still needs a designer touch as the message is used as title.
Maybe something like:

Alert.alert(
  'Expensify.cash does not have access to your camera', // Title
  'Please enable the permission and try again.', // Message
  [...]
)

NikkiWines

Some small comments

src/components/AttachmentPicker/index.native.js

src/components/AttachmentPicker/propTypes.js

kidroca · 2021-03-31T12:24:59Z

Updated

improved error messages

Since React Native 0.60 to open App settings use: `Linking.openSettings()` - works on both android and ios https://stackoverflow.com/a/58026920/4834103

kidroca · 2021-04-02T17:29:27Z

Updated

To preserve review history, I'll push a backup branch or a tag that will store the original commits
This way I think review history will be kept as the original commits would still exist

Sorry this didn't work. 😞

You can still see what changes were requested in the Conversation tab here, but they won't appear in "File Changes"

I've addressed all merge conflicts and tested that this still works as expected with the latest codebase

NikkiWines

This looks good! Gonna leave it to @Julesssss for the final review + merge.

Toggling the keyboard on the simulator will switch between these behaviors. Though for a real device the keyboard will always pop out (unless the device is connected to an external keyboard)

I would put this in your testing steps so that QA knows to do this if they're using a simulator. Otherwise they might think there's an issue with the PR.

Julesssss

Code is looking good, but I ran through the tests again and I'm seeing a regression in the menu options. The 'Upload Photo' option is displayed -- for every single platform.

Julesssss

Whoops, forget that last review -- the option has just been renamed to 'Upload Photo'. Outside of the scope of this PR.

isagoico · 2021-04-07T00:45:36Z

~~PR was a pass, only thing is that we were able to reproduce~~ this scratch that, we found an issue in iOS mWeb

Edit: we found an issue in mWeb.

isagoico · 2021-04-07T01:48:17Z

This issue was reproducible in mWeb iOS only. On mWeb Android it was working as expected.

Title:
iOS mWeb - Attachment - Upload photo button does not work

Expected Result

The 3 attachment options appear.

Actual Result

Nothing happens after clicking the upload photo button

Action Performed

In a iOS device, go to Safari or Chrome
Navigate to staging.expensify.cash
Log in and navigate to a conversation
Click on the attachment button
Click on upload photo button.

Platform

mWeb iOS ✔️

Version Number: 1.0.11-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation
I didn't add the deploy blocker label since the issue is already reported and reproducible in production.

Pr.2114.Mweb.mp4

isagoico · 2021-04-07T02:05:01Z

Step 7 of the Web steps is failing only on firefox browser. Chrome browser works as expected.

Title:
Web - Firefox browser - After File Preview closes focus does not return to the input field automatically

Expected Result

After File Preview closes focus should be returned to the compose input field

Actual Result

After File Preview closes focus does not return to the input field automatically

Action Performed

Login in app https://staging.expensify.cash/signin
Open a chat
Press the (+) plus button to open the input menu
Select "Add Attachment"
The browser's file picker should open and you can select a file
When you select a file you should see File Preview modal
Pressing upload should upload the file and you should see the attachment/thumb inside the chat

Platform

Web (Firefox) ✔️

Build: 1.0.13-0

Notes/Images/Videos
Important: This is only happening on Firefox. Let me know if I should open a separate issue for this. On chrome, the PR was a pass.

Bug5007651_PR_2114.mp4

kidroca · 2021-04-07T07:54:58Z

This issue was reproducible in mWeb iOS only. On mWeb Android it was working as expected.

It wouldn't do anything on desktop Safari too

Web - Firefox browser - After File Preview closes focus does not return to the input field automatically

I've went to the point before the changes introduced here - v1.0.10-2 and the issue is still recreatable - it predates these changes

Julesssss · 2021-04-07T09:56:36Z

Thanks @isagoico. We have these issues covered already:

iOS mWeb - Attachment - Upload photo button does not work

We have a separate issue for Safari attachments, which currently doesn't work

Web - Firefox browser - After File Preview closes focus does not return to the input field automatically

We're handling focus issues together, I'll add this not to the issue to ensure Firefox is tested with the eventual fix: https://github.com/Expensify/Expensify/issues/154180

isagoico · 2021-04-07T15:35:58Z

Thanks for the clarification! Will check the PR off then 🙇

src/pages/home/report/ReportActionCompose.js

kidroca requested a review from a team as a code owner March 26, 2021 19:32

botify requested review from NikkiWines and removed request for a team March 26, 2021 19:33

kidroca force-pushed the kidroca/attachment-modal-update branch 3 times, most recently from 5ea8508 to 8820d75 Compare March 29, 2021 10:51

NikkiWines requested a review from Julesssss March 30, 2021 00:58

kidroca commented Mar 30, 2021

View reviewed changes

src/pages/home/report/ReportActionCompose.js Outdated Show resolved Hide resolved