Skip to content

[fix] Modal: close only one modal per Escape key press - #2862

Open
vmanapat wants to merge 1 commit into
necolas:masterfrom
vmanapat:fix-modal-escape-keydown
Open

vmanapat wants to merge 1 commit into
necolas:masterfrom
vmanapat:fix-modal-escape-keydown

Conversation

@vmanapat

Copy link
Copy Markdown

When one dialog is open on top of another, pressing Escape once closes both instead of just the top one.

Test case: https://codesandbox.io/s/t85xr3 (Open settings, then Delete deck, then press Escape once. Both modals close.)

Change

A modal now closes on Escape keyup only when the same key press began while it was the active modal and its keydown was not handled (defaultPrevented). The keydown is recorded in the capture phase, so it is still seen when a focused TextInput stops its propagation. Closing stays on keyup, so:

  • Escape from a focused TextInput still closes the modal
  • holding Escape closes one modal, not one per key repeat
  • a key press that began before the modal became active does not close it

Tests

The existing Escape tests now send keydown followed by keyup, as a real key press does. New tests cover stacked modals with and without the app handling Escape, a focused TextInput, holding Escape, a handled keydown, and a key press that began before the modal was active.

npm run format, npm run lint and npm run unit pass. npm run flow reports no new errors.

When the app closed the top modal on Escape keydown, the keyup of the same
key press closed the modal underneath as well. A modal now closes only for
a key press that began while it was the active modal and whose keydown was
not handled.
@codesandbox-ci

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 95e2d84:

Sandbox Source
react-native-web-examples Configuration
rnw-modal-escape PR

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant