search

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.38k stars 32.14k forks source link

[Box] chrome 88: Failed to execute 'px' on 'CSS': The provided double value is non-finite. #24519

Closed ImanMahmoudinasab closed 3 years ago

ImanMahmoudinasab commented 3 years ago

Current Behavior 😯

Setting undefined value to the margin, padding,... props of a Box component would throw the following exception. This issue only is reproducible on chrome 88.

Failed to execute 'px' on 'CSS': The provided double value is non-finite.

Expected Behavior 🤔

Should ignore undefined value for props.

Steps to Reproduce 🕹

https://codesandbox.io/s/box-issue-with-undefined-forked-hxp13

<Box m={undefined} />
oliviertassinari commented 3 years ago

Note that it doesn't reproduce in v5: https://codesandbox.io/s/box-issue-with-undefined-forked-y4f2g?file=/demo.js.

ImanMahmoudinasab commented 3 years ago

I've started investigating the issue to fix it. Any comment is welcomed.

oliviertassinari commented 3 years ago

I have opened a related issue on JSS: https://github.com/cssinjs/jss/issues/1445 as it could be one great resolution path.

We also had a close issue #24182 that was about developers doing: -theme.spacing(1) which returns NaN, they should do theme.spacing(-1) instead.

lichkessel commented 3 years ago

@ImanMahmoudinasab I've got the same error. To reproduce: Set calculated property to a class: paddingLeft: props => theme.spacing(2 * (props.level - 1)) Use that class with useStyles(undefined). I do not really know how px function was implemented, but it throws an exception if NaN is passed as an argument to it. Also, possibly theme.spacing function must not return NaN instead of undefined.

lichkessel commented 3 years ago

@oliviertassinari Also, I'd like to laugh with you on this: typeof NaN is number. Not a number is a number. Very nice :)

oliviertassinari commented 3 years ago

@lichkessel I'm taking care of the spacing(undefined) => NaN transformation happening in the system in #24527, it's wrong.

Regarding theme.spacing(NaN) => NaN I think that it's expected, and should be fixed in https://github.com/cssinjs/jss/issues/1445.

GearoidCollins commented 3 years ago

I don't know if this is related but we are experiencing something similar with users that upgraded to Chrome 88.

Screenshot 2021-01-21 at 13 25 30

This issue for us seems to be related <Dialog /> https://material-ui.com/api/dialog/

lichkessel commented 3 years ago

@lichkessel I'm taking care of the spacing(undefined) => NaN transformation happening in the system in #24527, it's wrong.

Regarding theme.spacing(NaN) => NaN I think that it's expected, and should be fixed in cssinjs/jss#1445.

Maybe it is worth to silently fall back to undefined value everywhere we do not know what value is passed as a parameter ? At the same time I doubt it could be implemented ad-hoc since this is a policy and I'm not sure it is common for material-ui.

oliviertassinari commented 3 years ago

The issue with the Box is fixed in #24527. It will be released soon or later. Regarding the other cases, make sure you are not providing NaN as a CSS value. please continue the discussion on https://github.com/cssinjs/jss/issues/1445 (the best place to fix the root cause)

If you find a place where the NaN is generated from Material-UI, and you have a reproduction to prove it like it was the case with the Box in this issue (#24519), please open a new issue.

nickbreid commented 3 years ago

thanks for making this issue @ImanMahmoudinasab, and @oliviertassinari for your comments. it helped my team figure out our error. hopefully this thread will come up for others on Google as more people update Chrome and notice it affecting them

Gurmindermultani commented 3 years ago

I don't know if this is related but we are experiencing something similar with users that upgraded to Chrome 88.

Screenshot 2021-01-21 at 13 25 30

This issue for us seems to be related <Dialog /> https://material-ui.com/api/dialog/

I am also getting this bug on production right now, not fixed with the patch too.

GearoidCollins commented 3 years ago

Hi @Gurmindermultani, we managed to put a hotfix out for this by overwriting the default paperWidthXs like this:

createMuiTheme({
  ...
  overrides: {
    MuiDialog: {
      paperWidthXs: { maxWidth: false },
    },
  },
})

Not sure if this will help in your case, but it might help with you guys finding a solution.

oliviertassinari commented 3 years ago

Not sure if this will help in your case, but it might help with you guys finding a solution.

@GearoidCollins Probably because Math.max('10px', 10) // NaN. We assume that theme.breakpoints.values are unitless values.

CodersGas commented 3 years ago

How can this thread be closed. The issue hasn't been resolved yet.