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.8k stars 32.25k forks source link

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

Open newrice opened 4 years ago

newrice commented 4 years ago

Current Behavior 😯

Expected Behavior πŸ€”

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 πŸ”¦

Your Environment 🌎

Tech Version
Material-UI v4.9.9
React v16.13.1
Browser chrome 80.0.3987.149
oliviertassinari commented 4 years ago

@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 commented 4 years ago

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

newrice commented 4 years ago

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

oliviertassinari commented 4 years ago

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

newrice commented 4 years ago

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

oliviertassinari commented 4 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

mnajdova commented 3 years ago

Ah my bad, was looking into older version.

oliviertassinari commented 3 years ago

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 commented 3 years ago

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

What do you do for SC?

oliviertassinari commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 https://github.com/emotion-js/emotion/issues/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 commented 3 years ago

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.

Semigradsky commented 2 years ago

Still not any solutions?

Andarist commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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.