composeClasses should append keys from classes, not prepend them before the result of getUtilityClass

janus-reith commented 3 years ago

[x] The issue is present in the latest release.
[x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Right now, styling a component by passing classes is not possible anymore. This might be a most simple example:

.myClassName {
  backgroundColor: "green"
}

[...]

<Button color="primary" classes={{ root: "myClassName"  }}  />;

The button background would still have the primary color. The resulting className on the element would be(leaving aside focus and so on): "myClassName MuiButton-root"

Expected Behavior 🤔

The custom class would have priority over the internal one: "MuiButton-root myClassName"

Steps to Reproduce 🕹

Steps:

Take any MUI component that accepts classes
Pass a custom class, verify that MUI classes are appended after and overwrite it.

Context 🔦

This behavior is new in v5 - Passing classes is a common pattern in v4 to customize MUI Components. This is a blocker for an easy transition to v5 using tss-react

Solution 🚀

This could be fixed in a two-line change by moving this: https://github.com/mui-org/material-ui/blob/64c527a818756ae1b0ba88a3236f11b6060a7315/packages/material-ui-unstyled/src/composeClasses/composeClasses.ts#L18

to come before the if-block here: https://github.com/mui-org/material-ui/blob/64c527a818756ae1b0ba88a3236f11b6060a7315/packages/material-ui-unstyled/src/composeClasses/composeClasses.ts#L15

I don't know if there is any internal use of passing classes which might break due to that change since it expects the opposite behavior - should be rather simple to verify though.

Your Environment 🌎

`npx @material-ui/envinfo`

``` System: OS: macOS 11.5.1 Binaries: Node: 16.6.0 - ~/.nvm/versions/node/v16.6.0/bin/node Yarn: 1.22.11 - ~/.nvm/versions/node/v16.6.0/bin/yarn npm: 7.19.1 - ~/.nvm/versions/node/v16.6.0/bin/npm Browsers: Chrome: 92.0.4515.159 Edge: Not Found Firefox: 89.0.1 Safari: 14.1.2 npmPackages: @emotion/react: ^11.4.1 => 11.4.1 @emotion/styled: ^11.3.0 => 11.3.0 @material-ui/core: ^5.0.0-beta.4 => 5.0.0-beta.4 @material-ui/private-theming: 5.0.0-beta.4 @material-ui/styled-engine: ^5.0.0-alpha.11 => 5.0.0-alpha.11 @material-ui/system: 5.0.0-beta.4 @material-ui/types: 6.0.2 @material-ui/unstyled: 5.0.0-alpha.43 @material-ui/utils: 5.0.0-beta.4 @types/react: 17.0.14 => 17.0.14 react: 17.0.2 => 17.0.2 react-dom: ^17.0.2 => 17.0.2 typescript: 4.3.5 => 4.3.5 ```

eps1lon commented 3 years ago

Thanks for the feedback.

Could you provide a concrete example of customization that is no longer working as expected in 5.x?

To be clear: The order of classnames in the class attribute does not affect styles since the order of classnames does not affect specificity.

mnajdova commented 3 years ago

One more thing. Did you follow the Interoperability guide in case if you are using plain CSS for customization?

janus-reith commented 3 years ago

One more thing. Did you follow the Interoperability guide in case if you are using plain CSS for customization?

Yes, I checked that and also read through all migration guides.

janus-reith commented 3 years ago

Thanks for the feedback.

Could you provide a concrete example of customization that is no longer working as expected in 5.x?

Here I made a quick example hat shows different ways of styling using classes which all fail: https://stackblitz.com/edit/nextjs-7yaawl?file=src%2Fcomponents%2FsomeButton.tsx

tss-react using @emotion/react
Global CSS using @emotion/react and a static class name
Plain CSS Modules
Plain CSS in a style tag (Using _document in next.js)

To be clear: The order of classnames in the class attribute does not affect styles since the order of classnames does not affect specificity.

I would disagree. Especially with @emotion/react being used internally in MUI and in custom styles, with both sharing the same global cache, that seems to be the only thing relevant for specificity if I'm not missing something?

However it also seems to be the case outside of that ecosystem e.g. when using plain CSS classes.

janus-reith commented 3 years ago

Preparing a PR now.

mnajdova commented 3 years ago

@janus-reith can you prepare codesandbox without tss-react? If the problem is the integration with it, we could propagate the issue there. Also, for nextjs, we already have a template example that could be use as a starting point: https://github.com/mui-org/material-ui/tree/next/examples/nextjs-with-typescript

eps1lon commented 3 years ago

I would disagree. Especially with @emotion/react being used internally in MUI and in custom styles, with both sharing the same global cache, that seems to be the only thing relevant for specificity if I'm not missing something?

There's nothing to disagree. classname order not affecting specificity is a fact. If browsers do consider that for specificity then that's a browser bug since the CSS spec does not consider classname order.

Specificity is a weight that is applied to a given CSS declaration, determined by the number of each selector type in the matching selector. When multiple declarations have equal specificity, the last declaration found in the CSS is applied to the element.

-- https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity#how_is_specificity_calculated

janus-reith commented 3 years ago

@janus-reith can you prepare codesandbox without tss-react? If the problem is the integration with it, we could propagate the issue there. Also, for nextjs, we already have a template example that could be use as a starting point: https://github.com/mui-org/material-ui/tree/next/examples/nextjs-with-typescript

In my example above I included different examples, tss-react being one of them. But sure, I can create the same based on your example.

mnajdova commented 3 years ago

Great! My point was, nextjs requires some configuring of emotion and you have direct access to where you inject new style tags (you can add them after emotion). This should fix at least the plain CSS

janus-reith commented 3 years ago

There's nothing to disagree. classname order not affecting specificity is a fact. If browsers do consider that for specificity then that's a browser bug since the CSS spec does not consider classname order.

Specificity is a weight that is applied to a given CSS declaration, determined by the number of each selector type in the matching selector.

Sorry, could it be that we put a different meaning to the term "specificity" here? I'm not referring to the specificity of some css selector here -> Sure, fully agree, if one is more specific than another one, the order of classname would not change that.

From your quote:

When multiple declarations have equal specificity, the last declaration found in the CSS is applied to the element.

That exactly is my point. Scoped to that element, both have the same specificity, they are classnames which dont have any other selector logic involved. So if my custom class name why I pass as prop is applied before the internal one, and the rule is to use the last declaration when multiple exist, I would describe that as the custom class having less specificity(not selector specificity), however I won't insist on having the correct terminology, would you agree if refer to it as "priority" instead?

janus-reith commented 3 years ago

Great! My point was, nextjs requires some configuring of emotion and you have direct access to where you inject new style tags (you can add them after emotion). This should fix at least the plain CSS

Sure, will create an example using the default example, that might rule out any configuration differences in _document/_app for next.js

janus-reith commented 3 years ago

@mnajdova Here is a new demo, based on the default next-with-typescript example: https://stackblitz.com/edit/node-rr7cup?file=pages%2Findex.tsx

mnajdova commented 3 years ago

I've fixed the plain CSS and CSS modules: https://stackblitz.com/edit/node-vfbg4t?file=pages%2F_document.tsx

The changes required were:

add prepend: true on the emotion cache
move the style tag in _document.tsx after the emotion style tags are spread, line: 86 - this will ensure SSR work as expected

For the CSS modules, there were no styles, so I've just added the class there to set the background color :)

For the Global styles, I would say it is expected, as usually Global styles are used for setting some defaults, not for overriding. If you strongly think this should not work like that, you can change the order of the style tags created by the extractCriticalToChunks - line 68 in the _document.tsx file

eps1lon commented 3 years ago

That exactly is my point. Scoped to that element, both have the same specificity, they are classnames which dont have any other selector logic involved.

When multiple declarations have equal specificity, the last declaration found in the CSS is applied to the element.

That exactly is my point. Scoped to that element, both have the same specificity, they are classnames which dont have any other selector logic involved.

The spec references the order of declarations in the document e.g.

<style>
.foo {}
/* this one will override .firstClassName since it comes later*/
.bar {}
</style>

it does not refer to the order of classnames in the attribute.

To reword: Both of the following elements will have the same styles in any spec-compliant browsers. I'm not aware of any user agent that generates different styles for these: <div class="foo bar" /><div class="bar foo" />.

The only thing that would change the styles (of both elements) is re-ordering the declaration of the foo and bar rule i.e. going from

<style>
.foo {}
/* this one will override .firstClassName since it comes later*/
.bar {}
</style>

to

<style>
.bar {}
/* this one will override .firstClassName since it comes later*/
.foo {}
</style>

So if my custom class name why I pass as prop is applied before the internal one, and the rule is to use the last declaration when multiple exist, I would describe that as the custom class having less specificity(not selector specificity), however I won't insist on having the correct terminology, would you agree if refer to it as "priority" instead?

I would strongly advise against defining new terminology in Material-UI. CSS is hard enough to learn. Diverging from what you're used to, would be highly confusing.

mnajdova commented 3 years ago

I would strongly advise against defining new terminology in Material-UI. CSS is hard enough to learn. Diverging from what you're used to, would be highly confusing.

Agree 100%

eps1lon commented 3 years ago

But you don't have to believe me. You can verify it yourself: https://codesandbox.io/s/order-of-classes-in-attributes-does-not-matter-dyt1s

janus-reith commented 3 years ago

Thank you @mnajdova!

add prepend: true on the emotion cache

Hm, I am sure I used this before in a different example and it did not have an affect, will need to go though some tests again.

move the style tag in _document.tsx after the emotion style tags are spread, line: 86 - this will ensure SSR work as expected

I see, interesting, so in that case the class actually gets prioritized although it is not the last in the list and both reference a classname, I didn't get that.

For the CSS modules, there were no styles, so I've just added the class there to set the background color :)

Sorry, my bad, appareantly StackBlitz didn't copy something here. Yes, thank you, that's the same I had there.

For the Global styles, I would say it is expected, as usually Global styles are used for setting some defaults, not for overriding

Got it, thx. No I agree, I wouldn't want to change that order, it makes sense that the global styles come first.

But this still would leave me with the issue when using classes from the same emotion cache, that comes up when using tss-react. They are injected on the same level like the MUI ones if I'm not mistaking something, and in that case, the custom ones should be prioritized, that's what my PR is trying to solve. I would argue that this is is not even about css, but a general expectation with props on React components, except for maybe some special cases. If it has some internal defaults with some object, and the logic is to merge that default object with some that I pass from outside, the keys from the one outside would come after while merging/replace them, and not the other way around.

janus-reith commented 3 years ago

Thanks for taking the time to write this up @eps1lon! Yes it looks like I was under the wrong impression on that one.

mnajdova commented 3 years ago

move the style tag in _document.tsx after the emotion style tags are spread, line: 86 - this will ensure SSR work as expected

I see, interesting, so in that case the class actually gets prioritized although it is not the last in the list and both reference a classname, I didn't get that.

This is exactly @eps1lon's point, it works now because the <style /> tag is inserted after the emotion's. It has nothing to do with the order of the className prop in the react component.

For tss-react, I am not sure what the custom nextjs helpers do, I would suggest start looking there.

janus-reith commented 3 years ago

For tss-react, I am not sure what the custom nextjs helpers do, I would suggest start looking there.

Hm, I had just used these helpers in the sandbox before to make a simple example based on the default example they had there.

In my actual project that I'm trying to convert to v5, I base the logic off the example in the material-ui repo, with no specific logic for tss-react in _document/_app.js but just the default emotion ssr configuration and also prepend: true applied on the cache. But yeah, it indeed looks like this issue needs to be fixed within tss-react somehow then - Not sure if possible and how, but the emotion styles I define in some component would need to come after the ones generated from the MUI component rendered inside that component in the created CSS if I'm not mistaken.

mnajdova commented 3 years ago

Ok, going to close this one then. Feel free to link this issue if you open a new one there so that people will have context.

janus-reith commented 3 years ago

Ok, going to close this one then. Feel free to link this issue if you open a new one there so that people will have context.

Yup, already done earlier: https://github.com/garronej/tss-react/issues/17

garronej commented 3 years ago

@mnajdova Everything now works as expected in the latest version of tss-react. @janus-reith thanks for reporting the issue. ref

mui / material-ui