[Dialog] Fix setting component prop does not apply styles to root

[ZeeshanTamboli]

• I have followed (at least) the PR section of the contributing guide.

Fixes #33709

Fix default styles were not getting applied to DialogRoot when component prop was passed in Dialog component.

It's because when the component prop is passed, the styled ModalRoot component was getting ignored. Instead, we always pass the ModalRoot to components.Root, in turn, applying the default styles and changing the underlying HTML element with the as prop.

Caused due to changes in #32901 since @mui/material version 5.8.5.

---

Before: https://codesandbox.io/s/gifted-haze-63ubtn?file=/src/App.tsx

After: https://codesandbox.io/s/morning-flower-s6z4iv?file=/src/App.tsx

Ignore the TypeScript error on component prop. That's another issue.

[mui-bot]

Details of bundle changes

Generated by 🚫 dangerJS against adb6d4e

[oliviertassinari]

I'm a bit out of the loop with the way props flows now, but I don't get why we would need this

Suggested change

const Root = components.Root ?? component;

Would this be enough?

diff --git a/packages/mui-material/src/Modal/Modal.js b/packages/mui-material/src/Modal/Modal.js
index d631549e94..714bc3e6ef 100644
--- a/packages/mui-material/src/Modal/Modal.js
+++ b/packages/mui-material/src/Modal/Modal.js
@@ -5,6 +5,7 @@ import { isHostComponent, resolveComponentProps } from '@mui/base/utils';
 import { elementAcceptingRef, HTMLElementType } from '@mui/utils';
 import styled from '../styles/styled';
 import useThemeProps from '../styles/useThemeProps';
+import shouldSpreadAdditionalProps from '../utils/shouldSpreadAdditionalProps';
 import Backdrop from '../Backdrop';

 export const modalClasses = modalUnstyledClasses;
@@ -102,8 +103,6 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {

   const classes = extendUtilityClasses(ownerState);

-  const Root = components.Root ?? component;
-
   return (
     <ModalUnstyled
       components={{
@@ -112,18 +111,17 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {
         ...components,
       }}
       componentsProps={{
-        root: () => ({
+        root: {
           ...resolveComponentProps(componentsProps.root, ownerState),
-          ...(!isHostComponent(Root) && { theme }),
-          /** We pass the 'as' prop always because components.Root is set as ModalRoot wrapped with the
-           * styled API which thus supports the 'as' prop for host or React components.
-           */
-          as: Root,
-        }),
-        backdrop: () => ({
+          ...(shouldSpreadAdditionalProps(components.Root) && {
+            as: component,
+            ownerState: { ...componentsProps.root?.ownerState, },
+          }),
+        },
+        backdrop: {
           ...BackdropProps,
           ...resolveComponentProps(componentsProps.backdrop, ownerState),
-        }),
+        },
       }}
       onTransitionEnter={() => setExited(false)}
       onTransitionExited={() => setExited(true)}

[ZeeshanTamboli]

I'm a bit out of the loop with the way props flows now, but I don't get why we would need this

A custom Root component can be passed through the slot or component prop.

Would this be enough?

It works, the current tests do pass with your suggested change. Although I am not sure about this line ownerState: { ...componentsProps.root?.ownerState, }, in the diff as componentsProps.root can be a callback or an object and whether useSlotProps inside ModalUnstyled already appends ownerState properly.

What's the advantage of this?

IMO, I would rather keep my approach as it is simpler and easy to read at a first glance.

Instead what I have done is remove the unnecessary function definition from componentsProps.root and componentsProps.backdrop in commit adb6d4e after seeing your suggested change.

[oliviertassinari]

What's the advantage of this?

I guess it's simpler and it matches how it's done in:

material-ui/packages/mui-material/src/Slider/Slider.js

Line 518 in dee9529

as: component,

AFAIK, we shouldn't provide components.Root for the as prop here because we already provide it to the components prop.

I even wonder if components.Root is not meant to replace the first level deep component, while the component prop is meant to replace the leaf component.

[ZeeshanTamboli]

I guess it's simpler and it matches how it's done in:

Okay but IMO, I think it is not simple compared to what I did. As for it being consistent with other components then what about the following line:

ownerState: { ...componentsProps.root?.ownerState, }

This is wrong as componentProps slots can be either a callback or an object as I said in the previous comment. If we keep it by also considering callback here, we need to change in other components later on.

components.Root is meant to replace the first level deep component, while the component prop is meant to replace the leaf component

Can you please elaborate on this? I think I understand what you mean but the as prop can also be used to pass custom React components. Also component prop can be used for React components, not only for overriding HTML elements (docs).

I guess we should wait for others reviews.

[michaldudak]

@oliviertassinari This matches the implementation of Base components. In this library component = components.Root. I don't understand what you mean by "components.Root is meant to replace the first level deep component, while the component prop is meant to replace the leaf component" - which leaf component exactly?

[michaldudak]

@ZeeshanTamboli, as discussed in #34334, the component prop should be passed into the as prop, while the components.Root should be passed into ModalUnstyled's components.Root. This way developers can either customize the whole slot or the ultimately rendered component

[ZeeshanTamboli]

Closing this as it got fixed in @mui/material version 5.10.11 with the renaming of components prop to slots prop.