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

[ScopedCssBaseline] The box-sizing style of the child components is overwritten #20461

Open
2 tasks done
newrice opened this issue Apr 8, 2020 · 22 comments · Fixed by #20481
Open
2 tasks done

[ScopedCssBaseline] The box-sizing style of the child components is overwritten #20461

newrice opened this issue Apr 8, 2020 · 22 comments · Fixed by #20481
Labels
bug 🐛 Something doesn't work component: CssBaseline The React component package: styled-engine Specific to @mui/styled-engine waiting for 👍 Waiting for upvotes

Comments

@newrice
Copy link
Contributor

newrice commented Apr 8, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

  • ScopedCssBaseline is setting box-sizing: 'inherit' for all child components. -> souce
  • The above takes precedence over CSS classes which generated by material-ui itself (like .MuiInputBase-input ).
  • Therefore, the default box-sizing of material-ui not being used.

Expected Behavior 🤔

  • .MuiInputBase-input should be used as precedence over box-sizing: 'inherit' like CssBaseline.

Steps to Reproduce 🕹

https://codesandbox.io/s/the-box-sizing-style-of-the-child-components-is-overwritten-htv0c

Steps:

  1. Please check box-sizing of each element.

Context 🔦

  • Do not have to set box-sizing to each component.
  • Use Material-UI default styles easier under ScopedCssBaseline like CssBaseline.

Your Environment 🌎

Tech Version
Material-UI v4.9.9
React v16.13.1
Browser chrome 80.0.3987.149
@oliviertassinari oliviertassinari added the component: CssBaseline The React component label Apr 8, 2020
@oliviertassinari
Copy link
Member

@newrice Oh wow, thanks for raising. It's working with CssBasline because the * selector has zero specificity. We need to do something about it. I propose one of these two options:

  1. We document the limitation, developers need to import ScopedCssBaseline first. For instance: https://codesandbox.io/s/the-box-sizing-style-of-the-child-components-is-overwritten-7hle1?file=/src/Demo.js.
  2. We give up on the box-sizing reset.

What do you think?

@oliviertassinari
Copy link
Member

I propose we go with 1. What do you think?

diff --git a/docs/src/pages/components/css-baseline/css-baseline.md b/docs/src/pages/components/css-baseline/css-baseline.md
index 286b8be42..316264d5c 100644
--- a/docs/src/pages/components/css-baseline/css-baseline.md
+++ b/docs/src/pages/components/css-baseline/css-baseline.md
@@ -32,16 +32,20 @@ It's possible to apply the baseline only to the children by using the `ScopedCss
 ```jsx
 import React from 'react';
 import ScopedCssBaseline from '@material-ui/core/ScopedCssBaseline';
+import MyApp from './MyApp';

 export default function MyApp() {
   return (
     <ScopedCssBaseline>
       {/* The rest of your application */}
+      <MyApp />
     </ScopedCssBaseline>
   );
 }
 ```

+> ⚠️ Make sure you import `ScopedCssBaseline` first to avoid box-sizing conflicts as in the above example.
+
 ## Approach

 ### Page

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Apr 9, 2020
@newrice
Copy link
Contributor Author

newrice commented Apr 9, 2020

@oliviertassinari Thank you very much, your comment helped me. I think that document will help beginners like me🙏

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 9, 2020

@newrice We are happy to hear that you solved your issue. Do you want to submit a pull request? :)

@newrice
Copy link
Contributor Author

newrice commented Apr 9, 2020

@oliviertassinari Oh! If it’s ok, I want to do it✨

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 9, 2020

Go for it. The issue with 2. is that it could create surprises for people when switching between the two, it could be a breaking change for existing users (but small impact as a recent feature).

@binh1298
Copy link
Contributor

@oliviertassinari Hi. Is there any way to use ScopedCssBaseline yet still keep the box-sizing of .MuiInputBase-input to be content-box?
I'm using the outlined variant of select component and it seems like it becomes smaller when wrapped by ScopedCssBaseline (because of box-sizing). How can I avoid this behavior or is it meant to be smaller?
Currently I think there's 2 ways to get around this, the first way is to modify the theme, the second way is to pass in the props for box-sizing for every component that uses the .MuiInputBase-input class. Do you have any recommendations on which method should I use?

@oliviertassinari
Copy link
Member

@binh1298 If you are using v4, this issue describes the solution. If you are using v5 with sc, the solution still apply.

If you are using v5 with emotion then I have no idea. I'm reopening.

@mnajdova
Copy link
Member

mnajdova commented Apr 1, 2021

As far as I can see the ScopedCssBaseline component uses the withStyles API, but the component CSSBaseline is created with the new styled() API. Let's try to convert the component and see what we need to do then.

@oliviertassinari
Copy link
Member

@mnajdova Both CssBaseline and ScopedCssBaseline are using the styled API since very recently.

@mnajdova
Copy link
Member

mnajdova commented Apr 2, 2021

Ah my bad, was looking into older version.

@oliviertassinari
Copy link
Member

I have reopened without a reproduction, my bad. I tried in https://codesandbox.io/s/the-box-sizing-style-of-the-child-components-is-overwritten-forked-kze77?file=/src/Demo.js:125-194 but I couldn't see the error. We get this in the <head>:

.MuiScopedCssBaseline-root * {
  box-sizing: inherit;
}

.css-weuz2y-MuiInputBase-input-MuiOutlinedInput-input {
  box-sizing: content-box;
}

which is correct.

Screenshot 2021-04-02 at 21 08 46


Now, it gets interesting, if I render the same demo in the documentation:

Screenshot 2021-04-02 at 21 09 09

This time, the CSS swap:

.css-weuz2y-MuiInputBase-input-MuiOutlinedInput-input {
  box-sizing: content-box;
}

.MuiScopedCssBaseline-root * {
  box-sizing: inherit;
}

As far as I know, there are no solutions with emotion. The trick we have for JSS only works with styled-components. cc @Andarist in case it could interest him.

I would propose we document the limitation and close.

@Andarist
Copy link
Contributor

Andarist commented Apr 2, 2021

The trick we have for JSS only works with styled-components.

What do you do for SC?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 2, 2021

@Andarist From what I understand JSS and SC rely on the JavaScript execution order while emotion relies on prop spreading. The solution I propose is to update:

⚠️ Make sure you import ScopedCssBaseline first to avoid box-sizing conflicts as in the above example.

https://next.material-ui.com/components/css-baseline/#scoping-on-children

@Andarist
Copy link
Contributor

Andarist commented Apr 3, 2021

@Andarist From what I understand JSS and SC rely on the JavaScript execution order while emotion relies on prop spreading.

Not sure if I understand this correctly. With @emotion/react rules are injected when rendering so it somewhat depends on the render order. Not sure how SC works exactly without double-checking that but I would imagine they might work similarly, especially since they have introduced StyleSheetManager which resemebles our CacheProvider

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 4, 2021

Questions

With @emotion/react rules are injected when rendering so it somewhat depends on the render order.

Ok, this is what I recall @mnajdova saying.

Not sure how SC works exactly without double-checking that but I would imagine they might work similarly

@Andarist From what I understand, at the styled() creation time, an injection order is reserved:

Global CSS vs. styled

I have also noticed that they enforce the global style to always be injected first in SC:

I recall looking into emotion-js/emotion#2213, it seems that emotion does the same as SC. Global CSS are injected first: https://codesandbox.io/s/emotion-issue-template-forked-31990?file=/src/index.js:0-639.

Alternative solution

I had also considered solving this issue by using <Global> instead of styled() to leverage the CSS injection order, but we would need to add a unique class name to be stable during the hydration. Maybe we could have a default and ask developers to provide their own id if they want to use multiple ScopedCssBaseline with a different theme. Actually, maybe it's good enough, cc @mnajdova.

Is this issue important?

Not really. We can defer it post v5.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 11, 2021

I have tried the <Global> approach with:

diff --git a/packages/material-ui/src/ScopedCssBaseline/ScopedCssBaseline.d.ts b/packages/material-ui/src/ScopedCssBaseline/ScopedCssBaseline.d.ts
index d21c164ab0..4252db2fe0 100644
--- a/packages/material-ui/src/ScopedCssBaseline/ScopedCssBaseline.d.ts
+++ b/packages/material-ui/src/ScopedCssBaseline/ScopedCssBaseline.d.ts
@@ -14,6 +14,11 @@ export interface ScopedCssBaselineTypeMap<P = {}, D extends React.ElementType =
       /** Styles applied to the root element. */
       root?: string;
     };
+    /**
+     * The class name used to scope the CSS baseline styles.
+     * @default 'MuiScope'
+     */
+    scopeId?: string;
   };
   defaultComponent: D;
 }
diff --git a/packages/material-ui/src/ScopedCssBaseline/ScopedCssBaseline.js b/packages/material-ui/src/ScopedCssBaseline/ScopedCssBaseline.js
index 2b2c30be5a..5839a65197 100644
--- a/packages/material-ui/src/ScopedCssBaseline/ScopedCssBaseline.js
+++ b/packages/material-ui/src/ScopedCssBaseline/ScopedCssBaseline.js
@@ -3,14 +3,10 @@ import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled';
 import useThemeProps from '../styles/useThemeProps';
-import experimentalStyled from '../styles/experimentalStyled';
+import GlobalStyles from '../GlobalStyles';
 import { html, body } from '../CssBaseline/CssBaseline';
 import { getScopedCssBaselineUtilityClass } from './scopedCssBaselineClasses';

-const overridesResolver = (props, styles) => {
-  return styles.root || {};
-};
-
 const useUtilityClasses = (styleProps) => {
   const { classes } = styleProps;

@@ -21,45 +17,41 @@ const useUtilityClasses = (styleProps) => {
   return composeClasses(slots, getScopedCssBaselineUtilityClass, classes);
 };

-const ScopedCssBaselineRoot = experimentalStyled(
-  'div',
-  {},
-  {
-    name: 'MuiScopedCssBaseline',
-    slot: 'Root',
-    overridesResolver,
-  },
-)(({ theme }) => ({
-  /* Styles applied to the root element. */
-  ...html,
-  ...body(theme),
-  '& *, & *::before, & *::after': {
-    boxSizing: 'inherit',
-  },
-  '& strong, & b': {
-    fontWeight: theme.typography.fontWeightBold,
-  },
-}));
+export const styles = (scopeId) => (theme) => {
+  let defaultStyles = {
+    [`&.${scopeId}`]: {
+      ...html,
+      ...body(theme),
+      '& *, & *::before, & *::after': {
+        boxSizing: 'inherit',
+      },
+      '& strong, & b': {
+        fontWeight: theme.typography.fontWeightBold,
+      },
+    },
+  };
+
+  const themeOverrides = theme.components?.MuiScopedCssBaseline?.styleOverrides?.root;
+  if (themeOverrides) {
+    defaultStyles = [defaultStyles, themeOverrides];
+  }
+
+  return defaultStyles;
+};

 const ScopedCssBaseline = React.forwardRef(function ScopedCssBaseline(inProps, ref) {
   const props = useThemeProps({ props: inProps, name: 'MuiScopedCssBaseline' });
-  const { className, component = 'div', ...other } = props;
+  const { scopeId = 'MuiScope', className, component: Component = 'div', ...other } = props;

-  const styleProps = {
-    ...props,
-    component,
-  };
+  const styleProps = props;

   const classes = useUtilityClasses(styleProps);

   return (
-    <ScopedCssBaselineRoot
-      as={component}
-      className={clsx(classes.root, className)}
-      ref={ref}
-      styleProps={styleProps}
-      {...other}
-    />
+    <React.Fragment>
+      <GlobalStyles styles={styles(scopeId)} />
+      <Component ref={ref} className={clsx(classes.root, className, scopeId)} {...other} />
+    </React.Fragment>
   );
 });

@@ -85,6 +77,11 @@ ScopedCssBaseline.propTypes /* remove-proptypes */ = {
    * Either a string to use a HTML element or a component.
    */
   component: PropTypes.elementType,
+  /**
+   * The class name used to scope the CSS baseline styles.
+   * @default 'MuiScope'
+   */
+  scopeId: PropTypes.string,
 };

 export default ScopedCssBaseline;

But it's not working either. The specificity of the CSS selectors are the same, but emotion doesn't guarantee that <Global> is injected before styled() like SC does.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work waiting for 👍 Waiting for upvotes package: styled-engine Specific to @mui/styled-engine and removed good first issue Great for first contributions. Enable to learn the contribution process. labels Apr 11, 2021
@Semigradsky
Copy link
Contributor

Still not any solutions?

@Andarist
Copy link
Contributor

Andarist commented Apr 5, 2022

I got a little bit lost when it comes to the exact role of Emotion in all of this - if you want any help with Emotion, I would appreciate if somebody could explain the issue once more and provide a repro case for it. I could then happily jump into analyzing it.

@Semigradsky
Copy link
Contributor

Today I tried to migrate from @mui/styles to tss-react and got this problem in storybook. I will try to prepare a reproducible example.

@Semigradsky
Copy link
Contributor

Looks like it was something related to storybook, I can't reproduce the issue without it. My problem I fixed by caching the emotion cache in the js map and returning the early created cache instead of creating a new one.

@greglittlefield-wf
Copy link

greglittlefield-wf commented Nov 21, 2022

I recently ran into this issue using the latest MUI (using Emotion).

When trying to find a reduced test case, I found two different cases that trigger the issue, and case 1 ended up being what was happening in my app:

  1. When using multiple ScopedCssBaselines, where at least one has an sx (sandbox link)

    <ScopedCssBaseline>
      <TextField label="Behaves normally" />
    </ScopedCssBaseline>
    
    <ScopedCssBaseline sx={{ color: "red" }}>
      <TextField label="has overridden box-sizing" />
    </ScopedCssBaseline>
  2. When a component is rendered before a ScopedCssBaseline, and then later used inside a ScopedCssBaseline (sandbox link)

     <TextField label="Behaves normally" />
     
     <ScopedCssBaseline>
       <TextField label="has overridden box-sizing" />
     </ScopedCssBaseline>

Both of these cases end up looking like this:

Screen Shot 2022-11-21 at 10 51 56 AM

In both cases, it's due to the ScopedCssBaseline styles getting inserted after the TextField (or, more specifically, the InputBase) styles. See explanation below for more info.

Case 1 is easy to avoid (just don't use sx on ScopedCssBaseline), but I could see case 2 being potentially trickier, though it's probably way less common.

Ideally, I think it'd be nice to remove those & * selectors and "give up on the box-sizing reset", but I understand if that's not possible.

Explanation on what's causing the issue in these cases

This is somewhat redundant with info earlier in the thread, but I thought it'd be helpful to show how these cases were interacting with Emotion, and triggering this issue in slightly different ways.

Case 1

Normally when you render a mui InputBase (via TextField) inside of a ScopedCssBaseline, Emotion generates CSS classes for those styles and inserts them in render order:

1. styles for ScopedCssBaseline, generated CSS class .foo
2. styles for InputBase,         generated CSS class .bar

And if you render the ScopedCssBaseline again, Emotion reuses the .foo class since the styles are exactly the same. So, no new styles are inserted, and you end up with the same order.

However, if you set sx on ScopedCssBaseline, or wrap it in styled, those styles are different than that of the previously rendered ScopedCssBaseline, so instead of reusing the CSS class it generates a new one, and adds that to the end of the style rule list:

1. styles for ScopedCssBaseline,         generated CSS class .foo
2. styles for InputBase,                 generated CSS class .bar
3. styles for ScopedCssBaseline with sx, generated CSS class .qux

And since .qux * and .bar have the same specificity, .qux * wins since it comes after, and thus you have your box-sizing styles getting overridden.

Case 2

If you were to render a TextField before ever rendering a ScopedCssBaseline, let's say elsewhere in the app, those styles get inserted first:

1. styles for InputBase,         generated CSS class .foo

Then, later, if you render a ScopedCssBaseline with a TextField inside it, it reuses the class generated for InputBase, and then adds the ScopedCssBaseline styles afterwards, and you end up with.

1. styles for InputBase,         generated CSS class .foo
2. styles for ScopedCssBaseline, generated CSS class .bar

And since .bar * and .foo have the same specificity, .bar * wins since it comes after, and thus you have your box-sizing styles getting overridden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: CssBaseline The React component package: styled-engine Specific to @mui/styled-engine waiting for 👍 Waiting for upvotes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants