Material-ui: [styles] className contains invalid characters

[numToStr]

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

CSS classes should not contains these characters i.e /()

Current Behavior 😯

Class name generated by the @material-ui/styles/styled contains some invalid characters i.e () which are attached to the DOM element.

Steps to Reproduce 🕹

Just use this component

import React, { memo } from "react";
import Paper from "@material-ui/core/Paper";
import styled from "@material-ui/styles/styled";

const MyPaper = styled(Paper)({
    padding: "12px 24px"
});

const Surface = ({ children, ...props }) => (
    <MyPaper {...props}>{children}</MyPaper>
);

export default memo(Surface);

Your Environment 🌎

| Tech | Version |
|--------------|---------|
| Material-UI | 4.0.0-beta.0 |
| Material-UI Styles | 4.0.0-beta.0 |
| React | 16.8.6 |
| Browser | Chromium: 74.0.3729.108 |

[oliviertassinari]

@vkasraj The logic did change between v3 and v4.

• In v3, we were using a custom regexp:
  https://github.com/mui-org/material-ui/blob/aa82746fdecb4efdcb58079d9e66f6a89f5056b8/packages/material-ui-styles/src/createGenerateClassName.js#L4
• In v4, the logic was moved to the core of JSS, with one important difference. The class name escaping logic is using the native browser API when possible:

const escapeRegex = /([[\].#*$><+~=|^:(),"'`\s])/g

export default str => {
  // We don't need to escape it in production, because we are not using user's
  // input for selectors, we are generating a valid selector.
  if (process.env.NODE_ENV === 'production') return str

  const nativeEscape = typeof CSS !== 'undefined' && CSS.escape

  return nativeEscape ? nativeEscape(str) : str.replace(escapeRegex, '\\$1')
}

https://github.com/cssinjs/jss/blob/7c07db4b3ed19b4c451f1f6b7f2d74050ad548ef/packages/jss/src/utils/escape.js#L8

The output is valid cc @kof. Could you provide more details on your problem? For instance, I would tend to agree with the fact that \( makes it hard to read, it can be "scary", - would "look better".

[numToStr]

@oliviertassinari Yeah, It kind of scary to look at \(. If ain't broke don't fix it 😁.

[oliviertassinari]

@vkasraj We might want to revert the logic then 🤔
@kof Do we have a performance gain at using CSS.escape?

[kof]

There is a difference in perf, but it's on micro level https://esbench.com/bench/5cc826044cd7e6009ef6231e

I used it because this is exactly the thing that we need there, escaping CSS, more about semantics. I am not sure the concern of a single person with explanation "It kind of scary to look at" is a big enough reason to change that. I would do it if more people find it "scary" whatever that means.

[oliviertassinari]

Thanks for the feedback. I'm proposing the following: we introduce the regexp logic back. We only handle the parenthesis case. It's a common pattern in React to add parenthesis around component display names. I have asked this question myself during the alpha. We can have a shorter class name in dev,hence easier to debug. You won't see cases like this one when using styled-components (maybe I'm wrong, I haven't seen any yet).

[kof]

is one user, who doesn't know classes can be escaped, a good reason? is there more to it?

[kof]

MUI has so many user, if this is the only one who has a problem with it, I would say we should ignore.

[kof]

Another thought: WithStyles(SomeComponent) actually reads nicely, you can also see the HOC layering, it's a good info.

[oliviertassinari]

+90% of the people are using v3 that has this old regexp logic. Let's have more people transition to v4, see how they respond. My intuition tells me we should revert, but I can be wrong. It's better to run this experimentation and learn from it🔬:). Let's wait.

[kof]

Agree

[numToStr]

I initially thought that it was a bug. I am not saying that you should change this CSS.escape. And I think It should not be a problem for many users as It does not breaks anything.

[acostajake]

In a project upgrading from v3 to v4 broke a few HOCs and I realized it was because their displayNames had spaces. Would this fix apply to display names as well?

[oliviertassinari]

This issue is being resolved in v5 thanks to #22342, the generated classes don't leverage the display name of the underlying component.