fix(build): stop a stray BOM deleting the loader keyframes from the shipped CSS - #490
Merged
Conversation
…hipped CSS Dart Sass prepends an encoding hint whenever its output contains a non-ASCII character - a U+FEFF BOM in the compressed output we ship - and BannerPlugin then prepends the licence banner in front of it. A BOM at offset 0 is stripped by every CSS parser; at offset 167 it is a valid CSS ident code point, so the parser reads it as the start of a selector, swallows the rule that follows and drops both. The casualty was @Keyframes vscomp-animation-spin, the first rule after the banner, while .vscomp-options-loader::before kept referencing it - so the options loader rendered as a motionless arc and 'loading' became indistinguishable from 'hung'. Measured in Chrome: the keyframes rule is present in master's committed CSS and absent from a fresh build. Any first rule is vulnerable; the spinner was only the one there. The trigger is the WCAG 1.4.1 error cue added for the 1.4.0 work (content: '\26A0'), which Sass resolves to a literal warning sign. It is latent on master today because the committed CSS predates that rule, and would have shipped on the next release build. Two changes, both needed: - sassOptions: { charset: false } stops the hint being emitted at all, so no future non-ASCII character can resurrect the BOM; - the cue is emitted as the CSS escape \26A0 rather than the resolved character, keeping the stylesheet ASCII so the glyph does not depend on the consuming page's charset either. Guarded by byte-level assertions over the built file (scripts/ci/__tests__/stylesheet-bytes.test.mjs: no BOM, ASCII-only, keyframes present) plus a browser check that the rule resolves in the CSSOM. The byte checks live in the Node suite deliberately: written as cy.request() they passed against a known-bad build, because the HTTP layer strips the BOM in transit even with encoding: 'binary'. Suite: 414/414 across 27 specs; scripts 75/75 (3 red before the fix); tsc/eslint/stylelint clean.
PR Test Results — ✅ all checks passed
Tested commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prevents release-build CSS corruption where a mid-file UTF‑8 BOM (introduced by Sass and shifted by BannerPlugin) causes the first CSS rule—previously @keyframes vscomp-animation-spin—to be silently discarded, freezing the loader animation.
Changes:
- Disable Sass charset emission in the webpack
sass-loaderpipeline (sassOptions.charset: false) to prevent BOM insertion. - Emit the warning-sign cue as a CSS escape sequence rather than a literal non-ASCII glyph to keep the generated stylesheet ASCII-only.
- Add byte-level Node tests plus a Cypress E2E integrity check to catch future build regressions where the shipped stylesheet fails to parse as intended.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| webpack.config.js | Sets sassOptions.charset: false to stop Sass from emitting encoding hints/BOMs that can break CSS parsing after the banner is prepended. |
| src/sass/partials/virtual-select.scss | Changes the error cue to output a CSS escape to keep output ASCII and avoid reintroducing encoding-related artifacts. |
| scripts/ci/tests/stylesheet-bytes.test.mjs | Adds byte-level assertions ensuring the built CSS has no BOM, stays ASCII-only, and contains the required keyframes/reference. |
| cypress/e2e/build-stylesheet-integrity.cy.ts | Adds browser-level checks that keyframes survive CSS parsing and that the non-colour cue still renders. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…d stop naming one rule Three findings from a review of 237fd03. The fix itself held up; the guard around it did not. 1. The byte guards never saw a fresh build. They read dist/, but npm run test:scripts runs in the 'static' CI job which does not build, and the committed dist/ is deliberately stale - it contains neither vscomp-error-message nor 26A0. So they passed against a bundle predating the rule they exist to protect, and a PR deleting charset: false without rebuilding would have stayed green. Moved to scripts/build-checks/__tests__/ (out of the scripts/ci/** glob) behind a new npm run test:build, wired into the 'e2e' job immediately after Build. A missing or stale dist/ is now a hard failure instead of a skip - a skip is how this stayed invisible. 2. The assertions named vscomp-animation-spin, but the defect class is 'whichever rule follows the banner'. Nothing is hard-coded now: Node asserts only ASCII whitespace separates the banner from the first rule and that every declared @Keyframes is referenced; Cypress asserts the CSSOM's first rule is the file's first rule, and that every referenced animation resolves to a surviving @Keyframes. Reordering the partials can no longer silence the guard. 3. dist-archive/ was rewritten with post-1.3.0 code by the rebuild. Never committed; restored. Recurs on every local build until the version is bumped, which 'Before tagging' already covers. Corrects a false claim in 237fd03's comments: the Cypress byte checks did not pass 'because the HTTP layer strips the BOM'. cy.request() with a relative path resolved against a baseUrl ending in '#/', so the request hit the document root and the server returned index.html - 2478 bytes of ASCII. They were inspecting the docs homepage, never the stylesheet; curl against the same path returns the real CSS with its BOM. The browser-side cases now fetch sheet.href via win.fetch(), which cannot drift from what is under test. Also fixes a vacuous assertion introduced while writing finding 2's guard: JS counts U+FEFF as whitespace, so trim()/trimStart() deleted the character being looked for and the check passed on a BOM-carrying build. The gap before the first rule is now inspected untrimmed. Verified: 3 of 4 Node checks and 3 of 4 Cypress cases red against a bundle built from master's config, all green with the fix. Suite 416/416 across 27 specs; validate clean.
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.
Issue number: resolves #
What is the current behavior?
@keyframes vscomp-animation-spinis silently deleted from the shipped stylesheet, so the optionsloader renders as a motionless arc. "Loading" and "hung" become indistinguishable.
It is latent on
masterright now and detonates on the nextnpm run build— the committeddist/predates the rule that triggers it, which is the only reason it has not been noticed.Measured in Chrome by counting keyframes rules in the CSSOM:
vscomp-animation-spinin CSSOManimation-nameon the loadermaster's committed CSSvscomp-animation-spinvscomp-animation-spinThe reference survives while its target does not, which is what produces a frozen spinner rather
than a visible error.
Root cause — four steps, none of them wrong on its own
content: '\26A0'.⚠(U+26A0) in the output. Confirmed: a freshbuild contains U+26A0;
master's committed CSS contains no non-ASCII byte at all.@charset "UTF-8";in expanded output, and a U+FEFF BOM in the compressed output we ship.Harmless at offset 0, where every parser strips it.
webpack.BannerPluginprepends the licence banner in front of it, so the BOM lands atoffset 167, mid-file — immediately before the first rule. U+FEFF is ≥ U+0080 and therefore a
valid CSS ident code point, so the parser reads
<FEFF>@keyframes vscomp-animation-spin {…}asa qualified rule with an unparseable selector and discards the pair.
Verified directly against
sassrather than inferred from the docs:charset: falseexpanded@charset "UTF-8";compressed(what ships)Only the first rule after the banner dies. That it is currently the spinner is luck of partial
ordering — reorder the partials and a different rule vanishes instead, which is why this is fixed at
the source rather than by moving anything around. No source file contains a BOM; all of
src/wasscanned.
What is the new behavior?
Two changes, and both are needed:
sassOptions: { charset: false }onsass-loader— the encoding hint is not emitted at all,so no BOM can ever reach the banner. This is the systemic half: it protects against any future
non-ASCII character, not only today's
⚠.\26A0rather than the resolved character(
string.unquote('"\26A0"')), so the stylesheet stays ASCII. This matters because with noencoding hint emitted, a non-ASCII byte would decode according to the consuming page's charset —
keeping the sheet ASCII removes that dependency instead of trading one risk for another.
Verified on the built output: BOM count 0, non-ASCII bytes 0,
@keyframes vscomp-animation-spinback in the CSSOM, and the cue still resolves to⚠in the browser from.vscomp-error-message::before{content:"\26A0"}.Does this introduce a breaking change?
No source behaviour, API, class name or visual output changes — other than the spinner spinning
again. The CSS is otherwise equivalent; the only byte-level difference is the removed BOM and the
cue being an escape rather than a literal character, which the CSS parser resolves identically.
Other information
Tests — a class of coverage this repo did not have
Every existing spec exercises behaviour, so none of them can see a rule the CSS parser threw away.
That is precisely how this got through. Two complementary guards:
scripts/build-checks/__tests__/stylesheet-bytes.test.mjs— run by the newnpm run test:buildin the
e2ejob immediately after Build, reading the built file from disk. Asserts no BOM,ASCII-only, that only ASCII whitespace separates the banner from the first rule, and that every
declared
@keyframesis referenced. 3 of 4 red against a bundle built frommaster's config,e.g.:
A missing or stale
dist/is a hard failure, not a skip.Why
test:buildand nottest:scripts: thestaticjob that runstest:scriptsnever builds,and the committed
dist/is deliberately stale — it contains neithervscomp-error-messagenor26A0. Placed there, these assertions passed against a bundle predating the rule they exist toprotect, and a PR deleting
charset: falsewithout rebuilding would have stayed green. That wasverified against the actual workflow and the committed artefact, not assumed.
cypress/e2e/build-stylesheet-integrity.cy.ts— the half only a browser can answer: that theCSSOM's first rule is the file's first rule, that every animation the file references resolves to
a surviving
@keyframes, that the loader's animation is actually applied, and that the cue reachesthe page. 3 of 4 red against the same unfixed bundle.
Nothing is hard-coded to
vscomp-animation-spin: the defect class is "whichever rule follows thebanner", so naming today's first rule would stop guarding anything once the partials are reordered.
Both sides are derived from the file.
Verification
npm run test:scripts75/75 (3 red before the fix)npm run validate(tsc + ESLint + Stylelint) cleanNo build output is committed —
dist/,dist-archive/anddocs/assets/stay pinned tomaster,per the PR rule in
.github/README.md. Note that the fix therefore has no visible effect until arelease build is run.
Correction to an earlier revision of this description
An earlier version of this PR stated that the Cypress byte checks passed against a bad build
"because the HTTP layer strips the BOM in transit, even with
encoding: 'binary'". That waswrong, and the real cause was found while making the assertions generic:
cy.request('assets/virtual-select.min.css')resolves against abaseUrlending in#/, so therequest went to the document root and the server returned
index.html— 2,478 bytes of ASCII,BOM-free. The assertions were inspecting the docs homepage and never saw the stylesheet at all.
curlagainst the same path returns the real CSS with its BOM, so neither the server nor the HTTPlayer was ever the problem.
The browser-side cases now fetch
sheet.href— the URL the page actually loaded — viawin.fetch(),which cannot drift from what is under test.
One related bug of my own is also fixed here: the first version of the generic "nothing precedes the
first rule" guard used
trimStart()to isolate the gap, and JS counts U+FEFF as whitespace, soit deleted the exact character it was looking for and passed on a BOM-carrying build. The gap is now
inspected untrimmed, and both files carry a comment saying why.