Chat / The escape key should close open modals

[jboniface]

If you haven’t already, check out our contributing guidelines for onboarding!

---

Platform - version:
Desktop, build 280

Action Performed (reproducible steps):

Test 1

1. Click the paperclip icon
2. Select a file to upload
3. Before confirming the upload, press Escape
4. Observe that the upload modal stays on the screen

Test 2

1. Click the paperclip icon
2. Select an image to upload
3. Upload the file
4. Click into the image
5. Press escape
6. Observe that the attachment modal stays on the screen

Expected Result:
All modals should be cancelled when the user presses the Escape key.

Actual Result:
The modal stays on the screen

Notes/Photos/Videos:

view the job on Upwork here.

[laurenreidexpensify]

Have offered to two contributors in Upwork

[f0wu5u]

Just checked the AttachmentModal file and we can extend it with a key press listener (will only register for platforms that support key press in this case 'ESCAPE') and register on Modal mount and remove listener and clean up on Modal close or unmount.

Note that we will use a reusable helper object that will enable future key press listener registration on different components.

[henryrossiter]

I propose we use a key press listener as @code-fi proposed. However, I propose registering/deregistering the key press listener using functions passed to the onModalWillShow and onModalWillHide props (from the react-native-modal documentation).

This approach ensures the listener is only registered once the modal is opened. Assuming users won't attach files every single time they open the app, registering listeners as soon as the component is mounted may be unnecessary.

[henryrossiter]

I propose we use a key press listener as @code-fi proposed. However, I propose registering/deregistering the key press listener using functions passed to the onModalWillShow and onModalWillHide props (from the react-native-modal documentation).

This approach ensures the listener is only registered once the modal is opened. Assuming users won't attach files every single time they open the app, registering listeners as soon as the component is mounted may be unnecessary.

After testing multiple solutions, I found registering/deregistering the key press listener using onModalWillShow and onModalWillHide to be unpredictable - it often lead to duplicate event listener registration.

I registered the listeners during AttachmentModal component mount in PR 1160. I found no performance issues with this approach.

[marcaaron]

Thanks for the notes everyone!

we will use a reusable helper object that will enable future key press listener registration on different components

@code-fi I'm curious to learn more about what you meant by this. We have a key press library that can be used to bind keypresses https://github.com/Expensify/Expensify.cash/blob/master/src/libs/KeyboardShortcut/index.js can we use this? Or was there something else in mind?

This approach ensures the listener is only registered once the modal is opened

@henryrossiter I think it probably be fine to hook the listener into the component mounting/unmounting. We are using the library above to delegate events which I think just registers new callbacks instead of adding event listeners. So maybe it can help?

AttachmentModal file and we can extend it

registered the listeners during AttachmentModal component mount

I do think it would be nice to have this at the Modal level so we can have this work on all modals current and future. The Modal display state is determined by a parent component (this.props.isVisible), but we can use this callback function to set the modal closed.

https://github.com/Expensify/Expensify.cash/blob/27d5a6f4e9e22d07a97429b1984c8a08cd2e0d71/src/components/Modal.js#L14

So I would probably choose to place the logic here and have my keypress listener call props.onClose() instead of in AttachmentModal as this is likely to be a common and global action across the application. Thoughts?

[f0wu5u]

I'm curious to learn more about what you meant by this. We have a key press library that can be used to bind keypresses https://github.com/Expensify/Expensify.cash/blob/master/src/libs/KeyboardShortcut/index.js can we use this? Or was there something else in mind?

@marcaaron yes implementation will be similar to the KeyboardShortcut approach but it includes the following checks;

1. Ensure event registration is done only on platforms where document is available i.e not undefined; (Web and Desktop)
2. Extensible enough to handle special keys like ENTER, ESCAPE and the likes.. so I will add a property for registering event listener for ENTER and ESCAPE, this way anyone can add another property (method) that will listen to any key or use the default event listening method that accepts key and onPress callback. This method will also expose a default event listener removal method that will be fired on UNMOUNT.

[henryrossiter]

I do think it would be nice to have this at the Modal level so we can have this work on all modals current and future. The Modal display state is determined by a parent component (this.props.isVisible), but we can use this callback function to set the modal closed.

https://github.com/Expensify/Expensify.cash/blob/27d5a6f4e9e22d07a97429b1984c8a08cd2e0d71/src/components/Modal.js#L14

So I would probably choose to place the logic here and have my keypress listener call props.onClose() instead of in AttachmentModal as this is likely to be a common and global action across the application. Thoughts?

Yeah, this makes sense. I implemented the listener at the AttachmentModal level instead of Modal just to stay within the scope of this issue. I wasn't sure if the close-on-escape feature was desirable for all future implementations of the Modal component.

[marcaaron]

@code-fi

Ensure event registration is done only on platforms where document is available

If my understanding is correct, this is already happening in the noop implementation here https://github.com/Expensify/Expensify.cash/blob/master/src/libs/KeyboardShortcut/index.native.js

Extensible enough to handle special keys like ENTER, ESCAPE and the likes.. so I will add a property for registering event listener for ENTER and ESCAPE, this way anyone can add another property (method) that will listen to any key or use the default event listening method that accepts key and onPress callback.

Can we modify the existing KeyboardShortcut if it has no support for ESCAPE? I'm not sure if this is true or not, but that seems like it should be possible.

This method will also expose a default event listener removal method that will be fired on UNMOUNT.

So it does sound like this will be a component of some sort then? I think to keep this simple we may be better off just using the lifecycle methods instead of creating any helper components. While this is a good idea I'm not sure we need to do this yet.

[marcaaron]

implemented the listener at the AttachmentModal level instead of Modal just to stay within the scope of this issue

@henryrossiter Yep, apologies that could have been more clear on our part.

The only modals we have are the attachment ones right now, but it's in scope since the issue reads

All modals should be cancelled when the user presses the Escape key.

We'll be adding more modals soon so this would be great to tackle now if possible. Thanks!

[f0wu5u]

So it does sound like this will be a component of some sort then? I think to keep this simple we may be better off just using the lifecycle methods instead of creating any helper components. While this is a good idea I'm not sure we need to do this yet.

not a component but it works like the KeyboardShortcut property unsubscribe

[f0wu5u]

@marcaaron check this MR #1180

[isagoico]

Reopening this to add a new scenario that's not working

ESC doesn't close the new chat/new group modal

Expected result

Pressing the escape button should close the modal

Actual result

Escape button does nothing

Action Performed

1. Open chat app
2. Click on the + button
3. Press escape button

Platform

Issue is confirmed on:

Desktop ✔️

Build 1.0.22-1

Notes/Images/Video

---

From @quinthar: https://expensify.slack.com/archives/C01GTK53T8Q/p1618952591439200

ISSUES: When you click "New Chat" or "New Group" on desktop web, then press Esc, nothing happens -- I would expect that dialog to close.

[marcaaron]

Grabbing this one as it has not been triaged yet.