material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Dialog] DialogTitle overrides DialogContent className

Open jlin5 opened this issue 3 years ago • 21 comments

  • [x] The issue is present in the latest release.
  • [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The DialogTitle className (MuiDialogTitle-root) is overriding the set className to DialogContent.

Expected Behavior 🤔

The DialogTitle className should not override the set className to DialogContent.

Steps to Reproduce 🕹

https://codesandbox.io/s/sparkling-architecture-47w73?file=/src/index.tsx

This is happening after @material-ui/[email protected]

To reproduce, change the package dependency in the codepen @material-ui/core to 5.0.0-alpha.36 or higher.

jlin5 avatar Jun 16 '21 16:06 jlin5

The reason is the breaking change introduced by https://github.com/mui-org/material-ui/pull/26323. @eps1lon should we improve the description of the breaking change? I think this is a corner case, but as it already came up as an issue maybe we should mention the bigger specificity that is introduced in the styles.

mnajdova avatar Jun 17 '21 11:06 mnajdova

I didn't make that particular change nor do I think it should stay. It makes customization not straight forward. You'd have to ask @oliviertassinari what problem the sibling selector solves and what alternatives he tried.

eps1lon avatar Jun 17 '21 11:06 eps1lon

The biggest constraint I had was the lack of support for &:first-child that emotion sets on us. We moved to &:first-of-type during the migration from JSS to emotion. But now, with a flat header, we can no longer rely on &:first-of-type either. The DOM structure is

  • div (MuiDialog-root)
    • h2 (MuiDialogTitle-root)
    • div (MuiDialogContent-root)

The least worse solution I could find is: https://github.com/mui-org/material-ui/blob/e8dd87d14f9d64719faaca9c8ff8c97e943262e3/packages/material-ui/src/DialogContent/DialogContent.js#L43

oliviertassinari avatar Jun 17 '21 23:06 oliviertassinari

For the solution, an update of the docs like proposed by @mnajdova sounds great 👍

diff --git a/docs/src/pages/guides/migration-v4/migration-v4.md b/docs/src/pages/guides/migration-v4/migration-v4.md
index b66b55e80d..2d8629a8be 100644
--- a/docs/src/pages/guides/migration-v4/migration-v4.md
+++ b/docs/src/pages/guides/migration-v4/migration-v4.md
@@ -773,6 +773,10 @@ You can use the [`collapse-rename-collapsedheight` codemod](https://github.com/m
      </Typography>
   ```

+  Note that the DialogContent now applies its padding with a specificity of 2.
+  It was changed to work around the lack of [support of `:first-child`](https://github.com/emotion-js/emotion/issues/1178) in emotion.
+  See [#26795](https://github.com/mui-org/material-ui/issues/) for more details.
+
 ### Divider

 - Use border instead of background color. It prevents inconsistent height on scaled screens.

oliviertassinari avatar Jun 17 '21 23:06 oliviertassinari

Why can't we apply the styles to the DialogContent? A Dialog should always have a heading anyway. Instead, people should add padding if they purposely omit the heading.

eps1lon avatar Jun 18 '21 06:06 eps1lon

A Dialog should always have a heading anyway

@eps1lon It's written as is it was a fact. Why should a DialogTitle be always rendered? Material Design doesn't seem to support this affirmation.

Anatomy 2. Title (optional)

https://material.io/components/dialogs#usage

Or the first demo of the page.

Capture d’écran 2021-06-20 à 02 05 58

It seems that they are consistent with the legacy version:

https://material.io/archive/guidelines/components/dialogs.html#dialogs-behavior

Most alerts don't need titles.


Instead, people should add padding if they purposely omit the heading.

Actually, we could likely leverage that a dialog title will most often be used to reverse the CSS tradeoff.

oliviertassinari avatar Jun 19 '21 23:06 oliviertassinari

I have tried:

diff --git a/packages/material-ui/src/DialogContent/DialogContent.js b/packages/material-ui/src/DialogContent/DialogContent.js
index 131e5e6980..0ed698d896 100644
--- a/packages/material-ui/src/DialogContent/DialogContent.js
+++ b/packages/material-ui/src/DialogContent/DialogContent.js
@@ -32,7 +32,7 @@ const DialogContentRoot = styled('div', {
   // Add iOS momentum scrolling for iOS < 13.0
   WebkitOverflowScrolling: 'touch',
   overflowY: 'auto',
-  padding: '20px 24px',
+  padding: '0 24px 20px',
   ...(styleProps.dividers
     ? {
         padding: '16px 24px',
@@ -40,8 +40,8 @@ const DialogContentRoot = styled('div', {
         borderBottom: `1px solid ${theme.palette.divider}`,
       }
     : {
-        '.MuiDialogTitle-root + &': {
-          paddingTop: 0,
+        ':not(.MuiDialogTitle-root) + &': {
+          paddingTop: 20,
         },
       }),
 }));

But it's not working.


How about we add a new prop like noTitle on the DialogContent component, to flatten the CSS specificity for the most frequent use case (with title)?

oliviertassinari avatar Jun 20 '21 00:06 oliviertassinari

A dialog having no title is problematic UI. Customization should not be our concern in this case. Otherwise we encourage problematic UIs.

eps1lon avatar Jun 20 '21 07:06 eps1lon

Looking at WAI-ARIA, they seems to support that a dialog should always be labeled.

The dialog has either: A value set for the aria-labelledby property that refers to a visible dialog title. A label specified by aria-label.

https://www.w3.org/TR/wai-aria-practices-1.1/

We could support DialogContent being the aria-label of the dialog, and rendered as a h2, without the DialogTitle. This would help better support the guidelines of Material Design.

A dialog having no title is problematic UI.

@eps1lon This is confusing me. Do you mean from a DOM perspective or a design perspective? The two seems distinct. Also, it seems exposed as an axiom, adding resources to support it would help. I agree with this affirmation if it concerns the label of the dialog and concerns the DOM aspect, not the design one.

oliviertassinari avatar Jun 20 '21 10:06 oliviertassinari

Do you mean from a DOM perspective or a design perspective?

I don't know what a "DOM perspective" is. The DOM is just a programming interface.

I'm talking about a design perspective. Just as your documents should have titles. Dialogs are mini-documents that are nested within other documents. Especially for fullscreen dialogs I don't see how a dialog without a title is a good thing.

We could support DialogContent being the aria-label of the dialog, and rendered as a h2, without the DialogTitle.

"Content" is not the heading or title and abusing it as such is just straight up confusing. Consider how you would teach this.

eps1lon avatar Jun 20 '21 12:06 eps1lon

@eps1lon OK thanks for the clarification, so it's none of the option concepts I was wondering about (DOM, as the a11y tree, and design as the Material Design aspect. Your point is more conceptual, "title" as something that labels the content of the modal, which is close to the a11y that requires a label. I agree.

Here Material Design is saying that the "title" can either be the "DialogTitle" or "DialogContent" (if alone).

oliviertassinari avatar Jun 21 '21 09:06 oliviertassinari

Here Material Design is saying that the "title" can either be the "DialogTitle" or "DialogContent" (if alone).

I didn't find any such statement. I even found statements contradicting this claim:

A dialog’s purpose should be communicated by its title and button text.

-- https://material.io/components/dialogs#anatomy

When scrolling is required, the dialog title is pinned at the top, with buttons pinned at the bottom.

-- https://material.io/components/dialogs#behavior

The only statement that comes close is only applicable under certain conditions:

Titles should be succinct. They can wrap to a second line if necessary, and be truncated. If there are long titles, or titles of variable lengths (such as translations), place them in the content area instead of the app bar.

-- https://material.io/components/dialogs#full-screen-dialog

And then people can wire the dialog label up differently.

eps1lon avatar Jun 22 '21 09:06 eps1lon

@eps1lon From what I understand of the logic of Material Design: The DialogTitle + DialogContent is visually harmful with very short dialogs, e.g. quick confirmation prompts. Only rendering a DialogTitle wouldn't match their design expectations, and rendering a DialogTitle + DialogContent with the same content would be repetitive, making the action harder for the end-users. Hence why they have made the DialogTitle optional.


Here Material Design is saying that the "title" can either be the "DialogTitle" or "DialogContent" (if alone).

@eps1lon I come to this conclusion (DialogContent can act as a title when DialogTitle is not rendered) from:

  1. https://material.io/components/dialogs#anatomy, the DialogTitle (the design element) is optional:
Capture d’écran 2021-06-22 à 14 03 21
  1. The implementation with MDC-web demo: https://material.io/components/dialogs#interactive-demo
Capture d’écran 2021-06-22 à 14 04 40
  1. The overview of the different dialog "Types"
Capture d’écran 2021-06-22 à 14 05 37

A dialog’s purpose should be communicated by its title and button text.

We can label the DialogContent (design) as the title (a11y label), problem solved.

When scrolling is required

Developers should use a DialogTitle if they need a scrollbar, it doesn't contradict that DialogContent can act as the a11y label of the modal.

oliviertassinari avatar Jun 22 '21 12:06 oliviertassinari

A real-life example in Google's products (GDrive)

Capture d’écran 2021-06-26 à 13 13 10

Can also be found in YouTube.


For the solution of this GitHub issue. My proposal would be, built on top of #26814:

diff --git a/packages/material-ui/src/DialogContent/DialogContent.js b/packages/material-ui/src/DialogContent/DialogContent.js
index 131e5e6980..5ecfb34f7c 100644
--- a/packages/material-ui/src/DialogContent/DialogContent.js
+++ b/packages/material-ui/src/DialogContent/DialogContent.js
@@ -5,6 +5,7 @@ import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled
 import styled from '../styles/styled';
 import useThemeProps from '../styles/useThemeProps';
 import { getDialogContentUtilityClass } from './dialogContentClasses';
+import DialogContext from '../Dialog/DialogContext';

 const useUtilityClasses = (styleProps) => {
   const { classes, dividers } = styleProps;
@@ -32,18 +33,16 @@ const DialogContentRoot = styled('div', {
   // Add iOS momentum scrolling for iOS < 13.0
   WebkitOverflowScrolling: 'touch',
   overflowY: 'auto',
-  padding: '20px 24px',
+  padding: '0 24px 20px',
   ...(styleProps.dividers
-    ? {
-        padding: '16px 24px',
-        borderTop: `1px solid ${theme.palette.divider}`,
-        borderBottom: `1px solid ${theme.palette.divider}`,
-      }
-    : {
-        '.MuiDialogTitle-root + &': {
-          paddingTop: 0,
-        },
-      }),
+    && {
+    padding: '16px 24px',
+    borderTop: `1px solid ${theme.palette.divider}`,
+    borderBottom: `1px solid ${theme.palette.divider}`,
+  }),
+  ...(styleProps.noTitle && {
+    paddingTop: 20
+  }),
 }));

 const DialogContent = React.forwardRef(function DialogContent(inProps, ref) {
@@ -52,15 +51,18 @@ const DialogContent = React.forwardRef(function DialogContent(inProps, ref) {
     name: 'MuiDialogContent',
   });

-  const { className, dividers = false, ...other } = props;
-  const styleProps = { ...props, dividers };
+  const { className, dividers = false, noTitle = false, id: idProp, ...other } = props;
+  const styleProps = { ...props, noTitle, dividers };
   const classes = useUtilityClasses(styleProps);

+  const { titleId: id = idProp } = React.useContext(DialogContext);
+
   return (
     <DialogContentRoot
       className={clsx(classes.root, className)}
       styleProps={styleProps}
       ref={ref}
+      id={noTitle ? id : undefined}
       {...other}
     />
   );

This would solve the issue of @jlin5 by always having a flat level of specificity, making overrides easier. It also resonates with the point of @eps1lon around how the usage of the DialogTitle + DialogContent should be a lot more frequent. But still, it allows to support the short alert dialog use case. It can be used like this:

diff --git a/docs/src/pages/components/dialogs/AlertDialog.tsx b/docs/src/pages/components/dialogs/AlertDialog.tsx
index d06b6c6860..0b79f9ed9d 100644
--- a/docs/src/pages/components/dialogs/AlertDialog.tsx
+++ b/docs/src/pages/components/dialogs/AlertDialog.tsx
@@ -28,14 +28,8 @@ export default function AlertDialog() {
         aria-labelledby="alert-dialog-title"
         aria-describedby="alert-dialog-description"
       >
-        <DialogTitle id="alert-dialog-title">
+        <DialogContent noTitle>
           {"Use Google's location service?"}
-        </DialogTitle>
-        <DialogContent>
-          <DialogContentText id="alert-dialog-description">
-            Let Google help apps determine location. This means sending anonymous
-            location data to Google, even when no apps are running.
-          </DialogContentText>
         </DialogContent>
         <DialogActions>
           <Button onClick={handleClose}>Disagree</Button>

Before

Capture d’écran 2021-06-22 à 14 25 49

After

Capture d’écran 2021-06-22 à 14 24 24

oliviertassinari avatar Jun 22 '21 12:06 oliviertassinari

I don't understand why you're hell bent on making no title work when there's a really simple solution to the problem that puts the usability of the UI first: just add a title.

Instead we have to overload the DialogContent component which makes implementation harder and makes in near impossible to explain to a developer what this component does or how you should use it.

It's sacrificing the usability of the component and the created UI just because some stakeholder told you they really don't want to change how they work?

eps1lon avatar Jun 28 '21 07:06 eps1lon

With the current behavior, an outlined TextField in a DialogContent doesn't display its label correctly. https://codesandbox.io/s/textfield-in-dialog-content-dswq4?file=/demo.tsx

amaslakov avatar Jul 04 '21 06:07 amaslakov

Have the same issue. Title gets rid of the top padding for me on Dialog Content. Trying to add paddingTop to Dialog Content and it gets overridden by:

.MuiDialogTitle-root+.css-141nu3b-MuiDialogContent-root {
    padding-top: 0;
}

This is on "@material-ui/core": "^5.0.0-beta.1".

This is an issue if you have the whole title background a different color then the content. Content will bump right up to it.

Example:

https://codesandbox.io/s/hopeful-fermi-0v850?file=/src/App.js:514-515

ghost avatar Aug 19 '21 16:08 ghost

I just came to this issue after upgrading from v4 to v5. We have many dialogs where the first TextField label gets truncated as shared by @amaslakov above.

ValentinH avatar Sep 24 '21 13:09 ValentinH

Same for me like the previous replies - any TextField or Autocomplete's floating label gets truncated, the upper ~50% part gets to "disappear" by the DialogTitle's bottom padding (or by the "mistake" of the missing top padding of DialogContent). My opinion is that we should always have the DialogContent's top padding -> it would solve this issue + would work for dialogs without a DialogTitle. Img: https://ibb.co/xzzHprw Example: https://codesandbox.io/s/mui-v5-test-3t3nb?file=/src/App.js

w5lurz avatar Oct 11 '21 12:10 w5lurz

Was there a specific reason why this DialogTitle component didn't get the same prop system similar to primaryTypographyProps in ListItemText. I'm running into a use case where our Design System wants to use a different variant for all DialogTitles. There's no easy way to do that for dialogs... ForListItemText customization I would just add prop overrides within primaryTypographyProps...

But since h6 is baked in as the default DialogTitle typography variant, that's not possible.

I could always use disableTypography and add the right variant I wanted, but I feel like this could be supported to make sure that global Dialog customizations are possible from the theme file.

This applies to DialogContentText as well.

The use case is;

<Dialog>
  <DialogTitle>Use Google's Location Service</DialogTitle>
  <DialogContent>
    <DialogContentText>Content...</DialogContentText>
  </DialogContent>
</Dialog>


... in theme.js
{
  props: {
      MuiDialogTitle: {
            typographyProps: {
                component: 'h2',
                variant: 'h3',
                color: 'textSecondary',
            }
        },
        MuiDialogContentText: {
            typographyProps: {
                variant: 'body1',
            }
        }
  }
}

MUI V4

KyruCabading avatar Nov 16 '21 22:11 KyruCabading

I'm currently experiencing this same issue on @mui/[email protected]

Any padding-top I provide to the DialogContent component is overwritten with padding-top: 0 from the .MuiDialogTitle-root+.css-b3xomw-MuiDialogContent-root class.

I see that this thread hasn't had a reply in awhile. Has anybody been able to resolve this or find a work around?

20BBrown14 avatar Aug 09 '22 21:08 20BBrown14

I'm currently experiencing this same issue on @mui/[email protected]

Any padding-top I provide to the DialogContent component is overwritten with padding-top: 0 from the .MuiDialogTitle-root+.css-b3xomw-MuiDialogContent-root class.

I see that this thread hasn't had a reply in awhile. Has anybody been able to resolve this or find a work around?

We can't change anything for now, at least until the next major. I would propose bumping the specificity using &&

mnajdova avatar Aug 11 '22 10:08 mnajdova

To solve the problem of the floating labels getting cut of I'm using this for now: createTheme({compontents: { MuiDialogContent: { styleOverrides: { root: { paddingTop: "20px !important" } } } }

You could also set overflowY: unset

juliushuck avatar Feb 12 '23 17:02 juliushuck

I have fixed this by wrapping <DialogContent> in a div. Have not seen any side effects of this yet.

RidhwanDev avatar Oct 26 '23 14:10 RidhwanDev

I have fixed this by wrapping <DialogContent> in a div. Have not seen any side effects of this yet.

worked for me , but still very hacky.

undesicimo avatar Nov 14 '23 01:11 undesicimo

I'm currently experiencing this same issue on @mui/[email protected] Any padding-top I provide to the DialogContent component is overwritten with padding-top: 0 from the .MuiDialogTitle-root+.css-b3xomw-MuiDialogContent-root class. I see that this thread hasn't had a reply in awhile. Has anybody been able to resolve this or find a work around?

We can't change anything for now, at least until the next major. I would propose bumping the specificity using &&

As specified by @mnajdova here, best implementation for now would be adding style to parent Dialog component ,

 <Dialog
sx={{
        '&& .MuiDialogContent-root': {
          padding: '4px 24px',
        },
      }}
...
Screenshot 2023-11-14 at 10 41 09

undesicimo avatar Nov 14 '23 01:11 undesicimo