feat: add cssPropertyRename option to prevent CSS @property conflicts #2005
Conversation
❌ Deploy Preview for creative-fairy-df92c4 failed.
|
aklinker1
left a comment
aklinker1
left a comment
There was a problem hiding this comment.
Code looks good, thanks for writing tests.
We had really bad performance with the splitShadowRootCss function that some else found and improved. Since that function and this new one both are apart of the same function, createShadowRootUi, and slow performance here is known to cause a bad UX, can you benchmark the createShadowRootUi function using real CSS from tailwind?
Compare how many times per second createShadowRootUi can run with or without cssPropertyRename. If it's slow, try and optimize it. If it's fast and the hit is negligable, no need to make any improvements.
You can use Vitest's bench utils to write the benchmark, please include it in the PR, don't just delete it after writing it.
| const css = `.class { --tw-prop: value; }`; | ||
| const expected = `.class { prop: value; }`; |
There was a problem hiding this comment.
This is... probably fine? Trying to decide if throwing an error would be better than producing this CSS. Probably not? This isn't valid CSS but having weird styles is better than not showing your UI at all? Hmm, not sure. Did you consider throwing an error in this case?
There was a problem hiding this comment.
What do you think would be a better way to handle the situation when toPrefix or fromPrefix is empty? Maybe we could set both toPrefix and fromPrefix as required fields? If either of them is not passed in, no processing would be done?
There was a problem hiding this comment.
So I think there are three options:
- Throw an error and make devs look in the console to figure out why their content script UI isn't showing up
- Use what you have now and devs will see the UI, but things like borders and rings that depend on custom properties will be styled wrong and they have to debug why it's not showing up correctly.
- Don't change the CSS at all if there are invalid options and devs will never know their styles mess up tailwind V3 sites unless they are developing on one directly.
Now that I've written all that out... I think option 1 where we throw an error would be the best DX. It's clear something is broken because their UI wouldn't show up and we can clearly state the problem and solution in the error message without the dev having to go through the CSS and debug which properties are the cause of the bad styles.
There was a problem hiding this comment.
Okay, I also think option 1 is excellent.
| shadowHost: HTMLElement, | ||
| ) => TMounted; | ||
| /** | ||
| * Rename CSS custom property prefixes to prevent conflicts with host page styles. |
There was a problem hiding this comment.
| * Rename CSS custom property prefixes to prevent conflicts with host page styles. | |
| * Rename CSS custom property and var prefixes to prevent conflicts with host page styles. |
2 participants
Overview
Add
cssPropertyRenameoption tocreateShadowRootUithat allows renaming CSS custom property prefixes to preventconflicts with host page styles.
Problem:
When extensions use Tailwind CSS v4 with Shadow Root UI, the
@propertyrules (e.g.,@property --tw-gradient-from { syntax: "<color>"; }) are extracted and injected into the host page's<head>. This causes conflicts with host pagesusing Tailwind CSS v3, which use composite CSS variable values that don't match the typed syntax constraint.
Solution:
renameCssCustomProperties()utility function that renames:@propertyrules:@property --tw-xxx→@property --wxt-tw-xxx--tw-xxx: value→--wxt-tw-xxx: valuevar()references:var(--tw-xxx)→var(--wxt-tw-xxx)cssPropertyRenameoption toShadowRootContentScriptUiOptionsfromPrefixandtoPrefixare optional:toPrefixis provided: prepends prefix to all custom propertiesfromPrefixis provided: removes the matching prefixfromPrefixwithtoPrefixManual Testing
Related Issue
This PR closes #1955