[HOLD for payment 2021-10-30] Esc key doesn't dismiss any of the ConfirmModal around the app

[isagoico]

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

---

Action Performed:

1. Navigate to a user.
2. Add an attachment more than 50 MB.
3. Expect "attachment too large" warning to pop up.
4. Press Escape key.

Expected Result:

Escape should close the ConfirmModal

Actual Result:

Escape doesn't do anything.

Workaround:

User has to close the modal manually

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.0.95-0

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

Notes/Photos/Videos: Any additional supporting documentation

Upwork Job URL: https://www.upwork.com/jobs/~01ae467f5cc80cb019

Issue reported by: @rushatgabhane
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1631136254079800

View all open jobs on GitHub

[MelvinBot]

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

[parasharrajat]

Sorry for pitching in but IMO ConfirmModal should not be escaped. Their purpose is to be confirmed by the user so whatever action is mentioned on the modal should be confirmed by the user.

In this case User should click close. In other cases for example when money is transferred and the user is asked to confirm something, he should choose among the presented buttons.

---

Or we can change this behavior. 😸

[TomatoToaster]

Hmm I'm personally fine with it escaping the modal with escape key. Let's ping the room and see if we can get a larger consensus on this before implementing it.

[TomatoToaster]

Ok I think the accessibility case (mentioned in slack) is a good enough case. I'm going to make this external officially.

[MelvinBot]

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

Triggered auto assignment to @Luke9389 (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[parasharrajat]

Proposal

1. For the specific case of AttachmentModal we just need to pass the onCancel Callback which is called on the press of ESC.
   

   App/src/components/AttachmentModal.js

   Line 160 in 01f351c

   onConfirm={this.closeConfirmModal}

                    onCancel={this.closeConfirmModal}

2. For other confimModals, we can quickly run a search to find all instances of ConfirmModal and attach the callback to reset the isVisible prop of ConfirmModal to its onCancal Callback.

[rushatgabhane]

onCancel={this.closeConfirmModal}

@parasharrajat you wanna pass an additional prop to ConfirmModal? But that'd also add an extra button which isn't needed.

Also, we'll have to define and pass a callback everytime we use ConfirmModal. This is not very "DRY".

Do you think we can do better? So that this behaviour is built-in for the component?

Maybe something with the help of navigation.

[parasharrajat]

Ahh, I didn't see it through that it will render the button. Absolutely, we can inbuild it in the ConfirmModal if we just want to hide every ConfirmModal. I will update the Proposal to add that as well.

[parasharrajat]

I just checked onCancel does not show an extra button until shouldShowCancelButton is true but You made a good point that we can just push this code inside the ConfirmModal.

[rushatgabhane]

I just checked onCancel does not show an extra button until shouldShowCancelButton is true but You made a good point that we can just push this code inside the ConfirmModal.

Ohh okay, sounds good!

[parasharrajat]

If I do this:

1. If we want to close all confirmModals when Esc is pressed and don't want to manage it from the Parent Component. Then we can

   a. Add a state property isVisible and using componentDidupdate, Update this state property when isVisible prop changes.

   b. Then set the isVisible prop of Modal component to this.state.isVisible and set this state prop to false on onClose callback.

    <Modal
        onClose={() => {
			this.setState({isVisible: false});
            props.onCancel();
        });
        isVisible={this.state.isVisible}
        type={props.isSmallScreenWidth
            ? CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED
            : CONST.MODAL.MODAL_TYPE.CONFIRM}
    >

---

There is an issue with this @rushatgabhane. As ConfirmModal is controlled by the isVisible state of the parent component, we have to set it to false whenever the modal is closed. Otherwise, next time, ConfirmModal will not be shown as isVisible was already true.

e.g.

 onCancel={this.closeConfirmModal}

is necessary.

Then we still need to pass a callback to reset the parent Component state. Thus my first proposal stands correct.

Can you think of a way to mitigate this? If so, let me know.

[rushatgabhane]

@parasharrajat I get why it's a problem. And I don't have a better solution.

I'll throw some ideas, but I don't want to waste your time.

Umm some clever design pattern could do the trick.
Or, instead of using a prop to make it visible, navigate to the modal so you can "go back".
Onyx?

[Luke9389]

We will loop back around to this after the n6-hold is lifted

[trjExpensify]

Hired.

[trjExpensify]

Not overdue. Awaiting deploy to prod.

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.8-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Added cancel on ESC to the ConfirmModal #5741

If no regressions arise, payment will be issued on 2021-10-30. 🎊

[rushatgabhane]

Just curious, do I have to apply somewhere for issue reporting bonus?

[Luke9389]

Just curious, do I have to apply somewhere for issue reporting bonus?

@rushatgabhane Nope! We'll pay you when we pay the author of the PR. I think there's been a slight hold up here because @trjExpensify (the Contributor Manager on this issue) has been Out Of Office. He comes back tomorrow, and I'm sure the payment will be processed at that time. Sorry for the delay!

[laurenreidexpensify]

@parasharrajat you're now paid - thanks for patience - Tom was taking some well deserved OOO and this slipped through the net!

@rushatgabhane i've hired you for the job in upwork, as soon you accept I'll end the contract and get you paid tomorrow. Thanks!

[laurenreidexpensify]

@rushatgabhane paid! Thanks!

[Luke9389]

@laurenreidexpensify to the rescue! Thanks!

[rushatgabhane]

@laurenreidexpensify Appreciate it! Thank you!

[parasharrajat]

Hey, @laurenreidexpensify I was expecting the N6 bonus.

[laurenreidexpensify]

Ah sorry Rajat - will hook you up now!

[laurenreidexpensify]

Done!