[Alert] Filled variant text color should be using theme.palette[color].contrastText and not theme.palette.getContrastText

[povilass]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Current behavior 😯

Currenly theme.palette.getContrastText is used to calculate text color.

Expected behavior 🤔

Expected color to be provided from theme.palette[color].contrastText

Steps to reproduce 🕹

https://codesandbox.io/s/clever-almeida-u36ek8?file=/src/App.tsx

Context 🔦

Pull request broke Alert #29813 text colour. If contrastText is provided it should not calculate theme.palette.getContrastText or should account on provided contrastText

Your environment 🌎

No response

[mnajdova]

Thanks for the report. #29813 did change the behavior, but it was for solving an issue. The contrast color in the cases where the colors changed is because the black gives better contrast than the white color for the text. We could use contrastText for light mode, but that won't work for dark mode, mainly because we use the color.palette[color].dark as a background, and the color.palette[color].contrastText is meant as a contrast text for the main color:

index 5cfcd0492e..6ce845f83e 100644
--- a/packages/mui-material/src/Alert/Alert.js
+++ b/packages/mui-material/src/Alert/Alert.js
@@ -95,11 +95,14 @@ const AlertRoot = styled(Paper, {
                 theme.palette.mode === 'dark'
                   ? theme.palette[color].dark
                   : theme.palette[color].main,
-              color: theme.palette.getContrastText(
-                theme.palette.mode === 'dark'
-                  ? theme.palette[color].dark
-                  : theme.palette[color].main,
-              ),
+              color:
+                theme.palette.mode === 'light' && theme.palette[color].contrastText
+                  ? theme.palette[color].contrastText
+                  : theme.palette.getContrastText(
+                      theme.palette.mode === 'dark'
+                        ? theme.palette[color].dark
+                        : theme.palette[color].main,
+                    ),
             }),
       }),
   };

@siriwatknp what do you think about this change?

[povilass]

So the problem maybe is that theme.palette[color].contrastText only with theme.palette[color].main.
Kinda sticky situation because theme.palette[color].light need to have theme.palette[color].lightContrastText and theme.palette[color].dark need to have theme.palette[color].darkContrastText. Problem maybe can be solve if each color would have it's own contrastText color and it would be already resolve in createPalette function also it can be overriden by what actual color you passed.

{
  light?: string;
  main: string;
  dark?: string;
  contrastText?: string;
  lightContrastText?: string;
  darkContrastText?: string;
}

It would leave you with

{

function resolveContrastColor(color: any, theme: any) {
     const isLightMode = theme.palette.mode === 'light';
     const contrastText = isLightMode? theme.palette[color].contrastText ? theme.palette[color].darkContrastText;
     return contrastText ?? theme.palette.getContrastText(isLightMode? theme.palette[color].main: theme.palette[color].dark,)
}

color: resolveContrastColor(color, theme)
}

Or this should be resolve in theme palette after createPalette

{
success: {
            main: '#00C796',
            dark: '#008B69',
            light: '#33D2AB',
            contrastText: paletteMode === 'light' ? '#fff' : getContrastText('#008B69'),
        },
}

And you only left with

+     color: theme.palette[color].contrastText
+                  ? theme.palette[color].contrastText
+                  : theme.palette.getContrastText(
+                      theme.palette.mode === 'dark'
+                        ? theme.palette[color].dark
+                        : theme.palette[color].main,
+                    ),

[mnajdova]

We haven't had so far the need to have a contrast text color for each color in the palette, so I wouldn't add it until we see this required in more than one place. I feel this would be overengineering for the problem we have.

For the extendTheme util that is based on CSS variables, maybe we can create automatically contrast text for each color if we see the need for it.

Let's also consider what @siriwatknp thinks.

[siriwatknp]

I won't do anything at this point because the contrast ratio is expected (it should be black):

What we could do is add a comment to the Alert page that the filled variant calculates color from the default background and provide an example of how to change the behavior:

const theme = createTheme({
  components: {
    MuiAlert: {
      styleOverrides: {
        root: ({ ownerState }) => ({
          ...(ownerState.variant === "filled" && {
            color: "#fff"
          })
        })
      }
    }
  }
});

Here: https://codesandbox.io/s/admiring-shape-rzm6lw?file=/src/App.tsx:341-884

[povilass]

@siriwatknp @mnajdova I agreed not to do anything. But do we need extra logic in components on how to resolve contrastColor if it already is calculated in the palette itself

material-ui/packages/mui-material/src/styles/createPalette.js

Line 258 in 6275be7

color.contrastText = getContrastText(color.main);

. Maybe we need only calculate dark color also and just use it as it is theme.palette.mode === 'light' ? theme.palette[color].contrastText : theme.palette[color].darkContrastText which pobably can decrease bundle size also at some components.

Right now I see only code duplication for no reason in each component with contrast text colours and the same logic all over the place.

The other option is to remove contrastText form theme and changed it to getModeContrastText and use it in all places also. You do not need 2 ways on how colour should be resolved.

[mnajdova]

Thanks @povilass this is a good point. We are likely going to hit this in the future with CSS vars. @povilass currently we don't need this in other components, this is why I don't think we should add it right now. cc @siriwatknp did you need so far something like this for joy?

[richcatt]

Apologies if this is the wrong place to ask the question, but does this mean that the example in the docs is no longer supported - is the contrast text always either white or black now? And does the contrastText property have no effect when using a custom theme?

https://mui.com/material-ui/react-alert/

[richcatt]

Sorry, ignore me - I've realised the above example is achievable by not using the filled variant.