mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.96k stars 32.27k forks source link

[material-ui][theme] Lower CSS Specificity for color scheme rules in CSS Vars Theme #42893

Open joebochill opened 4 months ago

joebochill commented 4 months ago

Summary

I'm in the process of migrating themes to the new CSS Vars Theme/Provider. I have a light/dark theme which I'm combining per the migration guide into a single theme with dark/light color schemes.

As mentioned in the docs, this is causing a pretty big headache with regards to CSS rule specificity (any of the rules I set for the dark theme cannot be overridden in a component's sx prop without also specifying the color scheme selector.

I'm wondering if there is a better way that I should approach this or if there's a way that color schemes could be implemented without the extra css specificity.

Examples

This is a super simple example (not exactly what I'm doing in my theme, but shows the issue):

Let's say we have a rule in the theme for Button to change the background color:

styleOverrides: {
    root: ({ theme }) => ({
      backgroundColor: 'primary.main',
      [theme.getColorSchemeSelector('dark')]:{
          backgroundColor: 'primary.light'
      }
    }),
}

If I want to override the button background color somewhere in my project:

<Button sx={{backgroundColor: 'red'}} />

it only works for the light theme (with the old ThemeProvider approach, this would work for both).

To get it to work for the dark theme, I have to add an extra rule, which is a bit clunky:

<Button sx={(theme) => ({
    backgroundColor: 'red',
    [theme.getColorSchemeSelector('dark')]:{
        backgroundColor: 'red',
    }
})} />

Motivation

Essentially, I just want it to be easy to override theme styles for a component via sx without having to write a rule for both/all color schemes that I may have.

Search keywords: CssVars Theme getColorSchemeSelector specificity override sx

ZeeshanTamboli commented 4 months ago

A reproduction with v5: https://stackblitz.com/edit/vitejs-vite-ycxrk9?file=src%2FApp.jsx&terminal=dev.

It also gets overriden in v6 with new theme.applyStyles in dark mode: https://stackblitz.com/edit/vitejs-vite-3zw18f?file=src%2FApp.jsx,package.json&terminal=dev due to the attribute selector.

siriwatknp commented 4 months ago

~#42839 will fix this issue. The theme.applyStyles('dark', …styles) will use *:where() & that does not increase CSS specificity.~

My mistake, as @ZeeshanTamboli pointed out that the sx prop is still overridden in v6. In this case, I don't think we fix this because it's how Emotion generate styles.

A workaround would be to use :where(*) in sx prop:

<Button
  variant="contained"
  sx={{
    '&:where(*)': {
      backgroundColor: 'red',
    }
  }}
>
  Hello
</Button>

This does not increase CSS specificity (it's still (0, 1, )) but just to ensure that the order to the stylesheet is not in the component styles which will override the theme as expected.

siriwatknp commented 4 months ago

However, I don't think that this is a bug but something that we need to document because in some case, the current behavior is useful so that the styles in dark mode is not impacted.

For example,

<Button
  variant="contained"
  sx={{
    color: 'black',
  }}
>
  Hello
</Button>

In dark mode, the color will not be black which is expected. Normally the color between light and dark are different, so it's normal to specify the selector.

joebochill commented 3 months ago

Yes, I think you're probably right — in most cases, someone would need to set the color overrides (via sx) differently for light/dark theme anyway (and maybe having the styles break is a good indicator that they need to do that). The only time when that might be problematic is if, for example, a project only supports a single (non-default) color scheme — e.g., it's a dark mode only application and they have to specify the dark color mode in every style override they want to do via sx.

joebochill commented 3 months ago

Another case where this is a bit annoying is if you're trying to override a color via sx and you want to use a theme color (not a arbitrary hex color as in the example above).

E.g.,

<Button sx={{
    backgroundColor: 'primary.light',
}} />

When using values from the theme, I would expect a single rule to handle dark/light theme differences (that's the point of the theme).

You'd have to specify the same rule twice (once with a dark mode selector) or use the where(*) selector mentioned above. Both feel a little icky in practice.

rossipedia commented 1 month ago

For example,

<Button
  variant="contained"
  sx={{
    color: 'black',
  }}
>
  Hello
</Button>

In dark mode, the color will not be black which is expected. Normally the color between light and dark are different, so it's normal to specify the selector.

I don't think I agree with the statement that this is expected. Generally, if I specify color: 'black', without any other rules, I would expect that to apply as the default, in all color schemes, until I added additional rules to handle another color scheme.

(Maybe I'm mis-reading the statement though)

trungutt commented 1 month ago

@siriwatknp I think this specificity issue impacts all enterprise-scale codebases when folks try to migrate from the Non Vars to CSS Vars theme.

I understand the argument on having to specify both light and dark color schemes for the sx props to work correctly. In practice, it creates duplicates all over the place, or even using '&:where(*)': { trick it isn't great DX. I'm betting that lots of folks will expect color: 'black' will be simply color: 'black' in all color schemes. In case of a migration like @joebochill, bugs will fall through the cracks. I suspect lots of folks will be hitting this roadblock.

We could have a workaround for this: adding another redirection level to the CSS property we want to customize, and only use applyStyles to that additional redirection target, like following

styleOverrides: {
  root: ({ theme }) => ({
    backgroundColor: 'var(--custom-backgroundColor)',

    // Redirecting to a CSS variable to avoid applying `applyStyles` directly to the CSS property we want to customize
    // Only applying `applyStyles` to that CSS variable.
    '--custom-backgroundColor': 'primary.main',
    ...theme.applyStyles('dark', {
      '--custom-backgroundColor': 'primary.light',
    }),
  }),
}

So that

// This will always have red background
<Button sx={{ backgroundColor: 'red' }} />

// This will always follow the theme customization
<Button />

Theme customization will be more cumbersome, but we provide great DX to the rest of the codebase which has much more important size than theme customization part. Hope that helps @joebochill