Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiled css precedence #16609

Closed
2 tasks done
Luucky opened this issue Jul 15, 2019 · 40 comments · Fixed by #18238
Closed
2 tasks done

compiled css precedence #16609

Luucky opened this issue Jul 15, 2019 · 40 comments · Fixed by #18238
Assignees
Labels
bug 🐛 Something doesn't work package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. priority: important This change can make a difference v4.x

Comments

@Luucky
Copy link

Luucky commented Jul 15, 2019

When a project is compiled, the precedence of applied CSS rules changes.

  • 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 🤔

.MuiDialogActions-spacing > * + * {
    margin-left: 8px;
}

... other styles ...

.MuiButtonBase-root {
    color: inherit;
    border: 0;
    margin: 0;
    cursor: pointer;
    display: inline-flex;
    outline: none;
    padding: 0;
    position: relative;
    align-items: center;
    user-select: none;
    border-radius: 0;
    vertical-align: middle;
    justify-content: center;
    -moz-appearance: none;
    text-decoration: none;
    background-color: transparent;
    -webkit-appearance: none;
    -webkit-tap-highlight-color: transparent;
}

Current Behavior 😯

.MuiButtonBase-root {
    color: inherit;
    border: 0;
    margin: 0;
    cursor: pointer;
    display: inline-flex;
    outline: none;
    padding: 0;
    position: relative;
    align-items: center;
    user-select: none;
    border-radius: 0;
    vertical-align: middle;
    justify-content: center;
    -moz-appearance: none;
    text-decoration: none;
    background-color: transparent;
    -webkit-appearance: none;
    -webkit-tap-highlight-color: transparent;
}

... other styles ...

.MuiDialogActions-spacing > * + * {
    margin-left: 8px;
}

Steps to Reproduce 🕹

  1. Run a demo of Dialog on localhost to see the expected behavior
  2. Compile the demo and run to see the marginLeft of MuiDialogActions is overridden by margin of MuiButtonBase

Your Environment 🌎

Tech Version
Material-UI v4.1.3
React v16.8.6
Browser Chrome
@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label Jul 15, 2019
@oliviertassinari
Copy link
Member

@Luucky Please provide a full reproduction test case. This would help a lot 👷 .
A live example would be perfect. This codesandbox.io template may be a good starting point. Thank you!

@Luucky
Copy link
Author

Luucky commented Jul 16, 2019

@oliviertassinari Here's a link for the DEV EXAMPLE: https://codesandbox.io/embed/create-react-app-36pid
And here's a production build link: https://csb-36pid.netlify.com/

Notice how space between buttons disappears. The margin of ButtonBase overrides the DialogActions left of margin.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. and removed status: waiting for author Issue with insufficient information good first issue Great for first contributions. Enable to learn the contribution process. labels Jul 16, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 16, 2019

The CSS specificity of .x > * + * is 1 then we rely on

import '../Button'; // So we don't have any override priority issue.

to get the CSS injection order right.

The alternative is to increase the specificity to 2. I like this idea, it would make use component agnostic (work with everybody, not just our Button)

diff --git a/packages/material-ui/src/CardActions/CardActions.js b/packages/material-ui/src/CardActions/CardActions.js
index ee57f0120..91b7c574a 100644
--- a/packages/material-ui/src/CardActions/CardActions.js
+++ b/packages/material-ui/src/CardActions/CardActions.js
@@ -2,7 +2,6 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import withStyles from '../styles/withStyles';
-import '../Button'; // So we don't have any override priority issue.

 export const styles = {
   /* Styles applied to the root element. */
@@ -13,7 +12,7 @@ export const styles = {
   },
   /* Styles applied to the root element if `disableSpacing={false}`. */
   spacing: {
-    '& > * + *': {
+    '& > :not(:first-child)': {
       marginLeft: 8,
     },
   },
diff --git a/packages/material-ui/src/DialogActions/DialogActions.js b/packages/material-ui/src/DialogActions/DialogActions.js
index 6fb342f5c..458c61350 100644
--- a/packages/material-ui/src/DialogActions/DialogActions.js
+++ b/packages/material-ui/src/DialogActions/DialogActions.js
@@ -2,7 +2,6 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import withStyles from '../styles/withStyles';
-import '../Button'; // So we don't have any override priority issue.

 export const styles = {
   /* Styles applied to the root element. */
@@ -15,7 +14,7 @@ export const styles = {
   },
   /* Styles applied to the root element if `disableSpacing={false}`. */
   spacing: {
-    '& > * + *': {
+    '& > :not(:first-child)': {
       marginLeft: 8,
     },
   },
diff --git a/packages/material-ui/src/ExpansionPanelActions/ExpansionPanelActions.js b/packages/material-ui/src/ExpansionPanelActions/ExpansionPanelActions.js
index 5451c6387..2e3f9f179 100644
--- a/packages/material-ui/src/ExpansionPanelActions/ExpansionPanelActions.js
+++ b/packages/material-ui/src/ExpansionPanelActions/ExpansionPanelActions.js
@@ -2,7 +2,6 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import withStyles from '../styles/withStyles';
-import '../Button'; // So we don't have any override priority issue.

 export const styles = {
   /* Styles applied to the root element. */
@@ -14,7 +13,7 @@ export const styles = {
   },
   /* Styles applied to the root element if `disableSpacing={false}`. */
   spacing: {
-    '& > * + *': {
+    '& > :not(:first-child)': {
       marginLeft: 8,
     },
   },

cc @eps1lon as you were advocating for removing the import (tree-shaking)
cc @NMinhNguyen as you were wondering about it

@Luucky I can't explain the behavior of the deployment. Something is wrong in the bundling pipeline independently from my previous comment about CSS specificity. I can reproduce the problem with our CRA and Next.js examples.

@Luucky
Copy link
Author

Luucky commented Jul 16, 2019

I also had a similar thing happen when I tried the following:

<List>       
       <ListItem button component={Box} border={1}>
               Click this item
       </ListItem>
</List>

ButtonBase would reset my border also after deployment.
I fixed it by doing the following, I don't know which is a better practice:

<List>       
       <Box border={1}>
                <ListItem button>
                        Click this item
                 </ListItem>
       </Box>
</List>

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 16, 2019

ButtonBase would reset my border also after deployment.

@Luucky This one is expected. Unless you control the import order, the CSS win is unpredictable.

@canhbk
Copy link

canhbk commented Jul 18, 2019

i have the same issue with @Luucky

@dfmartin
Copy link

I'm running into an issue that may be related. If not, I will open another issue.

When building my react app in production mode ($ npm run build) the order of the MUI style elements is getting mixed up with my own jss style elements. However, if I run it in development ($ npm start) everything is as expected. This is project is using 4.2.0. I have a similar project that is using 3.9.1 and it works the same both production/dev (i.e. correctly as I would expect).

it seems the production mode is the issue, but I do not know where to begin looking.

@nickpalmer
Copy link

The same issue exists with .MuiCardActions-spacing.

I just ran into this issue setting up server rendering. Server rendering has them in the right order and React.render renders them in the right order but React.hydrate results in .MuiButtonBase-root being added after .MuiCardActions-spacing which makes the buttons drop the left margin after hydrate.

@oliviertassinari
Copy link
Member

@nickpalmer this symptom might hide another issue.

@nickpalmer
Copy link

@oliviertassinari After further investigation we have the exact same problem on our staging environment, we just never noticed. It thus has nothing to do with hydrate and everything to do with how create-react-app is compiling things for production.

@dva
Copy link

dva commented Sep 5, 2019

Fixed similar behaviour with webpack setting sideEffects to false, which is enabled in production mode by default.

module.exports = {
  //...
  optimization: {
    sideEffects: false
  }
};

https://webpack.js.org/configuration/optimization/#optimizationsideeffects

@nickpalmer
Copy link

nickpalmer commented Sep 5, 2019

@dva I can confirm that this resolves the issue.

@dva
Copy link

dva commented Sep 6, 2019

@nickpalmer final build getting bigger btw.

@kokill
Copy link

kokill commented Sep 6, 2019

it works correctly if we disable SSR. in case of SSR few styles are picked from server css file.
i tried removing muisheet.collect(<App {...props} />) from server side and it resolved the issue currently.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Sep 6, 2019
@oliviertassinari
Copy link
Member

While I can no longer reproduce the issue on Codesandbox, CRA and Next.js, I would propose that we move forward with #16609 (comment) so we can:

  1. Be invariant to custom margins people might implement on the button
  2. Maximize tree-shaking in people code that doesn't use a button in the actions
  3. Help people that have buggy tree-shaking env and that don't use our recommended tree-shaking plugin.

@joshwooding joshwooding added the hacktoberfest https://hacktoberfest.digitalocean.com/ label Sep 30, 2019
@alondahari

This comment has been minimized.

@alexandrecanuto
Copy link

alexandrecanuto commented Oct 9, 2019

I'm running into an issue that may be related. If not, I will open another issue.

When building my react app in production mode ($ npm run build) the order of the MUI style elements is getting mixed up with my own jss style elements. However, if I run it in development ($ npm start) everything is as expected. This is project is using 4.2.0. I have a similar project that is using 3.9.1 and it works the same both production/dev (i.e. correctly as I would expect).

it seems the production mode is the issue, but I do not know where to begin looking.

I have this exact same problem.

EDIT: The workaround for me was adding {index: 1} in all of our components' withStyles, to ensure they were created after all the Material UI ones in production.

export default withStyles(styles, {index: 1})(MyComponent);

@netcoding87
Copy link

We also have the same problem using version 4.3.3. The workaround provided by @alexandrecanuto helped.

It seems like jss.createStyleSheet is inserting new styles always at last when all have same index. If then a MUI component is rendered after makeStyles the order gets wrong. For me it looks like an issue with webpack optimization and jss in production mode vs development mode

Question: Why is this not set to 1 by default for makeStyles? Would this not help in future?

@hibearpanda

This comment has been minimized.

@Kizmar
Copy link

Kizmar commented Oct 20, 2019

Running into the same issue on 4.5.0 - we're not using SSR. Just a typical CRA. We have a shared export const useGlobalStyles = makeStyles(theme => ({ ... })); file we use, and I'm noticing when these are imported using the const globalClasses = useGlobalStyles(); hook and applied with className, they are being stomped on by MUI styles.

In the image example below, the element is used like this:
<Grid container spacing={2} className={globalClasses.gridContainer}>

I would expect the .makeStyles-gridContainer-241 class styles to be more important than the MuiGrid-spacing-xs-2, but that's not the case.

Annotation 2019-10-20 100221

This was working just a few days ago. We're seeing this even when running on localhost during development.

My workaround in this specific case was to change: margin: theme.spacing(0)
to: margin: '${theme.spacing(0)} !important'

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Oct 24, 2019
@Kizmar
Copy link

Kizmar commented Nov 1, 2019

Is there any movement on this? I'm getting more and more areas where I'm having to use !important, which is less than ideal.

@oliviertassinari
Copy link
Member

@Kizmar We would be happy to accept a pull request for #16609 (comment).

@joshwooding joshwooding removed the hacktoberfest https://hacktoberfest.digitalocean.com/ label Nov 3, 2019
@OnkelTem
Copy link

OnkelTem commented Nov 6, 2019

Having the same issue as in #16609 (comment) - i.e. Dev != Prod

A copy-paste:

/* Dev: */
.MuiCardHeader-root {
    display: flex;
    padding: 16px;
    align-items: center;
}
.makeStyles-statusBarOk-276 {
    color: #fff;
    background: #4e698c;
    padding-top: 8px;
    padding-bottom: 8px;
}

/* Prod: */
.jss276 {
    color: #fff;
    background: #4e698c;
    padding-top: 8px;
    padding-bottom: 8px;
}
.MuiCardHeader-root {
    display: flex;
    padding: 16px;
    align-items: center;
}

So the style 276 gets wrong priority. Any ideas how to fix this? Is there a way to provide the priority?

Material UI 4.2.0

@oliviertassinari oliviertassinari self-assigned this Nov 6, 2019
@oliviertassinari oliviertassinari removed the good first issue Great for first contributions. Enable to learn the contribution process. label Nov 6, 2019
@selfdocumentingcode

This comment has been minimized.

@kelly-tock

This comment has been minimized.

@NMinhNguyen

This comment has been minimized.

@kelly-tock

This comment has been minimized.

@kelly-tock

This comment has been minimized.

@NMinhNguyen

This comment has been minimized.

@kelly-tock

This comment has been minimized.

@CodySchaaf

This comment has been minimized.

@jonni-larjomaa

This comment has been minimized.

@amanteaux

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@amanteaux

This comment has been minimized.

@amanteaux

This comment has been minimized.

@oliviertassinari oliviertassinari added package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. v4.x labels Feb 19, 2021
@milos-kuznezov778

This comment has been minimized.

@milos-kuznezov778

This comment has been minimized.

@milos-kuznezov778

This comment has been minimized.

@leandrorazevedo

This comment has been minimized.

@mui mui locked and limited conversation to collaborators Jul 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 Something doesn't work package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. priority: important This change can make a difference v4.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.