[system] Fix duplicated styles in Box

[iamxukai]

fixes #33443

The Box should not forward the sx prop to the override component. We can overwrite the shouldForwardProp function of the @mui/styled-engine to solve the problem.

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 3d0d5d6

[mnajdova]

Actually, I believe we should just use the styled() from the @mui/system instead of @mui/styled-engine and that would solve the issue. This is what I mean:

index 60ef467b72..3365f69e58 100644
--- a/packages/mui-system/src/createBox.js
+++ b/packages/mui-system/src/createBox.js
@@ -1,6 +1,6 @@
 import * as React from 'react';
 import clsx from 'clsx';
-import styled from '@mui/styled-engine';
+import styled from './styled';
 import defaultStyleFunctionSx, { extendSxProp } from './styleFunctionSx';
 import useTheme from './useTheme';

We would need to maybe apply this to the other system components too. Lastly, let's add test for this :)

[iamxukai]

@mnajdova I agree with you. It would seem that If we use the styled() from the @mui/system instead of @mui/styled-engine, we should give the skipSx option with value true.

index 60ef467b72..aa00bbfc6f 100644
--- a/packages/mui-system/src/createBox.js
+++ b/packages/mui-system/src/createBox.js
@@ -1,6 +1,6 @@
 import * as React from 'react';
 import clsx from 'clsx';
-import styled from '@mui/styled-engine';
+import styled from './styled';
 import defaultStyleFunctionSx, { extendSxProp } from './styleFunctionSx';
 import useTheme from './useTheme';

@@ -11,7 +11,7 @@ export default function createBox(options = {}) {
     generateClassName,
     styleFunctionSx = defaultStyleFunctionSx,
   } = options;
-  const BoxRoot = styled('div')(styleFunctionSx);
+  const BoxRoot = styled('div', { skipSx: true })(styleFunctionSx);

   const Box = React.forwardRef(function Box(inProps, ref) {
     const theme = useTheme(defaultTheme);

[iamxukai]

Even though I apply this (including a true skipSx option) to Container, the component is still the case. The following styles seem to be duplicated:

material-ui/packages/mui-system/src/Container/createContainer.tsx

Lines 77 to 128 in 58b5d72

	const ContainerRoot = createStyledComponent(
	({ theme, ownerState }: StyleFnProps<Theme>) =>
	({
	width: '100%',
	marginLeft: 'auto',
	boxSizing: 'border-box',
	marginRight: 'auto',
	display: 'block', // Fix IE11 layout when used with main.
	...(!ownerState.disableGutters && {
	paddingLeft: theme.spacing(2),
	paddingRight: theme.spacing(2),
	// @ts-ignore module augmentation fails if custom breakpoints are used
	[theme.breakpoints.up('sm')]: {
	paddingLeft: theme.spacing(3),
	paddingRight: theme.spacing(3),
	},
	}),
	} as Interpolation<StyleFnProps<Theme>>),
	({ theme, ownerState }: StyleFnProps<Theme>) =>
	ownerState.fixed &&
	Object.keys(theme.breakpoints.values).reduce((acc, breakpointValueKey) => {
	const breakpoint = breakpointValueKey;
	const value = theme.breakpoints.values[breakpoint as Breakpoint];
	if (value !== 0) {
	// @ts-ignore
	acc[theme.breakpoints.up(breakpoint)] = {
	maxWidth: `${value}${theme.breakpoints.unit}`,
	};
	}
	return acc;
	}, {}),
	({ theme, ownerState }: StyleFnProps<Theme>) => ({
	// @ts-ignore module augmentation fails if custom breakpoints are used
	...(ownerState.maxWidth === 'xs' && {
	// @ts-ignore module augmentation fails if custom breakpoints are used
	[theme.breakpoints.up('xs')]: {
	// @ts-ignore module augmentation fails if custom breakpoints are used
	maxWidth: Math.max(theme.breakpoints.values.xs, 444),
	},
	}),
	...(ownerState.maxWidth &&
	// @ts-ignore module augmentation fails if custom breakpoints are used
	ownerState.maxWidth !== 'xs' && {
	// @ts-ignore module augmentation fails if custom breakpoints are used
	[theme.breakpoints.up(ownerState.maxWidth)]: {
	// @ts-ignore module augmentation fails if custom breakpoints are used
	maxWidth: `${theme.breakpoints.values[ownerState.maxWidth]}${theme.breakpoints.unit}`,
	},
	}),
	}),
	);

[mnajdova]

@iamxukai ignore what I've said, we want to keep the Box as light as we can, so just adding the shouldForwardProp logic sounds great. I am not sure we need the ownerState there tough. cc @siriwatknp for a final review.

[siriwatknp]

👍 Thanks for your first contribution! look forward to see more.

[siriwatknp]

@iamxukai I pushed a commit to just remove ownerState from the shouldForwardProp and add a couple of tests. I think we're good!

will merge once the CIs are green.

[Him-2C]

I got many warnings when passing some props style with styled function

So what happen when I put many props attribute?

[siriwatknp]

I got many warnings when passing some props style with styled function

So what happen when I put many props attribute?

Can you open a new issue with a code sandbox reproduction?

[Him-2C]

@siriwatknp sure try this and check console.log
https://codesandbox.io/s/gallant-lamport-sn25cz?file=/src/App.tsx

[siriwatknp]

@siriwatknp sure try this and check console.log https://codesandbox.io/s/gallant-lamport-sn25cz?file=/src/App.tsx

There is no need to styled the Box, do this instead:

const BoxWrapper = styled('div')((props: BoxWrapperProps) => ({
  color: "#FFF",
  backgroundImage: props.watermarkUrl ? `url(${props.watermarkUrl})` : "none"
}));

[Him-2C]

@siriwatknp Sorry that I mean example code, what about if I want to use Paper component or something else ?