[Dialog] Remove disableEscapeKeyDown prop, in favor of using the onClose callback

[nzayatz14]

Is there any particular reason that the disableBackdropClick prop is deprecated on the Dialog component but the disableEscapeKeyDown prop is not?

I feel like they should either both be deprecated and handled the same way via the handleClose function, or neither should be deprecated.

Docs (v4.12.1) are currently showing:

[michal-perlakowski]

From the pull request that removed disableBackdropClick:

disableEscapeKeyDown is on the chopping block as well but will be handed separately.

[nzayatz14]

Ok sounds good, just wanted to make sure things were remaining consistent

[mnajdova]

We will probably need to move this to v6. The reason is that the property still exists in v5, and we wouldn't like to introduce more braking changes now that we are in beta. Thanks for the report @nzayatz14

[eps1lon]

This is fine for v5. Otherwise the API just looks weird as mentioned in the issue description. I just forgot to do it.

[eps1lon]

On the other hand, Modal also does stop event propagation which is a bit scary to change. Let's do this in v6.

[oliviertassinari]

Is there any particular reason that the disableBackdropClick prop is deprecated on the Dialog component but the disableEscapeKeyDown prop is not?

@nzayatz14 Yes. It's similar to the other events but not identical. The motivation to keep it was to kill two birds (not call on click + call event.stopPropagation()) in one stone (disableEscapeKeyDown). It's meant as a helper with a pit of success. If we remove it, developers would have to not forget to call event.stopPropagation(), they will get bad surprises otherwise.

[eps1lon]

If we remove it, developers would have to not forget to call event.stopPropagation(), they will get bad surprises otherwise.

This is misleading.

[oliviertassinari]

As far as I know, we should close this issue. We don't yet have a case around the problem that we would fix. The concern around API consistency was addressed by highlighting that the props have a different behavior.

---

This is misleading.

@eps1lon Happy to have a longer conversation about it. I guess there are multiple different ways to see this problem.

1. stopPropagation vs. preventDefault. react-modal uses stopPropagation while bootstrap uses preventDefault. Who is right?

In #14991, we use the feedback in #13470 (comment) that makes the case for only using preventDefault for preventing default user-agent behaviors.

This article https://css-tricks.com/dangers-stopping-event-propagation/ makes the case for avoiding stopPropagation unless we absolutely want no other parent event handlers to know about the events. It's why it call stopPropagation in the Autocomplete because it strongly assumes that nobody else should handle the event.
https://github.com/mui-org/material-ui/blob/913461139a7a275230bf4e960064646bb90ccc54/packages/material-ui/src/useAutocomplete/useAutocomplete.js#L778-L785

2. should the modal manipulate the event with its default props? It currently does, hence why we need disableEscapeKeyDown to disable the built-in behavior. This answers the question of @nazartokar:
   "Is there any particular reason that the disableBackdropClick prop is deprecated on the Dialog component but the disableEscapeKeyDown prop is not?"

[oliviertassinari]

Regarding how we manage the product. Up until now, as the owner of it, I have used the milestones to include problems to fix in order to reach a specific larger goal. So I have removed the v6 milestone from this issue because we haven't yet a clear consensus on the direction the product should take on this one.

Now, we could act on a different way to manage milestones, by including issues that have been less processed. Meaning we would including potential opportunities, even if half never get into the final shipped outcome. It would be a different tradeoffs. The advantage is that it avoid us to forgot about something, and allows for more exploration, the disadvantage is that we might end up with never ending milestones. With the latter tradeoff, we can use the "discussion" label to signal that we haven't yet dive enough into the problem, and the product owner would have to be conscious about not making the scope of the milestones grow forever, removing issues when needed.

[joelbourbon]

Hey guys, just wanted to bring up that deprecating those fields makes it kindof impossible out of the box to disableBackdropClick or disableEscapeKeyDown at large in createTheme.

Since it is not an actual event received in onClose you cannot event overload it generally and stopPropagation or preventDefault or whatever.

I would love to bring a solution to the problem, but this is over my head 😅

[teaganga]

I document how I migrated here: https://dev.to/teaganga/handle-dialogdisablebackdropclick-descoped-property-warning-in-mui-v5-2i35