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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stylesheet ordering bug with @mui/material and styled-components #547

Open
jkimbo opened this issue Jan 17, 2024 · 9 comments
Open

Stylesheet ordering bug with @mui/material and styled-components #547

jkimbo opened this issue Jan 17, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@jkimbo
Copy link

jkimbo commented Jan 17, 2024 •
edited
Loading

Describe the bug

We are coming across a bug in Ladle where certain styles are being inserted into the <head> in the wrong order resulting in issues with styling. I've attached a reproduction with the Chip component from @mui/material where the custom Chip component should have a red background colour.

I think the issue is with the SynchronizeHead component and the fact that it's overriding the insertRule method on the CSSStyleSheet prototype to create separate style tags in the head (styled-components only uses 1 style tag) and so the browser will treat the later style tags as more specific. I have verified this theory by commenting out the code that overrides the insertRule method in my local node_modules (not sure how to do it in stackblitz).

Original PR: #488

Reproduction

https://stackblitz.com/edit/ladle-hzn2yy?file=src%2Fbasic.stories.tsx

Environment

  • OS: Mac
  • Browser: Chrome
  • Version: 120.0.6099.216
@jkimbo jkimbo added the needs triage needs to be reviewed label Jan 17, 2024
@tajo
Copy link
Owner

tajo commented Jan 18, 2024

I think the issue is with the SynchronizeHead component and the fact that it's overriding the insertRule method on the CSSStyleSheet prototype to create separate style tags in the head (styled-components only uses 1 style tag) and so the browser will treat the later style tags as more specific.

Having a single <style> tag vs multiple doesn't impact the specificity or priority, only the order of defined rules matter. That order is obviously reversed in your repro for some reason so the background is unchanged.

So something is wrong with the order of insertions coming form mui/styled-components or our implementation of addStyleElement / deleteStyleElement is wrong when it comes to positions.

Good step to verify if the issue is on our side would be to console.log arguments of those two functions and verify that rules are being added/deleted with correct indexes (in other words that the SC engine is executing insertRule / deleteRule as you would expect). You could also double-check that the SC engine doesn't use other window.CSSStyleSheet.prototype (deprecated) methods but afaik it shouldn't.

@tajo tajo added bug Something isn't working and removed needs triage needs to be reviewed labels Jan 18, 2024
@jkimbo
Copy link
Author

jkimbo commented Jan 18, 2024

@tajo so I've patched the stackblitz to log out the rule and index and I get the following:

Screenshot 2024-01-18 at 16 37 41

They key part is that it looks like the .hjaLXG (which sets the background to red) is being inserted at index 0 and .UTfgf (the styles from mui) is also being inserted at index 0. It's possible that the native insertRule does something different with clashing indexes but I can't tell from the spec: https://drafts.csswg.org/cssom/#insert-a-css-rule

@tajo
Copy link
Owner

tajo commented Jan 19, 2024

It's not in the spec but I guess it makes sense adding elements before existing index. https://github.com/tajo/ladle/blob/main/packages/ladle/lib/app/src/synchronize-head.tsx#L17 I think we just need to change after to before

@jkimbo
Copy link
Author

jkimbo commented Jan 19, 2024 •
edited
Loading

I don't think changing after to before is sufficient. I just tried it in our internal Ladle instance and I still get the same issue. I think an additional problem is (since styled-components can insert rules out of order) if the index doesn't currently exist the rule just gets added to the head which means it's loses it's original index position: https://github.com/tajo/ladle/blob/main/packages/ladle/lib/app/src/synchronize-head.tsx#L19-L22

For example here are the rules on our Ladle instance:
Screenshot 2024-01-19 at 10 05 13

The key bit is that rule .cFjwwI is inserted at index 5 but because it's the first rule it effectively gets inserted at index 0. Other rules then get appended (which aren't important for this example) and then rule .driNVn gets inserted at index 1. This then overrides rule .cFjwwI which should be after .driNVn but stays at index 0.

I think to properly account for this we need to maintain a map of index to rule and regenerate all the stylesheets in the correct order after each insertion. What do you think?

@tajo
Copy link
Owner

tajo commented Jan 19, 2024

Are we sure that there is just one instance of styled-components / CSS-in-JS library doing these insertions? It's really strange it attempts to insertRule at index 5 as its first operation. Not sure why would they do that. According to spec, that should just cause an error:

To insert a CSS rule rule in a CSS rule list list at index index, follow these steps:

  1. Set length to the number of items in list.
  2. If index is greater than length, then throw an IndexSizeError exception.

Anyway, if that's the case we could store the index value in the existing data-debug-css attribute and addStyleElement would prioritize that over the order of style tags. A bit mess. Something doesn't make sense right now.

@jkimbo
Copy link
Author

jkimbo commented Jan 24, 2024

@tajo there aren't multiple instances of styled-components on the page, it's because our .ladle/components.tsx file contains mui components and it seems they get rendered before the SynchronizeHead gets mounted. I've updated the demo to show that: https://stackblitz.com/edit/ladle-hzn2yy?file=.ladle%2Fcomponents.tsx

@jkimbo
Copy link
Author

jkimbo commented Jun 3, 2024

@tajo any further thoughts on this issue? IMO having an option to disable the SynchronizeHead behaviour would be good enough for us because we don't use iframes.

@tajo
Copy link
Owner

tajo commented Jun 11, 2024

@tajo any further thoughts on this issue? IMO having an option to disable the SynchronizeHead behaviour would be good enough for us because we don't use iframes.

Yea. Afaik, it should be disabled?

If not, something to fix.

@jkimbo
Copy link
Author

jkimbo commented Jun 11, 2024 •
edited
Loading

In my testing it didn't disable it because the CSSStyleSheet insertRule and removeRule functions still get patched even if there isn't a storyWindow:

ladle/packages/ladle/lib/app/src/synchronize-head.tsx

Lines 76 to 99 in b4d67c1

React.useEffect(() => {
const originalInsertRule = window.CSSStyleSheet.prototype.insertRule;
const originalDeleteRule = window.CSSStyleSheet.prototype.deleteRule;
window.CSSStyleSheet.prototype.insertRule = function (rule, index) {
const retVal = addStyleElement(rule, document, index);
if (active && iframeDocument) {
return addStyleElement(rule, iframeDocument, index);
}
return retVal;
};
window.CSSStyleSheet.prototype.deleteRule = function (index) {
deleteStyleElement(document, index);
if (active && iframeDocument) {
deleteStyleElement(iframeDocument, index);
}
};
return () => {
window.CSSStyleSheet.prototype.insertRule = originalInsertRule;
window.CSSStyleSheet.prototype.deleteRule = originalDeleteRule;
};
}, []);

Will raise a PR to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jkimbo @tajo