Strip a leading UTF-8 BOM when parsing a style sheet - #1301
Open
X-Coder264 wants to merge 1 commit into
Open
Conversation
U+FEFF is a valid ident code point, so a leading BOM was tokenised as part
of the CSS: `BOM html {}` silently parsed as a `\u{feff}html` type selector
that matches nothing, and `BOM /*! license */ html {}` failed to parse with
`Unexpected token Ident("html")`. CSS Syntax removes the BOM while decoding
the input byte stream, before tokenisation, so strip it in `parse_with`,
which every consumer and every bundled file goes through.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #338.
A leading UTF-8 BOM was tokenised as part of the CSS instead of being removed while decoding the input. U+FEFF is a valid ident code point, so it gets glued onto whatever follows, which produces two different failures:
+html{color:red}html{color:red}— parses, but the selector is nowhtmland matches nothinghtml{color:red}+/*! license */html{color:red}Unexpected token Ident("html")at 1:2/*! license */\nhtml{color:red}+@charset "UTF-8";html{color:red}Unexpected token AtKeyword("charset")at 1:2html{color:red}The first row is the one I found most alarming: no error, no warning, the rule is just silently dropped from the page. When a comment or at-rule follows the BOM, the same lexing yields two idents separated by non-whitespace, which is the parse error people actually report — and it is hard to diagnose, because the location is
1:2of a generated bundle and the identifier in the message is the first selector of the first stylesheet, often dozens of KB away from the cause.CSS Syntax §3.2 removes the BOM when decoding the input byte stream, before tokenisation, so this strips it in
StyleSheet::parse_with— the single choke point shared by the Rust API, the Node/WASM/C bindings and the bundler. Because the bundler parses every file through it, imported files are covered too, not just the entry.Why this is worth fixing now
The BOM is no longer only an artefact of hand-authored files saved in an editor: it is showing up in generated CSS. Dart Sass prepends a BOM (instead of
@charset "UTF-8") when compressed output contains non-ASCII characters, e.g. any project importingbootstrap-sassglyphicons. Until postcss 8.5.23 a postcss pass silently dropped it; postcss 8.5.24 changed to "Preserve the BOM after the processing", so it now reaches the minifier and breaks webpack/Parcel production builds that use lightningcss. That is how I ran into it.Decisions I would like your call on
@charsetrules; a consumer that needs one can prepend it.set_source_content, sosourcesContentlines up with the positions the parser reports. The same one-line change would be needed innapi/src/lib.rs,src/main.rsandc/src/lib.rsfor their source maps to be exact for BOM'd input (as it stands, line 1 columns would be one character to the left). I left those out to keep the diff focused, since it would mean either duplicating the helper or makingstrip_bompublic — happy to do whichever you prefer.StyleAttribute::parseis unchanged. An inlinestyleattribute is not a decoded byte stream, so a U+FEFF there is an ordinary character rather than an encoding marker.strip_prefixper style sheet at parse entry, nothing per token, so the benchmark concern raised in UTF-8 BOM Handling #338 shouldn't apply.Tests
tests::test_bominsrc/lib.rs— leading BOM followed by a rule, a/*!license comment,@charsetand@import, plus a non-leading BOM which must survive as an ordinary character.bundler::tests::test_bom— BOM on both the entry and an imported file.cargo test --all-featurespasses withRUSTFLAGS=-D warnings. My added code is rustfmt-clean; note that runningcargo fmton currentmasteralso reformats some pre-existing code insrc/selector.rs,src/bundler.rsandsrc/lib.rs, which I reverted so this diff stays focused (it is purely additive).I could not run
yarn test— no npm registry access in my environment — so there is no JS-level test here. Happy to add one tonode/test/transform.test.mjsif you'd like it covered there too.