Fix: mWeb/Web Safari - Selecting attachment

[kidroca]

@Julesssss

Details

Safari does not show the attachment picker if the button used to trigger file input disappears as soon as it's clicked
Added small updates to the CreateMenu components that preserve existing functionality but also allow to
trigger an action before the CreateMenu popover closes

Fixed Issues

Fixes #2109 and also fixes: #1159

Tests

This bug affected mobile Safari, mobile Chrome and Desktop Safari

1. From the home screen
2. Select the plus (+) button
3. Select "Upload Photo"
4. The file picker should appear

• check the screenshots
• on mobile I had no problem no a physical devices, but on the Simulator, the keyboard would try to appear and the file picking options would hide under. Pressing done would review them, but this is an issue with autofocus handling.

5. You can try to select the same file twice to see that When uploading a document, if you try to upload the same document more than once the preview doesn't appear. #1159 is fixed

QA Steps

Same as above

Note: there are changes to the Create Menu component - the component used to show the "New Chat", "Group Chat" options on the home screen FAB, and the Attachment Picker component which is also used on "Edit Photo" for the profile picture.
I've verified they still work, but you should probably test it as well

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web (iPhone 8 Simulator Safari)

Mobile Web (iPhone 11 Pro Safari)

Mobile Web (Android Pixel 2 Chrome)

[kidroca]

It might be better to wait for #2114 to be merged as it also made changes to the Attachment Picker

[tgolen]

OK, looks like that other PR would be good to merge first. Would you mind adding [HOLD PR#2114] to the beginning of the title of this PR? Then when the other PR has been merged, you can remove the HOLD and add a comment here to let me know it's ready for review again.

[Julesssss]

Other PR is now merged, removing the hold.

[kidroca]

Updated
@tgolen @Julesssss synced with master and resolved conflicts, addressed requested changes, this is ready for review

[kidroca]

Hey sorry I've just noticed but I need to also update the usage of the Attachment Picker on the ProfilePage:
https://github.com/Expensify/Expensify.cash/blob/5ab2683945ae684708bd3505a2810461a047e21e/src/pages/settings/ProfilePage/index.js#L196

        const menuItems = [
            {
                icon: Upload,
                text: 'Upload Photo',
                onSelected: () => {
                    setTimeout(() => {
                        openPicker({
                            onPicked: setAvatar,
                        });
                    }, 10);
                },
            },
        ];

I should remove the timeout and update the handling there as well

[kidroca]

Updated

When I originally added this logic, I tested on Android and I missed the fact that calling
openPicker before the first modal closes would not work on iOS native. on iOS the first
modal needs to close and only then you can call openPicker

But the tricky part is on mobile web and Safari invoking an action after the first modal
closes is ignored by the browser because the button that triggers input.click() is no longer in the DOM

• Details: We wait for the modal to hide and only then we trigger input.click(). This is a programatically triggered event. Such events are not welcomed warmly by the browser - imagine someone created a script that goes through all the links on a page and triggers click programmatically - it'll flood your browser. This is why browsers would only allow a programmatic click to happen in a small time frame after an actual user interaction

That's why I've added a configuration regarding how menu items should be triggered in CreateMenu

• on web/mWeb/desktop: menu item onSelected actions would be triggered before the modal closes
• on mobile native actions would be triggered after the menu modal closes (same as previous)
• no negative side effects or degradations noticed - tried all the places CreateMenu is used (settings/profile, FAB, chat compose)

[kidroca]

Updated

[Julesssss]

Would you mind reminding me why we need the native iOS menu here instead of our custom CreateMenu? Is this still the case for safari mobile web, even though we launch the next Modal before the first closes?

[kidroca]

Would you mind reminding me why we need the native iOS menu here instead of our custom CreateMenu?

You are seeing a screenshot of the second modal - the popup that appears after you select "Upload Photo" from the options in the CreateMenu. It's appearance is handled by the browser.
I didn't take a screenshot of the Create Menu as it only has 1 option at the moment
My screenshot captures the Menu that prior this PR would not appear on mobile web

Maybe I should add a video for clarity. I'll do that when I get the chance

[Julesssss]

Maybe I should add a video for clarity. I'll do that when I get the chance

No need, I'm with you. That's my screenshot btw.

Gotcha, so it's simply because we have more control on mobile (we can show document picker/image picker/camera). Whereas on web we delegate this to the browser, which displays the native context menu?

[kidroca]

Gotcha, so it's simply because we have more control on mobile (we can show document picker/image picker/camera). Whereas on web we delegate this to the browser, which displays the native context menu?

Correct

To completely cover the topic, there's an alternative:

You can have a <input type="file" capture="camera" /> which would directly start the camera. But this means we need to implement another 2nd modal with the options "Camera" and "Browse" -> camera would trigger the camera, while "Browse" would prompt you to select by popping yet another popup (the one in your screenshot). I guess you see how this would not be a great improvement.

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/capture

[kidroca]

@Julesssss @tgolen
Are you waiting for more changes? I don't think there's anything more I need to do here
@tgolen I've left this unresolved so you can verify the solution suits you: #2229 (comment)

[tgolen]

Thanks for the bump, I will get it reviewed again now!

[kidroca]

Updated (and merged latest from master)

[kidroca]

Updated

[kidroca]

Updated
Resolved merge conflicts

[kidroca]

The test stage fails at "Install cocoapods". There are no changes in package or pods in this PR though

[Julesssss]

LGTM. The test was failing due to a missing Airship dependency, which has now been resolved. After re-running the test it succeeded.