fix(build): stop a stray BOM deleting the loader keyframes from the shipped CSS

.github/workflows/pr-tests.yml

@@ -110,6 +110,13 @@ jobs:
       - name: Build
         run: node scripts/ci/run-step.mjs Build npm run build
 
+      # Byte-level assertions on the artefacts the build just produced (no BOM, ASCII-only,
+      # keyframes intact). This has to live in *this* job rather than in `static`: those checks
+      # read dist/, `static` never builds, and the committed dist/ is deliberately stale — so
+      # there they passed against a bundle predating the rule they exist to protect.
+      - name: Build output checks
+        run: node scripts/ci/run-step.mjs "Build Output" npm run test:build
+
       # Deliberately not `if: always()` — if the build failed there is nothing
       # meaningful to test, and the missing fragment renders as "E2E skipped".
       - name: E2E

cypress/e2e/build-stylesheet-integrity.cy.ts

@@ -0,0 +1,160 @@
+/** cSpell:ignore vscomp */
+
+/**
+ * The shipped stylesheet must actually parse.
+ *
+ * Every other spec exercises behaviour, so none of them can see a rule that the CSS parser
+ * silently discarded. That gap let a real defect through: Dart Sass prepends an encoding hint when
+ * 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 position 0 is stripped
+ * by every parser; mid-file 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". Any first rule is vulnerable;
+ * the spinner was simply the one that happened to be there.
+ *
+ * These cases run against the built bundle the docs site loads, so they fail if the build
+ * regresses, not merely if the source does.
+ */
+
+/** The stylesheet under test, as the docs site loads it. */
+const STYLESHEET = 'virtual-select.min.css';
+
+/** Our own sheet only - the docs page also loads fonts and vue.css. */
+const ownStyleSheet = (win: Window): CSSStyleSheet => {
+  const sheet = Array.from(win.document.styleSheets).find((s) => s.href?.includes(STYLESHEET));
+
+  expect(sheet, `${STYLESHEET} is loaded by the page`).to.not.equal(undefined);
+
+  return sheet as CSSStyleSheet;
+};
+
+/**
+ * Fetch the stylesheet's source through the **browser**, using the `href` the page actually loaded.
+ *
+ * Not `cy.request('assets/…')`: `baseUrl` ends in `#/`, so a relative path resolves to the document
+ * root and the server answers with `index.html`. An earlier version of this spec did that and its
+ * assertions were green against a build carrying a BOM - they were inspecting the docs homepage, not
+ * the stylesheet. Reading `sheet.href` cannot drift from what is under test.
+ */
+const fetchOwnStyleSheetText = (win: Window) =>
+  cy.wrap(
+    win
+      .fetch(ownStyleSheet(win).href as string)
+      .then((response) => {
+        expect(response.ok, `${STYLESHEET} fetched`).to.equal(true);
+        return response.text();
+      }),
+    { log: false },
+  ) as unknown as Cypress.Chainable<string>;
+
+describe('Build: the shipped stylesheet parses', { testIsolation: true }, () => {
+  beforeEach(() => {
+    cy.viewport(1280, 800);
+    cy.visit('properties');
+  });
+
+  /**
+   * Compares the file against the CSSOM instead of naming a rule. A mid-file BOM destroys
+   * **whichever** rule follows the banner, so hard-coding today's first rule would stop guarding
+   * anything the moment the partials are reordered. If the parser's first rule is not the file's
+   * first rule, something was swallowed.
+   */
+  it('accepts the first rule in the file as its first rule', () => {
+    cy.window().then((win) => {
+      fetchOwnStyleSheetText(win).then((css) => {
+        const afterBanner = css.slice(css.indexOf('*/') + 2);
+        // No trim(): JS counts U+FEFF as whitespace, so trimming would discard the very
+        // character that causes the defect.
+        const fileFirstHead = afterBanner.slice(0, afterBanner.indexOf('{')).replace(/^[\s]+/, '').trim();
+
+        const firstRule = ownStyleSheet(win).cssRules[0];
+        const parsedFirstHead = firstRule.cssText.slice(0, firstRule.cssText.indexOf('{')).trim();
+
+        // Normalised: the CSSOM re-serialises `@keyframes` and spacing in its own style.
+        const normalise = (s: string) => s.replace(/\s+/g, ' ').toLowerCase();
+
+        expect(normalise(parsedFirstHead), 'the parser kept the file first rule').to.equal(
+          normalise(fileFirstHead),
+        );
+      });
+    });
+  });
+
+  /**
+   * The consequence check, derived from the file rather than hard-coded: every animation the
+   * stylesheet references must resolve to a `@keyframes` the parser kept. A dangling
+   * `animation-name` is silent at runtime - the element simply never animates - which is what made
+   * the original defect invisible.
+   */
+  it('resolves every animation it references to a surviving keyframes rule', () => {
+    cy.window().then((win) => {
+      fetchOwnStyleSheetText(win).then((css) => {
+        const referenced = new Set(
+          [...css.matchAll(/animation(?:-name)?:([^;}]+)/g)]
+            .flatMap((m) => m[1].split(/[\s,]+/))
+            .filter((token) => new RegExp(`@(?:-\\w+-)?keyframes\\s+${token}\\b`).test(css)),
+        );
+
+        expect(referenced.size, 'the stylesheet references at least one animation').to.be.greaterThan(0);
+
+        const survived = Array.from(ownStyleSheet(win).cssRules)
+          .filter((rule) => rule.type === CSSRule.KEYFRAMES_RULE)
+          .map((rule) => (rule as CSSKeyframesRule).name);
+
+        referenced.forEach((name) => {
+          expect(survived, `@keyframes ${name} survived parsing`).to.include(name);
+        });
+      });
+    });
+  });
+
+  /** And the animation is actually applied - the CSSOM having the rule is necessary, not enough. */
+  it('applies a surviving animation to the element that asks for it', () => {
+    cy.window().then((win) => {
+      const $loader = win.document.createElement('div');
+      $loader.className = 'vscomp-options-loader';
+      win.document.body.appendChild($loader);
+
+      const { animationName, animationDuration } = win.getComputedStyle($loader, '::before');
+      $loader.remove();
+
+      expect(animationName, 'the loader names an animation').to.not.be.oneOf(['none', '']);
+      expect(animationDuration, 'and it has a non-zero duration').to.not.equal('0s');
+
+      const survived = Array.from(ownStyleSheet(win).cssRules)
+        .filter((rule) => rule.type === CSSRule.KEYFRAMES_RULE)
+        .map((rule) => (rule as CSSKeyframesRule).name);
+
+      expect(survived, `the loader's animation "${animationName}" exists`).to.include(animationName);
+    });
+  });
+
+  /**
+   * The byte-level guards on the *cause* - no BOM, ASCII-only - live in
+   * `scripts/build-checks/__tests__/stylesheet-bytes.test.mjs`, run by `npm run test:build`
+   * immediately after the build. They read the artefact from disk, with no server or HTTP layer in
+   * between, which is the most direct place to assert about bytes.
+   *
+   * What stays here is the half only a browser can answer: whether the rules survive *parsing*, and
+   * whether the cue reaches the page. A file can be byte-perfect and still parse wrongly, and the
+   * CSSOM is the only witness to that.
+   */
+
+  /** The cue itself must still reach the page - an ASCII escape the CSS parser decodes. */
+  it('still renders the non-colour error cue', () => {
+    cy.window().then((win) => {
+      const $message = win.document.createElement('div');
+      $message.className = 'vscomp-error-message';
+      win.document.body.appendChild($message);
+
+      const content = win.getComputedStyle($message, '::before').content;
+      $message.remove();
+
+      // Chrome resolves the escape, so the computed value is the character itself.
+      expect(content, 'warning sign resolves from the \\26A0 escape').to.contain('⚠');
+    });
+  });
+});

package.json

@@ -10,6 +10,7 @@
     "lint:js": "eslint src",
     "lint:css": "stylelint src/sass/**/*.scss",
     "test:scripts": "node --test \"scripts/ci/**/*.test.mjs\"",
+    "test:build": "node --test \"scripts/build-checks/**/*.test.mjs\"",
     "docs": "docsify serve docs",
     "t": "docsify serve docs -p 3001 | cypress open --e2e -b chrome",
     "test": "node scripts/ci/run-e2e.mjs"

scripts/build-checks/__tests__/stylesheet-bytes.test.mjs

@@ -0,0 +1,169 @@
+/**
+ * Byte-level guards on the stylesheet we ship.
+ *
+ * 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 webpack's BannerPlugin then prepends the
+ * licence banner in front of it. A BOM at position 0 is stripped by every CSS parser; mid-file 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. That silently deleted `@keyframes vscomp-animation-spin`
+ * while `.vscomp-options-loader::before` kept referencing it, leaving the options loader frozen.
+ *
+ * These assertions live here rather than in Cypress deliberately. Written as
+ * `cy.request('assets/virtual-select.min.css')` they passed against a bundle that genuinely carried
+ * a BOM - because Cypress's `baseUrl` ends in `#/`, so the relative path resolved to the document
+ * root and the server answered with `index.html`: the assertions were inspecting the docs homepage,
+ * which is ASCII and BOM-free, and reported green while never seeing the stylesheet at all. Reading
+ * the built file from disk removes both the URL and the HTTP layer from the question.
+ *
+ * `npm run test:build`, NOT `npm run test:scripts`, and that separation is load-bearing. These read
+ * `dist/`, which is committed deliberately stale (build output is not committed per
+ * `.github/README.md`), while the `static` CI job that runs `test:scripts` never builds. Run there,
+ * they passed against a bundle predating the very rule they exist to protect - a PR deleting
+ * `charset: false` without rebuilding would have stayed green. They run in the `e2e` job instead,
+ * immediately after `npm run build`.
+ *
+ * For the same reason a missing or stale `dist/` is a hard failure rather than a skip: a skip is
+ * how the original mistake stayed invisible.
+ */
+import test from 'node:test';
+import assert from 'node:assert/strict';
+import { readFile, stat } from 'node:fs/promises';
+import { fileURLToPath } from 'node:url';
+import path from 'node:path';
+
+const ROOT = fileURLToPath(new URL('../../../', import.meta.url));
+const CSS_PATH = path.join(ROOT, 'dist/virtual-select.min.css');
+const SCSS_DIR = path.join(ROOT, 'src/sass');
+
+const HINT = 'Run `npm run build` first — these assertions describe the built artefact, not the source.';
+
+/**
+ * Newest mtime under src/sass, so a `dist/` older than the stylesheet source is reported as stale
+ * rather than quietly asserted against. Without this the suite is green whenever someone edits SCSS
+ * and forgets to rebuild, which is exactly the blind spot being closed.
+ */
+async function newestScssMtime(dir = SCSS_DIR) {
+  const { readdir } = await import('node:fs/promises');
+  const entries = await readdir(dir, { withFileTypes: true });
+  let newest = 0;
+
+  for (const entry of entries) {
+    const full = path.join(dir, entry.name);
+    const mtime = entry.isDirectory() ? await newestScssMtime(full) : (await stat(full)).mtimeMs;
+    if (mtime > newest) newest = mtime;
+  }
+
+  return newest;
+}
+
+async function readCssBytes() {
+  let bytes;
+
+  try {
+    bytes = await readFile(CSS_PATH);
+  } catch (error) {
+    if (error.code === 'ENOENT') assert.fail(`dist/virtual-select.min.css is missing. ${HINT}`);
+    throw error;
+  }
+
+  const [built, source] = [(await stat(CSS_PATH)).mtimeMs, await newestScssMtime()];
+  assert.ok(built >= source, `dist/virtual-select.min.css is older than src/sass. ${HINT}`);
+
+  return bytes;
+}
+
+test('the shipped stylesheet carries no byte-order mark', async () => {
+  const bytes = await readCssBytes();
+  const found = [];
+
+  for (let i = 0; i < bytes.length - 2; i += 1) {
+    if (bytes[i] === 0xef && bytes[i + 1] === 0xbb && bytes[i + 2] === 0xbf) found.push(i);
+  }
+
+  assert.deepEqual(
+    found,
+    [],
+    `EF BB BF (U+FEFF) found at byte offset ${found.join(', ')}. A BOM anywhere but offset 0 ` +
+      'destroys the rule that follows it; BannerPlugin guarantees offset 0 is not where Sass put ' +
+      'it. Fix with `charset: false` in the sass-loader options, not by reordering rules.',
+  );
+});
+
+test('the shipped stylesheet is ASCII, so it needs no encoding declaration', async () => {
+  const bytes = await readCssBytes();
+  const found = [];
+
+  for (let i = 0; i < bytes.length; i += 1) {
+    if (bytes[i] > 0x7f) found.push(`0x${bytes[i].toString(16)}@${i}`);
+  }
+
+  assert.deepEqual(
+    found,
+    [],
+    `non-ASCII bytes: ${found.slice(0, 8).join(', ')}. With no encoding hint emitted, a non-ASCII ` +
+      "byte decodes according to the consuming page's charset. Emit the character as a CSS escape " +
+      "instead - e.g. string.unquote('\"\\\\26A0\"') - so the sheet stays ASCII.",
+  );
+});
+
+/**
+ * Derived from the file rather than hard-coded: the defect class is "a rule went missing", and
+ * naming one rule only guards the rule that happened to be first when it was written. Both sides of
+ * every animation are checked, so a dangling `animation-name` fails whichever animation it is.
+ */
+test('every animation the stylesheet references resolves to a keyframes rule', async () => {
+  const css = (await readCssBytes()).toString('utf8');
+
+  const declared = new Set([...css.matchAll(/@(?:-\w+-)?keyframes\s+([\w-]+)/g)].map((m) => m[1]));
+  /** the `animation` shorthand puts the name anywhere among its values, so take every ident and
+   *  keep the ones that name a real animation - matched loosely because the minifier rewrites
+   *  `0.8s` to `.8s`, and pinning the shorthand would assert its formatting instead */
+  const referenced = new Set(
+    [...css.matchAll(/animation(?:-name)?:([^;}]+)/g)]
+      .flatMap((m) => m[1].split(/[\s,]+/))
+      .filter((token) => declared.has(token)),
+  );
+
+  assert.ok(declared.size > 0, 'no @keyframes rules survived in the build output at all');
+  assert.ok(referenced.size > 0, 'no rule references any animation - the parser may have eaten one');
+
+  // Every declared animation should be used, and every used one declared.
+  assert.deepEqual(
+    [...declared].filter((name) => !referenced.has(name)),
+    [],
+    'these @keyframes are declared but referenced by nothing - either dead CSS or the rule that ' +
+      'used them was dropped',
+  );
+});
+
+/**
+ * The generic guard for the failure mode: a mid-file BOM destroys **whichever** rule follows the
+ * banner, so this names no rule. Only ASCII whitespace may sit between the banner and the first
+ * rule; anything else merges into the selector and the parser discards the rule.
+ *
+ * Deliberately does NOT use `trim()`/`trimStart()` to isolate the gap. JS treats U+FEFF as
+ * whitespace, so trimming silently removes the very character being looked for - an earlier version
+ * of this test did exactly that and passed against a build carrying a BOM at offset 167.
+ */
+test('nothing but ASCII whitespace separates the banner from the first rule', async () => {
+  const css = (await readCssBytes()).toString('utf8');
+
+  const bannerEnd = css.indexOf('*/');
+  assert.notEqual(bannerEnd, -1, 'the licence banner is missing from the build output');
+
+  const gap = css.slice(bannerEnd + 2, css.indexOf('{'));
+  const preludeStart = gap.search(/\S/) === -1 ? gap.length : gap.search(/[^\s]/);
+  const separator = gap.slice(0, preludeStart);
+
+  const offending = [...separator]
+    .map((char, i) => ({ char, i }))
+    .filter(({ char }) => !' \t\r\n'.includes(char))
+    .map(({ char, i }) => `U+${char.codePointAt(0).toString(16).toUpperCase()} at +${i}`);
+
+  assert.deepEqual(
+    offending,
+    [],
+    `invisible code point(s) between the banner and the first rule: ${offending.join(', ')}. ` +
+      'These merge into the following selector and the parser drops that rule entirely.',
+  );
+});

src/sass/partials/virtual-select.scss

@@ -1,4 +1,5 @@
 @use 'sass:math';
+@use 'sass:string';
 @use './variable' as v;
 @use './mixins' as m;
 
@@ -331,9 +332,19 @@
   padding-top: 4px;
   width: 100%;
 
-  /** shape cue, so the failure is not signalled by colour alone */
+  /**
+   * Shape cue, so the failure is not signalled by colour alone.
+   *
+   * The escape is kept intact in the output rather than written as a literal warning sign:
+   * Sass resolves `'\26A0'` to the character itself, and a single non-ASCII character makes Sass
+   * emit an encoding hint - a U+FEFF BOM in compressed output - which BannerPlugin then pushes
+   * into the middle of the file, where it destroys the rule that follows. `charset: false` in
+   * webpack.config.js stops the hint being emitted at all; keeping the stylesheet ASCII means
+   * there is nothing to declare in the first place, so the glyph does not depend on the consuming
+   * page's encoding either.
+   */
   &::before {
-    content: '\26A0';
+    content: string.unquote('"\\26A0"');
     flex: none;
     font-size: 1.1em;
     line-height: 1.2;