[babel-plugin] quote content values that contain quote characters - #1828
Conversation
transformValue passes a `content` value through untouched when a quote character occurs at least twice anywhere in it. The intent was to leave values the author already wrote as CSS alone, but that test also matches ordinary text. `content: "Bob's and Jim's"` and `content: 'He said "hello"'` both satisfy it, so they compile to `content:Bob's and Jim's` and `content:He said "hello"`. Neither is a CSS string, so the browser drops the whole declaration and nothing renders. There is no warning at build time. Replace the character count with a check on the parsed value. A value is now passed through only when it parses as a list of content components, meaning every part is a closed string, a function or one of the quote keywords, and at least one part is a string. Anything else is plain text and gets wrapped in a single quoted string. Wrapping escapes only what would end that string early: a double quote that is not already escaped, a trailing backslash that would otherwise escape the closing quote, and a line break, which a CSS string writes as `\A`. A backslash that opens an escape sequence is left alone, so `content: '\2014'` still compiles to `content:"\2014"` and still renders an em dash. Values such as `open-quote "hello" close-quote` and `"a" "b"` still pass through. Accepting a value requires a closed string, so the new check only accepts values the old check already accepted, and every value that used to compile to a well formed CSS declaration compiles to exactly the same bytes as before. What changes is the set of values that used to compile to a declaration the browser could not parse.
|
@Om-singhaI is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect handling of CSS content values in the StyleX Babel plugin when the raw text contains quote characters, ensuring the plugin emits valid CSS strings rather than leaving invalid, unquoted prose that browsers drop.
Changes:
- Replace quote-counting heuristics with
postcss-value-parser-based detection of already-valid CSScontentlists (strings/functions/quote keywords). - Escape only the characters that would terminate a double-quoted CSS string early (unescaped
"/ trailing\/ line breaks →\A) when wrapping plain text. - Add unit tests and integration snapshot tests covering quoted prose, escape sequences, trailing backslashes, and mixed keyword/string content lists.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/@stylexjs/babel-plugin/src/shared/utils/transform-value.js | Uses postcss-value-parser to distinguish valid CSS content lists from plain text, and safely wraps/escapes plain strings. |
| packages/@stylexjs/babel-plugin/src/shared/utils/tests/transform-value-test.js | Adds focused unit tests for quote-containing prose, escape-sequence preservation, and keyword+string combinations. |
| packages/@stylexjs/babel-plugin/tests/transform-value-normalization-test.js | Adds inline snapshot coverage so emitted CSS for tricky content cases is visible and regression-tested. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
This has been approved since the 31st. The three red checks are the fork ones: Vercel wants deploy authorization, and perf and size both 403 at their Post comment step. #1826 merged with the same three red. I merged main into it locally to check nothing had gone stale in the meantime. No conflicts, and the transform value tests still pass. Anything you need from me? |
What changed / motivation ?
There is no issue open for this, so here is the reproduction. On
main:compiles, with no warning, to
content:Bob's and Jim'sandcontent:He said "hello". Neither is a CSS string. I served both in Chrome and readgetComputedStyle(el, '::before').content: both come back asnone, so the browser threw the declaration away and nothing renders.The cause is in
transformValue, which decides whether acontentvalue is already CSS by counting quote characters:The intent is to leave a value alone when the author already quoted it, but what it asks is whether a quote character appears twice anywhere, and ordinary prose satisfies that.
The fix
The value is parsed with the
postcss-value-parserthatnormalize-value.jsnext door already imports. It passes through only when every part is a closed string, a closed function or a quote keyword, and at least one part is a string. Anything else is wrapped in one quoted string, escaping only what would end that string early: an unescaped double quote, a trailing backslash, and a line break as\A.A backslash that opens an escape sequence is left alone. Inside a CSS string a backslash means an escape, so
content: '\2014'is the author asking for an em dash and still compiles tocontent:"\2014". Escaping every backslash would emitcontent:"\\2014"and render the literal characters instead.Behavior change
I ran each case through Chrome, one declaration per stylesheet, reading the computed
contentbefore and after. Fourteen inputs emit byte identical CSS, including\2014,\201C hello \201D,C:\\Users,back\slash,open-quote "hello" close-quote,"icon" / "Alt text",attr(),counter()and any value already written as a string.Six change, and all six were broken before:
Bob's and Jim's,He said "hello",say "hi, and quotes the author escaped) computednoneand now renders the sentence.main, so Chrome swallowed the rule's closing brace and painted50% off "}. It now renders50% off \.noneand is now written with\A.Two inputs that are invalid CSS either way also change what the page shows, and I would rather state them than bury them:
"a" contentsuses a content list keyword that is in the spec but not in this file's keyword set, and"a" 5pxis not a valid content value. Both computednoneand painted nothing; each now paints its text literally. Neither ever produced what its author wanted, but if you would rathercontentskeep passing through I am happy to add it to the keyword set.Nothing that renders correctly on
mainrenders differently. Accepting a value requires at least one closed string, so the check can only accept what it accepted before, and for a value that was already quoted the new escaping is the identity unless it holds an unescaped quote, a trailing backslash or a line break.Linked PR/Issues
No open issue. The quote counting arrived in #782, whose description says it "Only adds quotes for plain text strings";
Bob's and Jim'sis plain text, so this closes that gap rather than changing the intent.Additional Context
Tests cover apostrophes, an embedded quoted word, the keyword and string combinations that must keep passing through, and a case per escape rule, plus two snapshot cases beside the existing
contenttest so the emitted CSS is visible.Reverting only
transform-value.jsand keeping the tests fails four of them, reportingExpected: "\"Bob's and Jim's\"" / Received: "Bob's and Jim's"among them; restoring it passes all 26.test:packagesis 1993 passed and 77 skipped across 87 suites with 1173 snapshots,flowreports no errors,prettier:reportis clean, andeslintis clean on the three changed files.Pre-flight checklist
Contribution Guidelines