event.stopPropagation() on a text input inside of a dialog should prevent the dialog from closing

[stevemao]

I have a <input /> and when I press ESC key, I want to restore the value and blur the input. The input is inside of a Dialog. So I don't want the dialog to close when i hit ESC on the input. event.stopPropagation() in onKeyDown of the button should stop it since it's a child a Dialog.

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

It should not close the dialog

Current Behavior

Steps to Reproduce

Link:

Context

Your Environment

Tech	Version
Material-UI	^3.3.2
React	^16.6.0
Browser	Version 70.0.3538.77 (Official Build) (64-bit)
TypeScript	Babel
etc.

[oliviertassinari]

@stevemao This should already be taken care of by our component:

material-ui/packages/material-ui/src/Modal/Modal.js

Line 162 in f5b38db

if (keycode(event) !== 'esc' || !this.isTopModal() || event.defaultPrevented) {

Bootstrap and react-modal are not event checking it. I doubt we can do anything about it. Do you have a reproduction example? Why don't you preventDefault or stop the event propagation on the keydown?

[stevemao]

I could do preventDefault() conditionally on ESC pressed down I guess. I did try stop the event propagation by calling event.stopPropagation() (I meant onKeyDown in my previous comment not onClick)... doesn't seem to work. https://stackoverflow.com/questions/24415631/reactjs-syntheticevent-stoppropagation-only-works-with-react-events might be the reason. I'm wondering why don't we just add onKeyDown on the Dialog instead of a real event on window.document? So that this event becomes a react synthetic event.

[oliviertassinari]

I'm wondering why don't we just add onKeyDown on the Dialog instead of a real event on window.document? So that this event becomes a react synthetic event.

@stevemao You are right, it sounds like a good path forward. Do you want to try? :)

[stevemao]

Yeah sure :) but no timeframe promised :)

[stevemao]

Just to be sure. This will be a breaking change. event.defaultPrevented check should be removed too since it's not a default browser behaviour.

[oliviertassinari]

This will be a breaking change.

How is this a breaking change? It's should definitely not be released in a patch.

it's not a default browser behaviour.

What do you mean by it's not a default browser behavior?
https://developer.mozilla.org/en-US/docs/Web/API/Event/defaultPrevented

[stevemao]

What do you mean by it's not a default browser behavior?

event.preventDefault() is meant to stop default behaviours like 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 these special HTML elements have these default bahaviours. I only call event.preventDefault() to stop these default browser behaviours so I don't think event.defaultPrevented is the right flag to check here. Problem for me is I have a selectable in the dialog and if I hit ESC, I want to discard the changes but I don't want to close the dialog. If I do event.preventDefault(), hitting arrow keys won't change the cursor in the selectable text field. Only way I could do is to check if (event.key === 'ESC') event.preventDefault() but it's not ideal. It should be just to stop the event bubbling.

How is this a breaking change? It's should definitely not be released in a patch.

For those who call event.stopPropagation() in a Dialog it won't stop closing the dialog by hitting ESC, but it will after this is implemented

For those who call preventDefault() to prevent closing the dialog by hitting ESC, after this, it won't.

[oliviertassinari]

@stevemao This makes a lot of sense. One could argue the opposite, I think that it's a matter of interpretation of what a default behavior is and if it can extend to non-native elements. However, considering none of the two implementations I have linked do it, I'm happy to remove this logic.

I have also notice that Bootstrap calls prevent default after handling the escape event while react-modal calls stop propagation. I know that reactstrap and react-overlay does nothing about it. We might want to arbitrate on that point. What do you think?

Do you think we should release that change in tha minor or a major? It can be considered a bug fix.

[stevemao]

We might want to arbitrate on that point.

Yeah 👍

Do you think we should release that change in tha minor or a major? It can be considered a bug fix.

If you are paranoid/conservative (which is what a lot of people do nowadays) then it it'll be a major. But I doubt if a lot of people are using event.preventDefault() and event.stopPropagation() and since this behaviour is not documented

[CoachThomLamb]

I am going to take a look at this and see about submitting a fix.