From d8f7024b2f21a646bbdb51d20146a0479d36f7f3 Mon Sep 17 00:00:00 2001 From: Tomas Madariaga Date: Mon, 28 Sep 2026 16:08:51 +0200 Subject: [PATCH] fix(packages/sui-critical-css): carry the definition at-rules into the rebuilt CSS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chrome's CSS coverage reports ranges that cover style rules only. 1.34.0 used that to restore the at-rules a covered rule is NESTED IN, but an at-rule that defines something instead of styling something matches no element, so it is never attributable to a range and no amount of nesting context brings it back. Measured on a Tailwind v4 app: the rebuilt CSS had 0 `@property`, 0 `@font-face` and 0 `@keyframes` against 29, 5 and 30 in the source sheet. `@property` is the one that changes what the browser paints. With the registration missing, `border-style: var(--tw-border-style)` resolves to the empty token, which is invalid at computed-value time, so `border-style` computes to its initial `none` — and with `border-style: none` the computed `border-width` is `0px` even though the utility's `1px` applied and won the cascade. Every button styled that way renders with no border until the full stylesheet arrives, and since consumers make the real sheets async whenever the critical CSS is non-empty, that is a visible jump rather than a progressive paint. So: - `@property` and `@font-face` are always kept, the same exception `@layer` statements already have. Both are cheap and safe when unused: a registration only sets an initial value, and a `@font-face` never triggers a download unless a matched rule asks for the family. - `@keyframes` is not bounded in size, so it is only kept when a declaration that survived the rebuild names it (`animation` or `animation-name`, vendor prefixes included). Scanning only the DIRECT declarations of the kept nodes, so a kept `@media` does not drag in the keyframes named by the children that were discarded from it. Measured on the same 425 KB production sheet with the coverage of 221 rules: 33279 -> 36913 bytes unminified (+3.6 KB), 29 of 29 `@property`, 5 of 5 `@font-face`, and 1 of 30 `@keyframes` kept. Co-Authored-By: Claude Opus 5 --- packages/sui-critical-css/README.md | 5 ++ packages/sui-critical-css/src/covered-css.js | 66 +++++++++++++++++++ .../test/server/covered-cssSpec.js | 50 ++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/packages/sui-critical-css/README.md b/packages/sui-critical-css/README.md index cf143b5e8..2fab3a989 100644 --- a/packages/sui-critical-css/README.md +++ b/packages/sui-critical-css/README.md @@ -6,6 +6,11 @@ 1. Read the config options and routes provided. 2. For each route, it opens a browser, navigate and extract the Critical CSS. + Chrome reports coverage for style rules only, so the extracted CSS is rebuilt with postcss + instead of concatenating the raw byte ranges. That keeps the `@media` / `@layer` / `@supports` + context of every rule it keeps, the `@layer` statements that fix the layer order, and the + at-rules that define rather than style: `@property`, `@font-face`, and the `@keyframes` named by + a declaration that survived. 3. Create a css file in the `critical-css` folder. 4. After doing this for each route, then creates a `critical.json` file that could be read for every path to extract the critical-css. 5. Use then `@s-ui/critical-css-middleware` to extract to use in your Express app the CSS. diff --git a/packages/sui-critical-css/src/covered-css.js b/packages/sui-critical-css/src/covered-css.js index da85a073f..ed27a1ede 100644 --- a/packages/sui-critical-css/src/covered-css.js +++ b/packages/sui-critical-css/src/covered-css.js @@ -14,8 +14,38 @@ import postcss from 'postcss' // So instead of slicing bytes, parse the stylesheet and rebuild it: keep every // declaration-holding node whose source range intersects a covered range, together // with its whole chain of ancestor at-rules. +// +// The same "ranges cover style rules only" limitation also means an at-rule that +// DEFINES something rather than styling something is never attributable to a range, +// so it can only be carried over by an explicit exception. See `isDefinitionAtRule` +// and `isKeyframes` below. const KEEP_AT_RULE = 'layer' +// `@property` registers a custom property and `@font-face` a font: neither matches an +// element, so coverage never reports a range for them and both used to be dropped from +// every rebuild. Measured on a Tailwind v4 app: 0 `@property` and 0 `@font-face` in the +// rebuilt CSS against 29 and 5 in the source sheet. +// +// The `@property` gap changes what the browser paints. `border-style:var(--tw-border-style)` +// with the registration missing resolves to the empty token, which is invalid at computed-value +// time, so `border-style` computes to its initial `none` — and with `border-style:none` the +// computed `border-width` is `0px` even though the utility's `1px` applied and won the cascade. +// A button styled that way renders with no border at all until the full stylesheet arrives. +// +// Both are cheap to keep and safe to keep when unused: a registration only sets an initial +// value, and a `@font-face` never triggers a download unless a matched rule asks for the family. +const DEFINITION_AT_RULES = ['property', 'font-face'] + +// `@keyframes` holds rules, not declarations, and those inner rules +// (`from{}`, `50%{}`) never match an element either. Unlike the two above it is not +// bounded in size, so it is only carried over when a declaration that survived the +// rebuild actually names it. +const KEYFRAMES_AT_RULE = /^(-\w+-)?keyframes$/ +const ANIMATION_DECLARATIONS = ['animation', 'animation-name'] +const ANIMATION_VALUE_SEPARATOR = /[\s,]+/ + +const atRuleName = node => node.name.toLowerCase() + const holdsDeclarations = node => node.type === 'rule' || (node.type === 'atrule' && node.nodes?.some(child => child.type === 'decl')) @@ -24,6 +54,26 @@ const holdsDeclarations = node => const isLayerStatement = node => node.type === 'atrule' && node.nodes === undefined && node.name.toLowerCase() === KEEP_AT_RULE +const isDefinitionAtRule = node => node.type === 'atrule' && DEFINITION_AT_RULES.includes(atRuleName(node)) + +const isKeyframes = node => node.type === 'atrule' && KEYFRAMES_AT_RULE.test(atRuleName(node)) + +// Only the direct declarations of a node, so scanning a kept `@media` does not reach the +// declarations of the children that were discarded from it. +const animationNamesOf = node => { + const names = [] + + node.each?.(child => { + if (child.type !== 'decl' || !ANIMATION_DECLARATIONS.includes(child.prop.toLowerCase())) return + + // Every token of the shorthand, since the name can sit anywhere in it. A duration or a + // timing function that happens to match a `@keyframes` name only keeps one extra rule. + names.push(...child.value.split(ANIMATION_VALUE_SEPARATOR)) + }) + + return names +} + // One statement per layer name, because clean-css 5.3.3 mis-parses a bodyless `@layer` // that names more than one layer: it drops the statement AND every declaration of the // rule that follows it (`@layer a,b;.x{color:red}` minifies to nothing at all). A @@ -54,11 +104,19 @@ export const rebuildCoveredCSS = ({text, ranges}) => { const keep = new Set() const layerStatements = [] const nestedLayerStatements = [] + const keyframes = [] const keepWithAncestors = node => { for (let current = node; current && current.type !== 'root'; current = current.parent) keep.add(current) } + // An at-rule kept as a whole needs its children kept too, or the pass that removes + // everything uncovered would empty it out. + const keepWithSubtree = node => { + keepWithAncestors(node) + node.walk?.(child => keep.add(child)) + } + root.walk(node => { if (isLayerStatement(node)) { if (node.parent.type === 'root') return layerStatements.push(...splitLayerStatement(node)) @@ -68,9 +126,17 @@ export const rebuildCoveredCSS = ({text, ranges}) => { // cannot be hoisted out of its parent the way a top-level one can. Keep it in place. return nestedLayerStatements.push(node) } + if (isDefinitionAtRule(node)) return keepWithSubtree(node) + if (isKeyframes(node)) return keyframes.push(node) if (holdsDeclarations(node) && intersectsCoveredRange(node, ranges)) keepWithAncestors(node) }) + // After the walk, so it sees every node the rebuild is going to keep. + const animationNames = new Set([...keep].flatMap(animationNamesOf)) + keyframes.forEach(node => { + if (animationNames.has(node.params.trim())) keepWithSubtree(node) + }) + // Replacing them during the walk above would make postcss visit the statements the split // produces, so they are only rewritten once the walk is over. nestedLayerStatements.forEach(node => { diff --git a/packages/sui-critical-css/test/server/covered-cssSpec.js b/packages/sui-critical-css/test/server/covered-cssSpec.js index 56a58943e..162f6841b 100644 --- a/packages/sui-critical-css/test/server/covered-cssSpec.js +++ b/packages/sui-critical-css/test/server/covered-cssSpec.js @@ -108,6 +108,56 @@ describe('@s-ui/critical-css covered-css', () => { ) }) + describe('definition at-rules', () => { + it('keeps a @property registration even though coverage never marks it as used', () => { + const text = '@property --tw-border-style{syntax:"*";inherits:false;initial-value:solid}.used{color:red}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('keeps the at-rules a @property registration is nested in', () => { + const text = '@layer properties{@property --x{syntax:"*";inherits:false}}.used{color:red}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('keeps a @font-face nobody covered, so the first paint does not swap fonts', () => { + const text = '@font-face{font-family:F;src:url(f.woff2)}.used{color:red}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('keeps a @keyframes named by a declaration that survived the rebuild', () => { + const text = '@keyframes spin{to{transform:rotate(1turn)}}.used{animation:spin 1s linear infinite}' + + expect(rebuild(text, '.used{animation:spin 1s linear infinite}')).to.equal(text) + }) + + it('keeps a @keyframes named by animation-name', () => { + const text = '@keyframes spin{to{opacity:0}}.used{animation-name:spin}' + + expect(rebuild(text, '.used{animation-name:spin}')).to.equal(text) + }) + + it('keeps a vendor-prefixed @keyframes', () => { + const text = '@-webkit-keyframes spin{to{opacity:0}}.used{animation:spin 1s}' + + expect(rebuild(text, '.used{animation:spin 1s}')).to.equal(text) + }) + + it('drops a @keyframes no kept declaration names', () => { + const text = '@keyframes spin{to{opacity:0}}.used{color:red}' + + expect(rebuild(text, '.used{color:red}')).to.equal('.used{color:red}') + }) + + it('does not keep a @keyframes named only by a rule that was discarded', () => { + const text = '@keyframes spin{to{opacity:0}}.used{color:red}.unused{animation:spin 1s}' + + expect(rebuild(text, '.used{color:red}')).to.equal('.used{color:red}') + }) + }) + it('returns an empty string when nothing is covered', () => { expect(rebuildCoveredCSS({text: '@media print{.unused{color:blue}}', ranges: []})).to.equal('') })