Repository navigation
Allow escaped characters in the class name - #1658
samuil-banti-wpenigne wants to merge 6 commits into
Conversation
|
coverage: 75.45% (+0.02%) from 75.43% — samuil-banti-wpenigne:enable-escaped-symbols-in-the-class-names into MyIntervals:main |
There was a problem hiding this comment.
🟡 Changes recommended
The escape helper repeatedly copies a growing array, introducing quadratic overhead for long compound selectors.
1 open issue
What changed in this PR
This PR extends selector parsing to support escaped structural characters in class names, including Tailwind CSS v4 arbitrary variants.
Changes:
- Apply escape detection before interpreting brackets, parentheses, braces, and selector separators.
- Add regression tests for escaped characters, backslash parity, and selector boundaries.
- Document the parsing improvement in the changelog.
| File | Description |
|---|---|
| tests/Unit/Property/SelectorTest.php | Test escaped selectors and component boundaries. |
| tests/Unit/Property/Selector/CompoundSelectorTest.php | Test escaped structural characters and stop-character handling. |
| src/Property/Selector/CompoundSelector.php | Extend escape detection across structural characters. |
| CHANGELOG.md | Record support for escaped selector characters. |
Review effort: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
JakeQZ
left a comment
JakeQZ
left a comment
There was a problem hiding this comment.
Thanks @samuil-banti-wpenigne, this looks great, with excellent tests.
I've just spent several hours trying to decipher the CSS spec, which is as clear as mud (but more interesting than trying to patch up web servers ;)). Think I've got to the bottom of it. We're not handling the \r and \n cases correctly, as they cannot be escaped, but I think those should be left alone for now to be addressed in a future PR. Most other characters can be escaped and this change seems to address that very well.
I've made a couple of other minor suggested improvements.
| case ',': | ||
| // The fallthrough is intentional. | ||
| case ' ': | ||
| // The fallthrough is intentional. | ||
| case "\t": | ||
| // The fallthrough is intentional. | ||
| case "\n": | ||
| // The fallthrough is intentional. | ||
| case "\r": | ||
| // The fallthrough is intentional. |
There was a problem hiding this comment.
I'm not sure the handling of the whitespace cases was originally correct.
According to https://drafts.csswg.org/selectors/#case-sensitive which in turn references https://drafts.csswg.org/css2/#characters:
Any character (except a hexadecimal digit, linefeed, carriage return, or form feed) can be escaped with a backslash to remove its special meaning.
Outside a string, a backslash followed by a newline stands for itself (i.e., a DELIM followed by a newline).
There's no mention of spaces and tabs that I can find. I note one of your tests includes an escaped space, so I assume that's valid.
I think we should keep the handling of CR and LF as it is for now, and address that as a separate PR.
The handling of the comma (,) can be moved along with >, +, ~, as that seems obviously correct, along with space and tab which is presumably correct.
| $nextCharacter = $parserState->peek(); | ||
| $nextCharacterIsEscaped = self::isCharacterEscaped($selectorParts); |
There was a problem hiding this comment.
I think we can just pass \end($selectorParts) to the helper method, so it can just focus on a single string.
Also, I think it's more logical to have these two assignations in the other order, as $nextCharacterIsEscaped is looking at the last character(s) parsed, and $nextCharacter is looking at what is to come.
| private static function isCharacterEscaped(array $selectorParts): bool | ||
| { | ||
| $lastPart = $selectorParts[\count($selectorParts) - 1] ?? null; | ||
| if (!\is_string($lastPart)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Neat refactoring. But I think this could just be passed a string which is the last chunk parsed - see other comment.
Some may belong in the |
Tailwind CSS v4 may contain square brackets in the class names i.e.
Other valid characters are also affected by the parser at this point:
So the goal of the PR is to enable parsing of such selectors.