[Modal] Should not call stopPropagation if onEscapeKeyDown=true

[mainfraame]

I'm running into an issue where I need to handle the event propagation for the escape key on my own. It's impossible to get around because it's hard-coded in the handleKeyDown callback.

To be specific, I have a Popper rendered (via portal) over the modal, and when a user hits the Escape key, I want to close the Popper, but not the modal.

• [-] The issue is present in the latest release. (not sure; cannot upgrade)
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When passing onEscapeKeyDown and disableEscapeKeyDown props to Modal, I do not want the event.stopPropagation to be called.

Instead of allowing the onEscapeKeyDown to handle the event propagation; the escape keydown event's propagation is stopped.

Expected Behavior 🤔

When modal is provided onEscapeKeyDown, a develop should expect to handle all aspects of the event. What also might be nice is if the modal manager is passed as a second parameter to the callback.

Tech	Version
Material-UI	v4.4.3

[oliviertassinari]

@mentierd It seems that you are right, the current logic doesn't match the props' descriptions. What do you think of this diff? Do you want to submit a pull request? :)

diff --git a/packages/material-ui/src/Modal/Modal.js b/packages/material-ui/src/Modal/Modal.js
index badf67093..dc8eb6cee 100644
--- a/packages/material-ui/src/Modal/Modal.js
+++ b/packages/material-ui/src/Modal/Modal.js
@@ -189,7 +189,7 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {
     // clicking a checkbox to check it, hitting a button to submit a form,
     // and hitting left arrow to move the cursor in a text input etc.
     // Only special HTML elements have these default behaviors.
-    if (event.key !== 'Escape' || !isTopModal()) {
+    if (event.key !== 'Escape' || !isTopModal() || disableEscapeKeyDown) {
       return;
     }

@@ -200,7 +200,7 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {
       onEscapeKeyDown(event);
     }

-    if (!disableEscapeKeyDown && onClose) {
+    if (onClose) {
       onClose(event, 'escapeKeyDown');
     }
   };

For curiosity, I have looked at how the problem is handled in a couple of alternatives:

• https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/js/dialog.js
• https://github.com/twbs/bootstrap/blob/a8883a3b96a94d9c096d1047564eeb87de7e79e7/js/src/modal.js#L301
• https://github.com/reactjs/react-modal/blob/5189e5b6928319a831edad27cecd964ebd59ec33/src/components/ModalPortal.js#L266
• https://github.com/reach/reach-ui/blob/d5c59b90c6d32c7640d7ae1e6a40ab92ef033800/packages/dialog/src/index.tsx#L118

[matthewbal]

I'd do it if @mentierd is too busy this weekend 😀

[weslenng]

@oliviertassinari I tried to implement your solution but one test fail:

it('when disableEscapeKeyDown should call only onEscapeKeyDownSpy', () => {

What do you think about this diff?

diff --git a/packages/material-ui/src/Modal/Modal.js b/packages/material-ui/src/Modal/Modal.js
index 1c38d39c4..748a70ec1 100644
--- a/packages/material-ui/src/Modal/Modal.js
+++ b/packages/material-ui/src/Modal/Modal.js
@@ -193,11 +193,11 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {
       return;
     }
 
-    // Swallow the event, in case someone is listening for the escape key on the body.
-    event.stopPropagation();
-
     if (onEscapeKeyDown) {
       onEscapeKeyDown(event);
+    } else {
+      // Swallow the event, in case someone is listening for the escape key on the body.
+      event.stopPropagation();
     }
 
     if (!disableEscapeKeyDown && onClose) {

[oliviertassinari]

@weslenng The test goes against the documentation: If `true`, hitting escape will not fire any callback but it's the same issue with disableBackdropClick. Ok, I think that your alternative proposal is great.

For v5, I think that we can drop disableBackdropClick, onBackdropClick, onEscapeKeyDown, disableEscapeKeyDown. onChange + reason + filtering should be enough.