[material-ui][Dialog] Specifying onClick prop disables backdrop clicks

[ryanburr]

Steps to reproduce

Link to live example

Steps:

1. [done in code] Specify the onClick prop to the Dialog component.
2. Click the "Open Simple Dialog" button to open the dialog.
3. Try to click on the backdrop to close the dialog (nothing happens).

Current behavior

The dialog does not close upon clicking the backdrop if the onClick prop is provided by the consumer of the dialog.

Expected behavior

The dialog should close when the backdrop is clicked, even if an onClick prop is provided to the dialog component.

Context

• The onClick prop can been used to stop propagation to parent components, as mentioned in this older issue.
  • While the main point of that issue talks about the click event propagating to the parent (which I believe is working as intended), it also mentions that specifying the onClick callback also "... breaks the behavior of the 'backdrop'". I want to make sure this gets addressed.
• The workaround @kykungz mentioned here worked to bring back the backdrop behavior while using the onClick prop for v5.
• When looking at the Dialog source code, I noticed a few things:
  • The component handles the backdrop functionality through the onClick callback here.
  • If the consumer also specifies an onClick prop, it gets overwritten by spreading {...others} here.
  • My proposed fix for this would be to:
    • Destruct onClick from props.
    • Invoke it from within handleBackdropClick here (if provided in props) instead of grouping it with ...others.

Your environment

npx @mui/envinfo

  System:
    OS: macOS 14.2.1
  Browsers:
    Chrome: 122.0.6261.112
    Edge: Not Found
    Safari: 17.2.1
  npmPackages:
    @emotion/react: ^11.11.4 => 11.11.4 
    @emotion/styled: ^11.11.0 => 11.11.0 
    @mui/base:  5.0.0-beta.37 
    @mui/core-downloads-tracker:  5.15.11 
    @mui/icons-material: ^5.15.11 => 5.15.11 
    @mui/lab: ^5.0.0-alpha.166 => 5.0.0-alpha.166 
    @mui/material: ^5.15.11 => 5.15.11 
    @mui/private-theming:  5.15.11 
    @mui/styled-engine:  5.15.11 
    @mui/system:  5.15.11 
    @mui/types:  7.2.13 
    @mui/utils:  5.15.11 
    @mui/x-date-pickers: ^6.19.6 => 6.19.6 
    @types/react: ^18.2.61 => 18.2.61 
    react: ^18.2.0 => 18.2.0 
    react-dom: ^18.2.0 => 18.2.0 
    typescript: 4.9.5 => 4.9.5

Search keywords: dialog onClick backdrop

[ZeeshanTamboli]

I agree it's a bug.

[ZeeshanTamboli]

Invoke it from within handleBackdropClick here (if provided in props) instead of grouping it with ...others.

Where to call handleBackdropClick then?

[ZeeshanTamboli]

Would the following fix work:

--- a/packages/mui-material/src/Dialog/Dialog.js
+++ b/packages/mui-material/src/Dialog/Dialog.js
@@ -182,6 +182,7 @@ const Dialog = React.forwardRef(function Dialog(inProps, ref) {
     fullWidth = false,
     maxWidth = 'sm',
     onBackdropClick,
+    onClick,
     onClose,
     open,
     PaperComponent = Paper,
@@ -211,6 +212,9 @@ const Dialog = React.forwardRef(function Dialog(inProps, ref) {
     backdropClick.current = event.target === event.currentTarget;
   };
   const handleBackdropClick = (event) => {
+    if (onClick) {
+      onClick(event);
+    }
     // Ignore the events not coming from the "backdrop".
     if (!backdropClick.current) {
       return;

However, I believe there should be a distinction between clicking the dialog root and clicking the backdrop.

[ryanburr]

I believe so! That's pretty much what I was thinking. You don't even need to reorder onClick={handleBackdropClick} since the onClick prop won't be part of others anymore.

there should be a distinction between clicking the dialog root and clicking the backdrop.

I agree and if I understand the docs correctly, that's the use case that (now deprecated) onBackdropClick and onClose (with reason) handle.

[ZeeshanTamboli]

I believe so! That's pretty much what I was thinking. You don't even need to reorder onClick={handleBackdropClick} since the onClick prop won't be part of others anymore.

Correct. Updated the above diff.

there should be a distinction between clicking the dialog root and clicking the backdrop.

I agree and if I understand the docs correctly, that's the use case that (now deprecated) onBackdropClick and onClose (with reason) handle.

I mean why is the dialog root's onClick handling the backdrop's click event?

[ryanburr]

Ah I see what you're saying. Good question. Not sure of the design decision there. Found the commit that put it there though.

Would an alternative be to wire this up through the Backdrop onClick prop? I haven't explored this component much yet.

[asif-choudhari]

I believe so! That's pretty much what I was thinking. You don't even need to reorder onClick={handleBackdropClick} since the onClick prop won't be part of others anymore.

Correct. Updated the above diff.

there should be a distinction between clicking the dialog root and clicking the backdrop.

I agree and if I understand the docs correctly, that's the use case that (now deprecated) onBackdropClick and onClose (with reason) handle.

I mean why is the dialog root's onClick handling the backdrop's click event?

DialogRoot is a Styled Modal and the underlying Modal component has the Pop Up(Modal) and Backdrop component with it, By passing onClick to DialogRoot eventually to Backdrop, callback is fired back when Backdrop is clicked to close it.