CSS rules specificity is order dependent on chips

[jbblanchet]

In v4, looking at the chips demo, the css rules specificity is dependent on the order of the css, which is problematic when using any build tools that might change the order

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

The should be no specificity conflict between classes.

Current Behavior 😯

Looking at the chips demo on your website, the generated code has both a .MuiChip-avatarColorPrimary and .MuiAvatar-colorDefault class defining the color and background-color css attributes. Same thing happens with .MuiChip-avatar and .MuiAvatar-root both defining width, height and font-size.

Steps to Reproduce 🕹

in https://material-ui.com/components/chips/, inpect element on the first blue chip shows:

[oliviertassinari]

@jbblanchet I fail to see the problem. Could you expand on what's wrong?

[marcelosuzumura]

For me, when running in dev the MuiChip is loaded after MuiAvatar, but when using in prod after building it with yarn build, the reverse occurs and MuiAvatar is loaded after MuiChip.

Since MuiAvatar-root and MuiChip-avatar have the same specificity and are applied to the same element, the load order makes it look different in each environment.

In dev it looks as I intended it to be:

And the load order is:

In prod (yarn build) it looks like this:

And now the load order is reversed:

This behavior surely didn't happen in 3.x, only after upgrading it to 4.1.3.

[oliviertassinari]

@jbblanchet Oh, this is a bug. Thanks for providing more details! I believe that the import is wrong. What do you think of this diff?:

diff --git a/packages/material-ui/src/Chip/Chip.js b/packages/material-ui/src/Chip/Chip.js
index 08ae064eb..8214233b9 100644
--- a/packages/material-ui/src/Chip/Chip.js
+++ b/packages/material-ui/src/Chip/Chip.js
@@ -8,7 +8,7 @@ import { emphasize, fade } from '../styles/colorManipulator';
 import { useForkRef } from '../utils/reactHelpers';
 import unsupportedProp from '../utils/unsupportedProp';
 import { capitalize } from '../utils/helpers';
-import '../Avatar/Avatar'; // So we don't have any override priority issue.
+import '../Avatar'; // So we don't have any override priority issue.

 export const styles = theme => {
   const height = 32;

Do you want to submit a pull request? :)

[jbblanchet]

@oliviertassinari Sorry for the delay. Re-reading my description right now, I'm missing half of the info, I probably shouldn't have opened this during a rush between 2 meetings. 😄

@marcelosuzumura Thanks for completing my bug report, this is exactly the behavior I was seeing. 👍

I sadly have no idea if this is the right fix or not, I've never used emotion in any project so it's unclear to me what happens in the background to create the css, but from the comments I'd say it's probably it.

[eps1lon]

Isn't this a general issue that should be resolved in some other way? The current approach hurts tree shaking because it pre-emptively imports components whose classNames might appear on the same class attribute.

[oliviertassinari]

@eps1lon What other way do you have in mind? For instance, take styled-components, you would have to import the component and to reference it in the selector.

In this very occurrence of the problem, I believe that the cjs and esm versions of the Avatar components are loaded. It can't work correctly.

[eps1lon]

What other way do you have in mind?

I would've included this and not just asked if it was possible. It's in general an issue for tree-shaking not just because it bloats the bundle for possible specificity conflicts but also because it violates sideEffects: false which might cause all sorts of subtle bugs that we don't even know about yet.

A naive approach would guarantee that the stylesheet order is stable but I'm not sure how big of a perf hit that would be.

[NMinhNguyen]

@eps1lon @oliviertassinari this is the exact issue I mentioned in #15355 (comment) btw

[oliviertassinari]

There is another possible solution explored in #16609 (comment).

[davidbonnet]

We can see the issue right now on https://material-ui.com/components/chips/

[oliviertassinari]

@davidbonnet Do you want to handle the proposed fix #16374 (comment)?

[Dhruvi16]

@oliviertassinari Hey! I was going through the issue and I liked the idea of increasing the specificity in #16609 . So, from all the reading I understood that you are suggesting increasing specificity of the .MuiChip-avatar class so it cannot be overridden by another class. And my query is, does it affect any other piece of code or create some bugs I am not aware of?

This might be a silly doubt but I am new to this so could you please help me out?
Thank you!

[oliviertassinari]

@Dhruvi16 We need to be cautious when we increase the specificity as it makes overrides harder, especially for people that don't know much about how CSS specificity works. Aside from it, no, I'm not aware of any other issue.

[oliviertassinari]

@Dhruvi16 In our case, I think that we should apply #16374 (comment), It would already be a great step. Do you want to take care of it? :)

[Dhruvi16]

Yes, I will do it. But it hurts tree shaking as mentioned earlier. So, do you think this is the best idea we can go with?

[oliviertassinari]

I have no idea what's the best option. They all their pros and cons. IMHO it won't make a big difference, if we can optimize for the least amount of work to close this issue, it's fine by me.

• Does the tree shaking issue change anything at the end of the day?
• Would increasing the specificity be a breaking change?
• What if we can avoid conflicting style properties in the first place?

[Dhruvi16]

I think the best thing to do will be avoid conflicting style properties. But I am not sure if that is possible. So the next best thing we can do is apply the solution in #16374 (comment) and move forward with this.

I can send a PR if you agree on this.

[oliviertassinari]

@Dhruvi16 Sounds good to me. If we can solve 60% of the problem with this simple change, it's good to take, it would already significantly reduce the priority of this issue compared to the other one we have :).

[hibearpanda]

Found this via #16609

I'm currently on 4.5.1 and have the same issue:

<Chip
  avatar={<Avatar><Face /></Avatar>}
  label={t('project:unassigned')}
  onClick={(e) => setAnchorEl(e.currentTarget)}
  variant="outlined"
/>

[hibearpanda]

Do you guys have a reproduction (it does happen with the SSR build in our docs, maybe an issue between the vendor and page assets)? The fact that it's still present worries me. We rely on the synchronous breast first search traversal of the JavaScript modules dependency tree (like React does with elements) to enforce the correct CSS injection order (so does styled-components).

Wanted to create a reproduction for you. Sorry for the rushed reply yesterday - was leaving the office.

I've created a repo https://github.com/idisclosure-legalx/mui-chip that demonstrates the issue with Next. Please let me know if there's anything else I can help with.

[oliviertassinari]

@hibearpanda Thanks for the reproduction with Next.js. We can observe it too on the documentation that uses Next.js. Does it reproduce outside? Could it be a Next.js specific issue?

[mcMickJuice]

This issue is not specific to next. I'm experiencing it with Create React App when running yarn build

[oliviertassinari]

OK thanks, there is a new hypothesis I need to verify. Let's say we mark the Avatar module side effect free, the Avatar has a side effect on the CSS injection order, and webpack? decides to remove the import. 🔥

[pcwa-ahendricks]

Maybe related to #16609

I've seen the avatar issue and a similar issue with the margin-left property with <Button> inside <DialogActions/>. Not sure if it's related to this avatar issue.

[oliviertassinari]

@pcwa-ahendricks if the side effect free theory is verified, then yes, it should be the same.

[pcwa-ahendricks]

Okay. I can confirm that config.optimization.sideEffects = false in next.config.js fixes both the dialog and avatar issue. I'll add that that particular Webpack config change also increased the projects initial JS payload size by about ~80kb after making the change.

[oliviertassinari]

@hibearpanda's reproduction confirms the theory of #16374 (comment). We need a way to mark the Avatar import, not side effect free. The following seems to do the job:

diff --git a/packages/material-ui/src/Chip/Chip.js b/packages/material-ui/src/Chip/Chip.js
index 5a7d605d7..2eb5635ec 100644
--- a/packages/material-ui/src/Chip/Chip.js
+++ b/packages/material-ui/src/Chip/Chip.js
@@ -7,7 +7,11 @@ import { emphasize, fade } from '../styles/colorManipulator';
 import useForkRef from '../utils/useForkRef';
 import unsupportedProp from '../utils/unsupportedProp';
 import capitalize from '../utils/capitalize';
-import '../Avatar'; // So we don't have any override priority issue.
+import Avatar from '../Avatar';
+
+// Force a side effect so we don't have any override priority issue.
+// eslint-disable-next-line no-unused-expressions
+Avatar.styles;

 export const styles = theme => {
   const backgroundColor =

Does somebody want to try it out?

@pcwa-ahendricks For #16609, the concern is different, the solution proposed here wouldn't scale to the case where somebody implements a custom button with styled-components (SC). Because the user uses SC, he has configured the CSS injection order to have Material-UI first and SC second, this means that for two CSS selectors of specificity 1, SC gets precedence (win). Now, if you consider that Safari adds a margin to the native button element, the SC button likely has a button margin reset to 0. The Modal CSS selector is: .MuiDialogActions-spacing > * + *, this has a specifity of 1 => ❌. I think that a solid solution is presented in #16609 (comment). If you want to open a pull request, you are good to go :).

[NMinhNguyen]

@oliviertassinari

Avatar.styles;

Playing around with compress.pure_getters on https://try.terser.org/ I think that pure_getters option (see https://github.com/terser/terser#compress-options) may treat this property access as side-effect free and drop it. But only if pure_getters: true, and by default it's strict, so we're prob okay in most cases.

[oliviertassinari]

@NMinhNguyen Thank you for looking at it. I'm not familiar with webpack tree shaking pipeline. Do you know if the uglify/terser step happens before or after the webpack tree shaking step? cc @eps1lon

[NMinhNguyen]

@oliviertassinari not too sure, sorry!

[jperasmus]

I believe the issue occurs for other components as well, not only for the Avatar component. We experience the same issue with the MuiToggleButton and MuiToggleButtonGroup.

On dev, the MuiToggleButtonGroup style injection is after MuiToggleButton, but in prod, MuiToggleButton is last.

Dev:
https://share.getcloudapp.com/2NuA6OE1

Prod:
https://share.getcloudapp.com/NQue401E

[oliviertassinari]

@jperasmus Thanks for the report. Yes, it's possible. We might want to increase the specificity here too.

[figmentc]

Having the same issue with the <Button /> component as well.

Not sure why this is marked off topic. The same import order issue is happening for Button and Typography components as well. The import orders are not consistent between dev and prod when importing using destructuring

import { Typography, Button } from '@material-ui/core'

Tested on the Next-js typescript example in the mui repo.

[gpessa]

Damn, I have the same @karpkarp. Have you found any solution?

[figmentc]

@gpessa we ended up switching off the example from the mui repo for Next.js, specifically the Link.js file seemed to be a source of issues. I put up a a ticket on the Next.js repo as well and they seem to have planned in a future iteration.
vercel/next.js#15212

For now this is what we have in prod

    import Link from 'next/link'
    <Link href={href}>
      <Button className={classes.root} component='a' color='inherit'>
        <NavIcon
          className={classes.navItemIcon}
          fontSize='inherit'
          data-test='navItemIcon'
        />
        <Box display={{ xs: 'none', lg: 'block' }}>
          <Typography className={classes.navItemText} data-test='navItemText'>
            {name}
          </Typography>
        </Box>
      </Button>
    </Link>

[o0x2a]

I'm also experiencing same issue, the order of added styles tag are different between dev and prod builds. I'm using CRA.
In dev, I get MuiButton,MuiToolbar,MuiTab,MuiTabs, and in prod, I get MuiToolbar, MuiButton, MuiTabs,MuiTab. The issue show itself when my custom style get different orders from dev to prod.

Not sure the issue comes from jss, mui, or a conflict with webpack configs?

[ghost]

same here，

"next": "^10.0.0",
"@emotion/core": "^10.1.1",
"@emotion/styled": "^10.0.27",
"@material-ui/core": "^5.0.0-alpha.14",
 "@material-ui/icons": "^4.9.1",

prod ， dev Different

[o0x2a]

I solved the issue by by creating my own generateClassName function, by doing this you can create unique classNames for production and make sure the classNames will be unique. I also increase the specify of my customisations for mui classes (e.g. Mui-Paper) to mydiv.Mui-Paper.
here is how you can provide your own generateClassName

    <ThemeProvider theme={theme}>
      <StylesProvider generateClassName={generateClassName}>
        <BotEditor />
      </StylesProvider>
    </ThemeProvider>

the mui default function can be found here https://github.com/mui-org/material-ui/blob/master/packages/material-ui-styles/src/createGenerateClassName/createGenerateClassName.js, I copied the code and modified in a way to make sure it will generate unique names in production. I refactored rule counter outside of createGenerateClassName for different instance of generateClassName don't conflict with each other.

Here you can find my code for createGenerateClassName:

https://gist.github.com/code-guru/cd6e74f287bcda0d456d8087e7b5ca74

[kelly-tock]

tracing down this same issue, just happening for a customization of Button.

Okay. I can confirm that config.optimization.sideEffects = false in next.config.js fixes both the dialog and avatar issue. I'll add that that particular Webpack config change also increased the projects initial JS payload size by about ~80kb after making the change.

This worked for me, but can confirm that it increased bundle size, for me it was 100kb increase as reported by next.js cli, so kind of a non starter.

I then was experimenting with the suggestion in the FAQ on the material ui site, by making sure to wrap with a StylesProvider as mentioned here:

https://material-ui.com/getting-started/faq/#why-arent-my-components-rendering-correctly-in-production-builds

Should StylesProvider be inside of ThemeProvider, or outside of it? none of the examples have both.

for example, in next.js _app.tsx:

// outside of function component
const generateClassName = createGenerateClassName({
  productionPrefix: 'c',
});

<ThemeProvider theme={theme}>
  <StylesProvider injectFirst generateClassName={generateClassName}>
    <CssBaseline />
     <Component {...pageProps} />
  </StylesProvider>
</ThemeProvider>

if I do a production prefix, it breaks completely. if I leave the options empty, it renders, but I still have the same issue.

in trying @Code-guru 's suggestion above, the app is back to a broken state. I set up my app initially following the next js material ui example app:

https://github.com/mui-org/material-ui/tree/master/examples/nextjs

i'll get a minimal working repo up soon. The only overriding i'm doing to button is through the classes api.

[kelly-tock]

update, I was able to fix this. in my wrappers for mui, I was importing in a few places in the style of destructuring from @material-ui/core, so example of

import { Button, ButtonProps } from '@material-ui/core';

I then had a component that was in my wrapper, that was outside of the wrapper directory. I changed the button wrapper to do direct imports instead:

import Button, { ButtonProps } from '@material-ui/core/Button';

and things started working again. lesson learned, for wrappers in my case, import directly from material component.