[$1000] Remove Safari workaround to fix attachment picker issue

[dangrous]

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

---

In #19011 we added in a workaround for Safari's specific behavior for pop-ups and actions, because they always have to be triggered by a user action. This works, but is probably not ideal. Let's figure out a way to improve it!

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ce38317e6e2f0866
• Upwork Job ID: 1689306743240826880
• Last Price Increase: 2023-08-16

[melvin-bot]

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Current assignee @twisterdotcom is eligible for the NewFeature assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

[twisterdotcom]

Doesn't need designing.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01ce38317e6e2f0866

[melvin-bot]

Current assignee @twisterdotcom is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[alex2bc]

Proposal

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

Remove Safari workaround that enables file picker dialog to be opened.

What is the root cause of that problem?

A workaround was added because Safari allows popups, like the file input dialog, to be opened programmatically only when triggered directly by a user action.

The problem lies in the PopoverMenu component. The onItemSelected function is invoked immediately after the user's action, which is why the hack uses it as an alternative. However, onSelected is only triggered as a result of the onModalHide callback. Therefore Safari blocks file input popup.

App/src/components/PopoverMenu/index.js

Lines 79 to 85 in c74032f

	onModalHide={() => {
	setFocusedIndex(-1);
	if (selectedItemIndex !== null) {
	props.menuItems[selectedItemIndex].onSelected();
	setSelectedItemIndex(null);
	}
	}}

Before jumping to solution i would like to highlight that current PopoverMenu may be a bit unpredictable. The onSelected callback will not be triggered when an item is selected unless the isVisible prop is set to false within the onItemSelected function. This is currently being done in every usage of PopoverMenu component, so no issue has been noticed yet.

Why so confusing logic with onSelected prop in PopoverMenu?

It fixed the unresponsiveness of buttons in PopoverMenu on iOS.

Here is a PR where this logic was introduced and this is the issue it was solving.

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

I suggest reverting the changes made in #19339. This will allow us to remove the workaround immediately, but it will also bring back the issue of unresponsive buttons.
To fix the that we can use InteractionManager.runAfterInteractions in selectItem instead of calling onSelected in onModalHide.

App/src/components/PopoverMenu/index.js

Lines 54 to 58 in c74032f

	const selectItem = (index) => {
	const selectedItem = props.menuItems[index];
	props.onItemSelected(selectedItem, index);
	setSelectedItemIndex(index);
	};

Will result in

    const selectItem = (index) => {
        const selectedItem = props.menuItems[index];
        props.onItemSelected(selectedItem, index);
        InteractionManager.runAfterInteractions(() => {
            selectedItem.onSelected();
        });
    };

I've tested on iOS managed to reproduce issue with unresponsive buttons and runAfterInteractions fixes it.

@dangrous Please let me know your thoughts and if there's anything else I can add. 😉

[jeet-dhandha]

Proposal

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

• To remove work around for the safari browser's issue with Open Attachment Picker

What is the root cause of that problem?

• As explained by @allroundexperts the picker only open's by user's interaction. And not on call of onSelected in onModalHide.
• We want to simplyfy the code to not use index and find more of a generic solution.

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

Solution 1:

• We can use InteractionManager for IOS and Safari browser to run the onSelected method after the interaction is completed.

if (getOperatingSystem() === CONST.OS.IOS || Browser.isSafari()) {
    InteractionManager.runAfterInteractions(() => {
        selectedItem.onSelected();
    });
} else {
    selectedItem.onSelected();
}

Pros for this solution:

• It is a generic solution and onSelected method is handled only in one place.

Cons for this solution:

• It increases execution time by 40-60 ms.

Solution 2:

• We can pass a boolean variable triggerOnItemSelected with onSelected prop to menuItems.
  triggerOnItemSelected: getOperatingSystem() === CONST.OS.IOS || Browser.isSafari().

• Original Code
  

  App/src/pages/home/report/ReportActionCompose.js

  Lines 1032 to 1045 in d0b4fc2

  	const menuItems = [
  	...this.getMoneyRequestOptions(reportParticipants),
  	...this.getTaskOption(),
  	{
  	icon: Expensicons.Paperclip,
  	text: this.props.translate('reportActionCompose.addAttachment'),
  	onSelected: () => {
  	if (Browser.isSafari()) {
  	return;
  	}
  	triggerAttachmentPicker();
  	},
  	},
  	];

• Updated code

const menuItems = [
    ...this.getMoneyRequestOptions(reportParticipants),
    ...this.getTaskOption(),
    {
        icon: Expensicons.Paperclip,
+       triggerOnItemSelected: getOperatingSystem() === CONST.OS.IOS || Browser.isSafari(), // Checking for 2 platforms as both needs to be handled this way.
        text: this.props.translate('reportActionCompose.addAttachment'),
        onSelected: () => {
            if (Browser.isSafari()) {
                return;
            }
            triggerAttachmentPicker();
        },
    },
];

• And then in onModalHide we can check if triggerOnItemSelected is true then we call onSelected from selectItem method.

• Original Code
  

  App/src/components/PopoverMenu/index.js

  Lines 54 to 58 in d0b4fc2

  	const selectItem = (index) => {
  	const selectedItem = props.menuItems[index];
  	props.onItemSelected(selectedItem, index);
  	setSelectedItemIndex(index);
  	};

• Updated Code

const selectItem = (index) => {
        const selectedItem = props.menuItems[index];
        props.onItemSelected(selectedItem, index);
+       if (selectedItem.triggerOnItemSelected) {
+           selectedItem.onSelected();
+       }
        setSelectedItemIndex(index);
};

• Original Code
  

  App/src/components/PopoverMenu/index.js

  Lines 79 to 85 in d0b4fc2

  	onModalHide={() => {
  	setFocusedIndex(-1);
  	if (selectedItemIndex !== null) {
  	props.menuItems[selectedItemIndex].onSelected();
  	setSelectedItemIndex(null);
  	}
  	}}

• Updated Code

<PopoverWithMeasuredContent
    anchorPosition={props.anchorPosition}
    anchorRef={props.anchorRef}
    anchorAlignment={props.anchorAlignment}
    onClose={props.onClose}
    isVisible={props.isVisible}
    onModalHide={() => {
        setFocusedIndex(-1);
-       if (selectedItemIndex !== null) {
+       if (selectedItemIndex !== null && !props.menuItems[selectedItemIndex].triggerOnItemSelected) {
            props.menuItems[selectedItemIndex].onSelected();
            setSelectedItemIndex(null);
        }
    }}

Pros for this solution:

• Execution time remains same as we are just adding a check.

Cons for this solution:

• We have to add triggerOnItemSelected to all the menuItems having openPicker method call and also we will have to maintain it for future openPicker calls.

What alternative solutions did you explore? (Optional)

• N/A

[twisterdotcom]

Are these proposals any good @sobitneupane?

[sobitneupane]

Thanks for the proposal @alex2bc.

The onItemSelected function is invoked immediately after the user's action, which is why the hack uses it as an alternative. However, onSelected is only triggered as a result of the onModalHide callback. Therefore Safari blocks file input popup.

I suggest reverting the changes made in #19339. This will allow us to remove the workaround immediately, but it will also bring back the issue of unresponsive buttons.

I agree with your reasoning for reverting the changes made in #19339.

That will left us with #19310 issue. Can you please explain more about the issue. What's the cause of the issue? Using runAfterInteractions looks like a workaround.

[sobitneupane]

Thanks for the proposal @jeet-dhandha.

Can you please add more details in the Root Cause Analysis.

Solution 1 looks similar to this proposal. I didn't get your second proposal. Can you please add more details. Please make use of permalinks to refer to the files and code blocks where you are suggesting changes.

[jeet-dhandha]

Added few of the code samples. @sobitneupane Updated Proposal - #24297 (comment)

[alex2bc]

@sobitneupane It's the issue of react-native-modal library. When 2 modal are rendered at the same time on iOS/iPad it freezes, so we need to show new modal only when previous is hidden. More on that issue

Currently, there is no non-workaround fix available. Some solutions suggested by community:

1. Use presentationStyle="fullScreen". This doesn't work in our case
2. Do not use isVisible from Modal, like so:

-<Modal isVisible={isVisible} />
+{isVisible ? <Modal/> : null}

But it may affect performance of all modal in the app, so I don't think this is a good idea to change base modal rendering

I agree that runAfterInteractions looks a bit like workaround but I think here is the most correct one.

• it mimics the current logic of the app without drastic changes
• it removes desired Safari workaround

Please let me know your thought on that

[melvin-bot]

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

[sobitneupane]

Thanks for the update @jeet-dhandha.

But probably, your solution will resurface #19310 issue. And also, it looks like a workaround.

[sobitneupane]

Thanks for the update @alex2bc.

As @alex2bc suggested here, reverting #19339 PR will solve the issue in Safari and we can remove the workaround. But, it will recreate another issue. Looks like we don't have any way of solving the issue, without using some workaround like InteractionManager.runAfterInteractions.

What's your thought on this @dangrous?

[dangrous]

Okay, if all of our possibilities are really just replacing one workaround with another (which I understand), I think we might be okay to just close this issue, and not complete. What do you think?

[alex2bc]

I'd say that with this PR, we can replace two workarounds, one of which causes the other, with a single, more appropriate one for this problem

[sobitneupane]

Okay, if all of our possibilities are really just replacing one workaround with another (which I understand), I think we might be okay to just close this issue, and not complete. What do you think?

We will be replacing workarounds with some other workaround. So, I agree on closing the issue.

[twisterdotcom]

Apologies but I am finally going on Parental Leave. I am reassigning to another member of the team. But I also agree, we should close.

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @dannymcclain (NewFeature)

[dangrous]

Thanks! closing makes sense.

[jeet-dhandha]

Thanks for the update @jeet-dhandha.

But probably, your solution will resurface #19310 issue. And also, it looks like a workaround.

I already tested this it works 👍 fine.