[HOLD for payment 2021-11-04] Pressing Escape on the confirmation modal while removing a Member closes both modals - @parasharrajat

[isagoico]

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

---

Action Performed:

1. Navigate to Workspace settings
2. Navigate to members
3. Check one from the list and click on remove
4. Once the remove modal appears, press the esc button

Expected Result:

Only the remove modal should be dismissed

Actual Result:

Both the remove modal and the right panel are dismissed

Workaround:

Don't press the escape button in the modal.

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.1.7-3

Reproducible in staging?: Yes
Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

Recording.224.mp4

Expensify/Expensify Issue URL:

Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1633962922114200

View all open jobs on GitHub

[MelvinBot]

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

[parasharrajat]

Proposal

I tried many ways to intercept the Escape key but Modal onBackButtonPress which handles the escape key does not pass the event to the callback.

App/src/components/Modal/BaseModal.js

Lines 77 to 79 in 7fac08c

	// Note: Escape key on web/desktop will trigger onBackButtonPress callback
	// eslint-disable-next-line react/jsx-props-no-multi-spaces
	onBackButtonPress={this.props.onClose}

So the best solution so far is to Block the close of a page when Modal is shown over the screen. which should be the case. For example, on any screen if a user has done some action and a confirmation modal is shown then escape key should only interact with that Modal not other parts of the app.

Solution

1. We will block the screen dismiss at

   App/src/components/ScreenWrapper.js

   Line 59 in 7fac08c

   Navigation.dismissModal();

 this.unsubscribeEscapeKey = KeyboardShortcut.subscribe('Escape', () => {
            if (!this.props.modal.isVisible) {
                Navigation.dismissModal();
            }
        }, [], true);

2. Modal visibility can be taken from Onyx.

 withOnyx({
        modal: {
            key: ONYXKEYS.MODAL,
        },
    }),

[MelvinBot]

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

[kadiealexander]

Upwork job: https://www.upwork.com/jobs/~017d9eada7d9235f63

[MelvinBot]

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

[thorep]

I dont know what css framework you are using, but a lot of them don’t allow stacked modals. Workaround is to wrap the modals and disable keyboard input, input will then be handled individually by the modals. Don’t have an up work account I’m afraid :/

[ThadeusAjayi]

Proposed Fix:

Have a ref for each of the modals to track the active/topmost modal, listen to keyboard event on the escape key and dismiss modal based on the most recent modal ref.

[Julesssss]

Thanks for the proposals!

I prefer @parasharrajat's solution, as it's simpler and more self-contained than tracking recent modal state. Would you like to go ahead with this?

---

This is out of scope for now, but it made me wonder if it would be worthwhile to give all pages/Modals the ability to be lifecycle aware. Having callbacks for becoming active (top-level) and inactive (when another Modal/Page is opened on top) would allow us to be subscribed to the Escape key ONLY when it should be closable.

[parasharrajat]

So.

1. When this code is merged, Modal Screens will not close until all the modals over are closed. and Modals are always top-level when Modal Screen is active.
2. If we have multiple modals over the screen, then the Escape key will close the top-level only and thus close all one by one on each keypress.

it means we don't need to handle anything for now. @Julesssss

[kadiealexander]

Deployed to staging, waiting for production deployment. Begone, Melvin.

[botify]

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

• Fix: Modal and screen closing with single esacpe keypress #5889

If no regressions arise, payment will be issued on 2021-11-04. 🎊

[kadiealexander]

@parasharrajat sorry I hadn't sent the Upwork hire! Please let me know when this is accepted and I'll pay it out asap. :)

[parasharrajat]

@kadiealexander Done.

[parasharrajat]

Ping for

[mallenexpensify]

Paid @parasharrajat , inc. reporting and n6-hold bonus

[kadiealexander]

Thanks for grabbing this on my weekend @mallenexpensify!!