Skip to content

Don't escape underscores for the first parameter of var() #14776

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

Merged
adamwathan merged 2 commits into from
Oct 24, 2024
Merged

Don't escape underscores for the first parameter of var() #14776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1abedd4
Don't escape underscores for the first parameter of `var()`
philipp-spiess Oct 24, 2024
75aa966
Apply suggestions from code review
philipp-spiess Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed

- Don't convert underscores in the first argument to `var()` to spaces ([#14776](https://github.com/tailwindlabs/tailwindcss/pull/14776))

### Fixed

- Ensure individual logical property utilities are sorted later than left/right pair utilities ([#14777](https://github.com/tailwindlabs/tailwindcss/pull/14777))
- Don't migrate important modifiers inside conditional statements in Vue and Alpine (e.g. `<div v-if="!border" />`) ([#14774](https://github.com/tailwindlabs/tailwindcss/pull/14774))
- Ensure third-party plugins with `exports` in their `package.json` are resolved correctly ([#14775](https://github.com/tailwindlabs/tailwindcss/pull/14775))
- Ensure underscores in the `url()` function are never unescaped ([#14776](https://github.com/tailwindlabs/tailwindcss/pull/14776))
- _Upgrade (experimental)_: Ensure `@import` statements for relative CSS files are actually migrated to use relative path syntax ([#14769](https://github.com/tailwindlabs/tailwindcss/pull/14769))

## [4.0.0-alpha.29] - 2024-10-23
Expand Down
60 changes: 43 additions & 17 deletions packages/tailwindcss/src/candidate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1059,24 +1059,50 @@ it('should not replace `_` inside of `url()`', () => {
let utilities = new Utilities()
utilities.functional('bg', () => [])

expect(run('bg-[url(https://example.com/some_page)]', { utilities })).toMatchInlineSnapshot(`
[
{
"important": false,
"kind": "functional",
"modifier": null,
"negative": false,
"raw": "bg-[url(https://example.com/some_page)]",
"root": "bg",
"value": {
"dataType": null,
"kind": "arbitrary",
"value": "url(https://example.com/some_page)",
expect(run('bg-[no-repeat_url(https://example.com/some_page)]', { utilities }))
.toMatchInlineSnapshot(`
[
{
"important": false,
"kind": "functional",
"modifier": null,
"negative": false,
"raw": "bg-[no-repeat_url(https://example.com/some_page)]",
"root": "bg",
"value": {
"dataType": null,
"kind": "arbitrary",
"value": "no-repeat url(https://example.com/some_page)",
},
"variants": [],
},
"variants": [],
},
]
`)
]
`)
})

it('should not replace `_` in the first argument to `var()`', () => {
let utilities = new Utilities()
utilities.functional('ml', () => [])

expect(run('ml-[var(--spacing-1_5,_var(--spacing-2_5,_1rem))]', { utilities }))
.toMatchInlineSnapshot(`
[
{
"important": false,
"kind": "functional",
"modifier": null,
"negative": false,
"raw": "ml-[var(--spacing-1_5,_var(--spacing-2_5,_1rem))]",
"root": "ml",
"value": {
"dataType": null,
"kind": "arbitrary",
"value": "var(--spacing-1_5, var(--spacing-2_5, 1rem))",
},
"variants": [],
},
]
`)
})

it('should parse arbitrary properties', () => {
Expand Down
28 changes: 28 additions & 0 deletions packages/tailwindcss/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,34 @@ describe('compiling CSS', () => {
),
).toMatchSnapshot()
})

test('unescapes underscores to spaces inside arbitrary values except for `url()` and first argument of `var()`', async () => {
expect(
await compileCss(
css`
@theme {
--spacing-1_5: 1.5rem;
--spacing-2_5: 2.5rem;
}
@tailwind utilities;
`,
['bg-[no-repeat_url(./my_file.jpg)', 'ml-[var(--spacing-1_5,_var(--spacing-2_5,_1rem))]'],
),
).toMatchInlineSnapshot(`
":root {
--spacing-1_5: 1.5rem;
--spacing-2_5: 2.5rem;
}

.ml-\\[var\\(--spacing-1_5\\,_var\\(--spacing-2_5\\,_1rem\\)\\)\\] {
margin-left: var(--spacing-1_5, var(--spacing-2_5, 1rem));
}

.bg-\\[no-repeat_url\\(\\.\\/my_file\\.jpg\\) {
background-color: no-repeat url("./")my file. jpg;
}"
`)
})
})

describe('arbitrary properties', () => {
Expand Down
15 changes: 13 additions & 2 deletions packages/tailwindcss/src/utils/decode-arbitrary-value.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ describe('decoding arbitrary values', () => {

it('should not replace underscores in url()', () => {
expect(decodeArbitraryValue('url(./my_file.jpg)')).toBe('url(./my_file.jpg)')
expect(decodeArbitraryValue('no-repeat_url(./my_file.jpg)')).toBe(
'no-repeat url(./my_file.jpg)',
)
})

it('should not replace underscores in the first argument of var()', () => {
expect(decodeArbitraryValue('var(--spacing-1_5)')).toBe('var(--spacing-1_5)')
expect(decodeArbitraryValue('var(--spacing-1_5,_1rem)')).toBe('var(--spacing-1_5, 1rem)')
expect(decodeArbitraryValue('var(--spacing-1_5,_var(--spacing-2_5,_1rem))')).toBe(
'var(--spacing-1_5, var(--spacing-2_5, 1rem))',
)
})

it('should leave var(…) as is', () => {
Expand Down Expand Up @@ -55,8 +66,8 @@ describe('adds spaces around math operators', () => {
['calc(24px+(-1rem))', 'calc(24px + (-1rem))'],
['calc(24px_+_(-1rem))', 'calc(24px + (-1rem))'],
[
'calc(var(--10-10px,calc(-20px-(-30px--40px)-50px)',
'calc(var(--10-10px,calc(-20px - (-30px - -40px) - 50px)',
'calc(var(--10-10px,calc(-20px-(-30px--40px)-50px)))',
'calc(var(--10-10px,calc(-20px - (-30px - -40px) - 50px)))',
Comment on lines +69 to +70
Copy link
Member Author

@philipp-spiess philipp-spiess Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case didn't have the proper number of closing quotes. I remember that @RobinMalfait once mentioned that we do validate this already at the parser level.

Copy link
Member

@RobinMalfait RobinMalfait Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only validate the parens in your CSS file (and throw), but we don't validate if it happens in a candidate.

Input:

<div class="[--foo:calc(var(--10-10px,calc(-20px-(-30px--40px)-50px)]"></div>

Output:

.\[--foo\:calc\(var\(--10-10px\,calc\(-20px-\(-30px--40px\)-50px\)\] {
  --foo: calc(var(--10-10px,calc(-20px - (-30px - -40px) - 50px);
}

Copy link
Member Author

@philipp-spiess philipp-spiess Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, lol!

With this change the closing parentheses would be automatically inserted actually (because we can't encode the absence of them in the ValueParser—That's how I found out about this example in the first place! But I think it would be better if we throw away candidates like this 🤔

],
['calc(theme(spacing.1-bar))', 'calc(theme(spacing.1-bar))'],
['theme(spacing.1-bar)', 'theme(spacing.1-bar)'],
Expand Down
55 changes: 50 additions & 5 deletions packages/tailwindcss/src/utils/decode-arbitrary-value.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import * as ValueParser from '../value-parser'
import { addWhitespaceAroundMathOperators } from './math-operators'

export function decodeArbitraryValue(input: string): string {
// We do not want to normalize anything inside of a url() because if we
// replace `_` with ` `, then it will very likely break the url.
if (input.startsWith('url(')) {
return input
// There are definitely no functions in the input, so bail early
if (input.indexOf('(') === -1) {
return convertUnderscoresToWhitespace(input)
}
Comment on lines +6 to 8
Copy link
Member Author

@philipp-spiess philipp-spiess Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was moved up from being inside the addWhitespaceAroundMathOperators, since neither the url() nor the var() handling need to run if the arbitrary value has no parenthesis.


input = convertUnderscoresToWhitespace(input)
let ast = ValueParser.parse(input)
recursivelyDecodeArbitraryValues(ast)
input = ValueParser.toCss(ast)

input = addWhitespaceAroundMathOperators(input)

return input
Expand Down Expand Up @@ -41,3 +44,45 @@ function convertUnderscoresToWhitespace(input: string) {

return output
}

function recursivelyDecodeArbitraryValues(ast: ValueParser.ValueAstNode[]) {
for (let node of ast) {
switch (node.kind) {
case 'function': {
if (node.value === 'url' || node.value.endsWith('_url')) {
// Don't decode underscores in url() but do decode the function name
node.value = convertUnderscoresToWhitespace(node.value)
break
}

if (node.value === 'var' || node.value.endsWith('_var')) {
// Don't decode underscores in the first argument of var() but do
// decode the function name
node.value = convertUnderscoresToWhitespace(node.value)
for (let i = 0; i < node.nodes.length; i++) {
if (i == 0 && node.nodes[i].kind === 'word') {
continue
}
recursivelyDecodeArbitraryValues([node.nodes[i]])
}
break
}

node.value = convertUnderscoresToWhitespace(node.value)
recursivelyDecodeArbitraryValues(node.nodes)
break
}
case 'separator':
case 'word': {
node.value = convertUnderscoresToWhitespace(node.value)
break
}
default:
never()
}
}
}

function never(): never {
throw new Error('This should never happen')
}
5 changes: 0 additions & 5 deletions packages/tailwindcss/src/utils/math-operators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ export function hasMathFn(input: string) {
}

export function addWhitespaceAroundMathOperators(input: string) {
// There's definitely no functions in the input, so bail early
if (input.indexOf('(') === -1) {
return input
}

// Bail early if there are no math functions in the input
if (!MATH_FUNCTIONS.some((fn) => input.includes(fn))) {
return input
Expand Down