Ensure existing spaces in attribute selectors are valid #14703

RobinMalfait · 2024-10-17T12:59:13Z

This PR fixes an issue where spaces in a selector generated invalid CSS.

Lightning CSS will throw those incorrect lines of CSS out, but if you are in an environment where Lightning CSS doesn't run then invalid CSS is generated.

Given this input:

data-[foo_=_"true"]:flex

This will be generated:

.data-\[foo_\=_\"true\"\]\:flex[data-foo=""true] {
  display: flex;
}

With this PR in place, the generated CSS will now be:

.data-\[foo_\=_\"true\"\]\:flex[data-foo="true"] {
  display: flex;
}

RobinMalfait · 2024-10-17T12:59:28Z

Run tests without optimization #14704
Ensure existing spaces in attribute selectors are valid #14703 👈
next

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @RobinMalfait and the rest of your teammates on Graphite

RobinMalfait · 2024-10-17T13:01:09Z

packages/tailwindcss/src/variants.ts

+      let value = match.slice(1).trim()
+
      // If the value is already quoted, skip.
-      if (match[1] === "'" || match[1] === '"') {
+      if (value[0] === "'" || value[0] === '"') {


_ values in the value are already converted to which means that we can just trim the valid (if we ignore the first = character)

adamwathan · 2024-10-17T13:11:43Z

Do we actually want people to be able to write stuff like data-[foo_=_"true"]:flex in the first place? You can't put spaces there in CSS so I don't know that it's something we need to support in Tailwind either. I think I'd rather just toss the candidate personally but maybe that's more work?

RobinMalfait · 2024-10-17T13:20:23Z

@adamwathan I noticed it while working on the codemods and created an internal issue so that I didn't forget about it. Fun thing is, that you can put spaces in that position in CSS (see https://jsfiddle.net/ezu8qjdk/) and since arbitrary values should be valid CSS (as an escape hatch if the value isn't used), I figured that it is best to at least generate the correct CSS if you put it in that way.

We can of course throw it out (or even fix it at a codemod level), but since it's valid CSS it might be good to keep it? I'm happy with either solution, just want to fix the scenario that exists today where the currently generated CSS is invalid.

RobinMalfait · 2024-10-17T13:23:03Z

The part I'm missing is that you can't put spaces if you use [data-foo ^ = "bar"] (but [data-foo ^= "bar"] is valid). Will add an explicit test for this case.

packages/tailwindcss/src/variants.test.ts

adamwathan · 2024-10-17T20:26:14Z

Fun thing is, that you can put spaces in that position in CSS

Well then…

adamwathan · 2024-10-17T20:29:25Z

packages/tailwindcss/src/variants.ts

 function quoteAttributeValue(value: string) {
  if (value.includes('=')) {
+    // Do not allow invalid operators in attribute values
+    if (/[*$^|~]\s+=/.test(value)) {


Should we try to always precompile regular expressions?

That's something we could do, it's also a non-global one (/g) so it's stateless.

This only ever runs in data-* or aria-* variants when using arbitrary values that contain an = so I'm assuming this will be used very rarely. Especially because the aria-foobar already translates to aria-[foobar="true"], but this doesn't go via this function since it's a bare value instead of an arbitrary value.

adamwathan · 2024-10-17T20:49:45Z

packages/tailwindcss/src/variants.ts

 function quoteAttributeValue(value: string) {
  if (value.includes('=')) {
+    // Do not allow invalid operators in attribute values
+    if (/[*$^|~]\s+=/.test(value)) {


I think this condition would break this variant:

data-[potato="^_="]

Feeling like the better overall solution to this problem should rely on segment — do we just need to segment the value on = and quote the last chunk if it isn't already quoted? Maybe we don't need to protect people from writing data-[potato_^_=_foo] and just let that be user error that generates junk.

Reworked it by using segment instead of the regex.

philipp-spiess

Looks good now and feels like a simple fix. Just one idea regarding the change log entry.

CHANGELOG.md

packages/tailwindcss/src/variants.ts

CHANGELOG.md

packages/tailwindcss/src/variants.ts

If you inject existing spaces in the attribute selector, then invalid CSS was generated. Lightning CSS itself throws it out, but if you are in an environment where Lightning CSS doesn't run then invalid CSS exists in your final CSS file. This fixes that by ensuring that the spaces around the `=` don't cause any issues.

This test will verify that the usage of `segment` is correct. Before we were relying on a regex and check for `(=.*)` which would also match _inside_ of the quoted value.

Co-authored-by: Adam Wathan <[email protected]>

RobinMalfait force-pushed the robin/ensure_existing_spaces_in_attribute_selector_are_valid branch 2 times, most recently from d5f1b79 to d239cc5 Compare October 17, 2024 13:00

RobinMalfait commented Oct 17, 2024

View reviewed changes

RobinMalfait requested review from philipp-spiess and thecrypticace October 17, 2024 13:02

RobinMalfait force-pushed the robin/ensure_existing_spaces_in_attribute_selector_are_valid branch from d239cc5 to 9ff2d99 Compare October 17, 2024 13:49

RobinMalfait requested a review from a team as a code owner October 17, 2024 13:49

RobinMalfait mentioned this pull request Oct 17, 2024

Run tests without optimization #14704

Closed

RobinMalfait force-pushed the robin/ensure_existing_spaces_in_attribute_selector_are_valid branch from 9ff2d99 to 5e26aec Compare October 17, 2024 14:11

RobinMalfait changed the title ~~Ensure existing spaces in attribute selector are valid~~ Ensure existing spaces in attribute selectors are valid Oct 17, 2024

thecrypticace approved these changes Oct 17, 2024

View reviewed changes

philipp-spiess reviewed Oct 17, 2024

View reviewed changes

packages/tailwindcss/src/variants.test.ts Show resolved Hide resolved

adamwathan reviewed Oct 17, 2024

View reviewed changes

adamwathan requested changes Oct 17, 2024

View reviewed changes

RobinMalfait force-pushed the robin/ensure_existing_spaces_in_attribute_selector_are_valid branch from 71c24fe to 5120b3b Compare October 18, 2024 11:03

RobinMalfait requested review from adamwathan and philipp-spiess October 18, 2024 11:09

RobinMalfait force-pushed the robin/ensure_existing_spaces_in_attribute_selector_are_valid branch from 6aa4ed3 to 7b0465b Compare October 18, 2024 13:37

philipp-spiess approved these changes Oct 18, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

adamwathan reviewed Oct 18, 2024

View reviewed changes

packages/tailwindcss/src/variants.ts Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

packages/tailwindcss/src/variants.ts Show resolved Hide resolved

RobinMalfait and others added 5 commits October 18, 2024 16:06

update changelog

f89ea21

rely on segment when quoting attributes

3ed8d96

do not rely on regex

9d5ae22

This test will verify that the usage of `segment` is correct. Before we were relying on a regex and check for `(=.*)` which would also match _inside_ of the quoted value.

Update packages/tailwindcss/src/variants.ts

fc10222

Co-authored-by: Adam Wathan <[email protected]>

RobinMalfait and others added 2 commits October 18, 2024 16:07

Update CHANGELOG.md

9aec43a

Co-authored-by: Adam Wathan <[email protected]>

always trim the value

8d48594

RobinMalfait force-pushed the robin/ensure_existing_spaces_in_attribute_selector_are_valid branch from 5e4504d to 8d48594 Compare October 18, 2024 14:10

adamwathan approved these changes Oct 18, 2024

View reviewed changes

RobinMalfait merged commit b7c4d25 into next Oct 18, 2024
2 checks passed

RobinMalfait deleted the robin/ensure_existing_spaces_in_attribute_selector_are_valid branch October 18, 2024 17:51

Uh oh!

Ensure existing spaces in attribute selectors are valid #14703

Ensure existing spaces in attribute selectors are valid #14703

Uh oh!

Conversation

RobinMalfait commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RobinMalfait commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RobinMalfait Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

adamwathan commented Oct 17, 2024

Uh oh!

RobinMalfait commented Oct 17, 2024

Uh oh!

RobinMalfait commented Oct 17, 2024

Uh oh!

Uh oh!

adamwathan commented Oct 17, 2024

Uh oh!

adamwathan Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

RobinMalfait Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

adamwathan Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

RobinMalfait Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

philipp-spiess left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RobinMalfait commented Oct 17, 2024 •

edited

Loading

RobinMalfait commented Oct 17, 2024 •

edited

Loading