MuiListItemText-secondary overrides custom theme color for typography variant

[EliRobinson]

When using a <ListItemText/> component with the secondaryTypographyProps set to use a custom variant with a color value, the default color rgba(0, 0, 0, 0.6) overrides the custom variant color.

• [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 😯

Setting a custom theme color gets overridden by the default palette.text.secondary.

Expected Behavior 🤔

Setting the color attribute for a custom typography variant should display properly

Steps to Reproduce 🕹

https://codesandbox.io/s/muilistitemtext-bug-6y9k1

Custom theme...

typography: {
      header3Bold: {
        color: Constants.arrivedDark,
        fontFamily: apercuFonts,
        fontSize: getFontSize(700, isMobile),
        fontWeight: 700,
        lineHeight: '1.133em',
      },
},

Context 🔦

Using a custom typography variant with the color css attribute gets overridden by the default palette.text.secondary.

Your Environment 🌎

`npx @material-ui/envinfo`

``` System: OS: macOS 11.4 Binaries: Node: 15.14.0 - ~/.nvm/versions/node/v15.14.0/bin/node Yarn: Not Found npm: 7.14.0 - ~/.nvm/versions/node/v15.14.0/bin/npm Browsers: Chrome: 91.0.4472.114 Edge: Not Found Firefox: Not Found Safari: 14.1.1 npmPackages: @emotion/react: ^11.1.5 => 11.1.5 @emotion/styled: ^11.3.0 => 11.3.0 @material-ui/codemod: ^5.0.0-alpha.35 => 5.0.0-alpha.35 @material-ui/core: ^5.0.0-alpha.38 => 5.0.0-alpha.38 @material-ui/icons: ^5.0.0-alpha.37 => 5.0.0-alpha.37 @material-ui/lab: ^5.0.0-alpha.38 => 5.0.0-alpha.38 @material-ui/private-theming: 5.0.0-alpha.36 @material-ui/styled-engine: 5.0.0-alpha.34 @material-ui/system: 5.0.0-alpha.38 @material-ui/types: 6.0.1 @material-ui/unstyled: 5.0.0-alpha.38 @material-ui/utils: 5.0.0-alpha.36 @types/react: ^17.0.2 => 17.0.3 react: ^17.0.2 => 17.0.2 react-dom: ^17.0.2 => 17.0.2 styled-components: ^5.2.1 => 5.2.3 typescript: ^4.2.3 => 4.2.4 ```

[mnajdova]

Could you please share codesandbox illustrating the problem? This would help us to debug easier the issue.

[EliRobinson]

@mnajdova - Added above in steps to reproduce

[siriwatknp]

// ListItemText.js
if (secondary != null && secondary.type !== Typography && !disableTypography) {
    secondary = (
      <Typography
        variant="body2"
        className={classes.secondary}
        color="text.secondary" // <----- turned to `sx` overrides
        display="block"
        {...secondaryTypographyProps}
      >
        {secondary}
      </Typography>
    );
  }

@mnajdova because the color is forced in the ListItemText, that's why the color from theme (typography variant) is overridden.

[mnajdova]

Looks like it worked like that always, see forked sandbox with v4 - https://codesandbox.io/s/muilistitemtext-bug-forked-lf90f?file=/src/App.js

The general problem is not that much how the color is provided, I've tried this diff it worked the same:

index 52cd68f178..afbdd06757 100644
--- a/packages/material-ui/src/ListItemText/ListItemText.js
+++ b/packages/material-ui/src/ListItemText/ListItemText.js
@@ -50,6 +50,10 @@ const ListItemTextRoot = styled('div', {
   }),
 }));

+const ListItemTextSecondary = styled(Typography)(({ theme }) => ({
+  color: theme.palette.text.secondary,
+}));
+
 const ListItemText = React.forwardRef(function ListItemText(inProps, ref) {
   const props = useThemeProps({ props: inProps, name: 'MuiListItemText' });
   const {
@@ -95,15 +99,14 @@ const ListItemText = React.forwardRef(function ListItemText(inProps, ref) {

   if (secondary != null && secondary.type !== Typography && !disableTypography) {
     secondary = (
-      <Typography
+      <ListItemTextSecondary
         variant="body2"
         className={classes.secondary}
-        color="text.secondary"
         display="block"
         {...secondaryTypographyProps}
       >
         {secondary}
-      </Typography>
+      </ListItemTextSecondary>
     );
   }

The style overrides will always take presedence over the styled defined for the component (even if default props are updated, still the styles come from he component itself), namely from https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Typography/Typography.js#L45

---

At this moment I wouldn't change anything, as any solution that comes to mind would introduce breaking changes. I would propose you add style overrides (via adding sx prop, or new value for the color prop).

@siriwatknp am I missing something? Is there an opportunity for fixing the issue with the current state of things?

[siriwatknp]

@mnajdova agree, I don't see other option so far and I think it works as expected from the implementation, therefore it is not a bug. To change the color of SecondaryText globally is to update theme.palette.text.secondary, not theme.typography.

I think we can close this issue.

[eps1lon]

, I don't see other option so far and I think it works as expected from the implementation, therefore it is not a bug.

The implementation does not decide whether something is a bug or not. If it did, nothing would be a bug since everything would work as implemented.

It seems like there is a misunderstanding of the API. So this does need work e.g. documentation, runtime warnings, different API. But just handwaving as "this is the implementation" is not user-friendly.

@mnajdova Though it does sound like it was already an issue in v4 in which case we can remove it from the v5 milestone.

[mnajdova]

@mnajdova Though it does sound like it was already an issue in v4 in which case we can remove it from the v5 milestone.

Makes sense, will remove it from the milestone.

I can come back to this once we are done with the v5 release. My point is, I don't think it is expected that adding theme overrides for a component used somewhere, should win over the styles defined for this component where it is used? If this worked like this, we would have had bugs all over the place. Namely, the order of the overrides is the following:

1. styles defined via the styled() utility
2. overrides defined in the theme (test case that it wins over 1 - https://github.com/mui-org/material-ui/blob/next/packages/material-ui-system/src/styled.test.js#L234)
3. variants defined in the theme (test case that it wins over 2 - https://github.com/mui-org/material-ui/blob/next/packages/material-ui-system/src/styled.test.js#L337)
4. styled() wrapper around the component should win over variants (test case that it wins over 3- https://github.com/mui-org/material-ui/blob/next/packages/material-ui-system/src/styled.test.js#L367)

If we think this is correct (which I think it is), we should disregard this as work as expected, if not, we should change how it works globally.

[EliRobinson]

Just checking back in. The correct answer to solve this issue is to change the palette.secondary.text color to match what I want it to be and wrapping it in it's own Typography with a variant is not the right order?

[mnajdova]

Just checking back in. The correct answer to solve this issue is to change the palette.secondary.text color to match what I want it to be and wrapping it in it's own Typography with a variant is not the right order?

I am sorry, it's hard to follow. Can you share a sandbox? I haven't suggested solution to the problem, just explained how it works. You can use the classname for the secondary typography to add any color you want. See https://codesandbox.io/s/muilistitemtext-bug-forked-f3kdy

[EliRobinson]

Ok, I'll go with a one-off styled solution. I'll keep following this to see where we land with the order of priority in terms of colors on the secondary text.