Skip to content

Fix bug replacing modifier variable shorthand syntax underscores #17889

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

brandonmcconnell
Copy link
Contributor

@brandonmcconnell brandonmcconnell commented May 6, 2025

Resolves #17888

Reproduction URL: https://play.tailwindcss.com/YvIekuzVRd

Changes:

  • Don't use decodeArbitraryValue when parsing variable shorthand syntax in modifiers
    • replace decodeArbitraryValue(modifier.slice(1, -1)) with modifier.slice(1, -1)
    • added test case, passing ✅

Edit by maintainer:

This PR fixes an issue where the arbitrary modifier shorthand (e.g.: bg-red-500/(--my_variable)) was incorrectly replacing the _ with a (space).
It's important that we still decode the arbitrary value and replace _ with in other situations.

For example, if you have this:

<div class="font-(family-name:--my_font,comic_sans_ms)"></div>

You would expect the following CSS:

.font-\(family-name\:--foo\,comic_sans_ms\) {
  font-family: var(--my_font,comic sans ms);
}

Where the _ in the variable is not replaced, but the _ in the fallback value is being replaced.

@brandonmcconnell brandonmcconnell requested a review from a team as a code owner May 6, 2025 07:40
@RobinMalfait
Copy link
Member

Hey!

We used to have some functionality like this, but we reverted it in #16206 (/cc @philipp-spiess) and we have some dedicated tests where we ensure that if you write --foo\\_bar that it correctly parses as-if you wrote --foo_bar:
https://github.com/tailwindlabs/tailwindcss/blob/5c5ae04db6d228a5882ae211e07cbb8c1bafc501/packages/tailwindcss/src/candidate.test.ts#L938-L108

In the event that we want to re-introduce this (where the variable doesn't require underscores), then we have some more work to do because the fallback value can also contain underscores where we do want to convert them to spaces. E.g.:

<div class="bg-(--my_variable,rgb(0_2_3))"></div>

Which already parses properly as:

.bg-\(--my_variable\,rgb\(0_2_3\)\) {
  background-color: var(--my_variable,rgb(0 2 3));
}

But looking at the reproduction, I do feel like there is a bug with the modifier.

<div class="bg-(--my_variable,rgb(0_2_3))/(--my_opacity,some_fallback_value)"></div>

Compiles to:

.bg-\(--my_variable\,rgb\(0_2_3\)\)\/\(--my_opacity\,some_fallback_value\) {
  background-color: color-mix(in oklab, var(--my_variable,rgb(0 2 3)) var(--my opacity,some fallback value), transparent);
}

So the spaces int he fallback should be kept, but not in the actual variable (first argument).

@RobinMalfait RobinMalfait self-assigned this May 6, 2025
These tests include CSS variables with underscores that should not be
replaced, but also fallback values that should be replaced.
This now has the same behavior as arbitrary value shorthand where
`(--my-value)` is wrapped in a `var(--my-value)` behind the scenes.

Doing this, allows us to perform 2 things:

1. We can check early if the value is an actual CSS variable — aka, an
   arbitrary modifier shorthand starts and ends with `(` and `)`
   respectively and starts with `--` inside the parens. E.g.: `(--…)`
2. Wrapping in `var(…)` allows us to use the `decodeArbitraryValue`
   which handles CSS variables (and fallbacks) already for us.
Essentially if we had `value[0] !== '-' && value[1] !== '-'`, a value
such as `_-` would pass. E.g.: `var(_--foo)`

Instead, we have to ensure that both the first and second character are
`-` characters.

Alternatively, we could write `!(value[0] === '-' && value[1] === '-')`,
but applying De Morgan's law means:

`!(value[0] === '-' && value[1] === '-')`

Becomes:

`value[0] !== '-' || value[1] !== '-'`
@RobinMalfait RobinMalfait enabled auto-merge (squash) May 6, 2025 13:32
@RobinMalfait RobinMalfait merged commit 4f8539c into tailwindlabs:main May 6, 2025
7 checks passed
@brandonmcconnell
Copy link
Contributor Author

I appreciate the quick turnaround on this, @RobinMalfait and @thecrypticace. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid variable shorthand modifier replacement in modifiers
3 participants