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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
ImanMahmoudinasab opened this issue Jan 20, 2021 · 14 comments
Closed
2 tasks done
Labels
component: Box The React component. package: system Specific to @mui/system regression A bug, but worse v4.x

Comments

@ImanMahmoudinasab
Copy link
Contributor

ImanMahmoudinasab commented Jan 20, 2021

  • 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 馃槸

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} />
@ImanMahmoudinasab ImanMahmoudinasab added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 20, 2021
@oliviertassinari oliviertassinari added v4.x and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 20, 2021
@oliviertassinari
Copy link
Member

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

@ImanMahmoudinasab
Copy link
Contributor Author

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 21, 2021

I have opened a related issue on JSS: cssinjs/jss#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.

@oliviertassinari oliviertassinari added component: Box The React component. package: system Specific to @mui/system labels Jan 21, 2021
@lichkessel
Copy link

@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
Copy link

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

@oliviertassinari
Copy link
Member

@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.

@GearoidCollins
Copy link

GearoidCollins commented Jan 21, 2021

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
Copy link

@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
Copy link
Member

oliviertassinari commented Jan 21, 2021

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 cssinjs/jss#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

This comment has been minimized.

@Gurmindermultani

This comment has been minimized.

@GearoidCollins

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@CodersGas

This comment has been minimized.

@mui mui locked as resolved and limited conversation to collaborators Jan 22, 2021
@oliviertassinari oliviertassinari added the regression A bug, but worse label Jun 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: Box The React component. package: system Specific to @mui/system regression A bug, but worse v4.x
Projects
None yet
Development

No branches or pull requests

7 participants