diff --git a/.github/workflows/pr-tests.yml b/.github/workflows/pr-tests.yml index d977b46..7ba3b28 100644 --- a/.github/workflows/pr-tests.yml +++ b/.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 diff --git a/cypress/e2e/build-stylesheet-integrity.cy.ts b/cypress/e2e/build-stylesheet-integrity.cy.ts new file mode 100644 index 0000000..3027e15 --- /dev/null +++ b/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; + +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('⚠'); + }); + }); +}); diff --git a/package.json b/package.json index 72cb4a0..19a6ceb 100644 --- a/package.json +++ b/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" diff --git a/scripts/build-checks/__tests__/stylesheet-bytes.test.mjs b/scripts/build-checks/__tests__/stylesheet-bytes.test.mjs new file mode 100644 index 0000000..7d5a541 --- /dev/null +++ b/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.', + ); +}); diff --git a/src/sass/partials/virtual-select.scss b/src/sass/partials/virtual-select.scss index 88be746..4612286 100755 --- a/src/sass/partials/virtual-select.scss +++ b/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; diff --git a/webpack.config.js b/webpack.config.js index 3354b39..135f317 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -70,7 +70,37 @@ module.exports = (env, options) => { { test: /\.scss$/, exclude: /(node_modules)/, - use: [MiniCssExtractPlugin.loader, 'css-loader', 'postcss-loader', 'sass-loader'], + use: [ + MiniCssExtractPlugin.loader, + 'css-loader', + 'postcss-loader', + { + loader: 'sass-loader', + options: { + sassOptions: { + /** + * Dart Sass prepends an encoding hint whenever the output contains a non-ASCII + * character - `@charset "UTF-8";` in expanded output, and a U+FEFF BOM in the + * compressed output we ship. BannerPlugin then prepends the licence banner in + * front of it, so the BOM lands mid-file, where U+FEFF is a valid CSS ident + * code point rather than an ignorable mark: the parser reads it as the start of + * a selector, swallows the rule that follows and drops the pair. + * + * That silently deleted `@keyframes vscomp-animation-spin` - the first rule + * after the banner - while `.vscomp-options-loader::before` kept referencing it, + * so the options loader rendered frozen. Any first rule is vulnerable; the + * spinner was only the one that happened to be there. + * + * The stylesheet is ASCII (the `\26A0` cue is emitted as an escape, see + * virtual-select.scss), so there is no encoding to declare and nothing is lost + * by suppressing the hint. Kept as a rule rather than relying on the source + * staying ASCII, so a future non-ASCII character cannot resurrect the BOM. + */ + charset: false, + }, + }, + }, + ], }, { test: /\.css$/,