fix(packages/sui-critical-css): preserve at-rule context when rebuilding covered CSS

package.json

@@ -20,7 +20,7 @@
     "test:server:ci": "npm run test:server",
     "test:client": "npx @s-ui/test browser -P 'packages/**/!(server)/*Spec.js' -H",
     "test:client:watch": "npm run test:client -- --watch",
-    "test:server": "npx @s-ui/test server -P 'packages/{sui-test-contract,sui-js-compiler}/**/server/*Spec.js'",
+    "test:server": "npx @s-ui/test server -P 'packages/{sui-test-contract,sui-js-compiler,sui-critical-css}/**/server/*Spec.js'",
     "test:server:watch": "npm run test:server -- --watch",
     "test:e2e": "node ./packages/sui-studio/test/server/integration/static-server.js ./packages/sui-studio/test/server/integration/sample-studio/public && npx @s-ui/test-e2e --baseUrl=http://localhost:1234",
     "pre-commit": "sui-lint js --staged && sui-lint sass --staged",

packages/sui-critical-css/package.json

@@ -5,13 +5,14 @@
   "type": "module",
   "main": "src/index.js",
   "scripts": {
-    "test": "echo \"Error: no test specified\" && exit 1"
+    "test": "sui-test server"
   },
   "dependencies": {
     "clean-css": "5.3.3",
     "node-fetch": "2.7.0",
     "path-to-regexp": "0.1.10",
     "playwright-chromium": "1.25.0",
+    "postcss": "8.4.31",
     "@s-ui/critical-css-middleware": "1"
   },
   "keywords": [],

packages/sui-critical-css/src/covered-css.js

@@ -0,0 +1,114 @@
+import postcss from 'postcss'
+
+// Chrome's CSS coverage reports ranges that cover the style rule ONLY. The `@media`,
+// `@layer`, `@supports` or `@container` block the rule lives in is never part of the
+// range, so slicing the raw bytes out of `entry.text` and concatenating them produces
+// CSS that has lost every at-rule wrapper. Two things break as a result:
+//
+//   1. Nothing is layered any more, so ties that the layer order used to decide now
+//      fall back to source order. A `@layer base` reset extracted after a
+//      `@layer utilities` rule of the same specificity silently starts winning.
+//   2. Media queries are flattened, so rules only meant for the extraction viewport
+//      apply at every width.
+//
+// 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.
+const KEEP_AT_RULE = 'layer'
+
+const holdsDeclarations = node =>
+  node.type === 'rule' || (node.type === 'atrule' && node.nodes?.some(child => child.type === 'decl'))
+
+// `@layer a, b, c;` is a statement, not a style rule, so coverage never marks it as
+// used — yet it is the only thing that fixes the order of the layers. Always keep it.
+const isLayerStatement = node =>
+  node.type === 'atrule' && node.nodes === undefined && node.name.toLowerCase() === KEEP_AT_RULE
+
+// 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
+// sequence of single-name statements establishes exactly the same order.
+const splitLayerStatement = node => node.params.split(',').map(name => `@${node.name} ${name.trim()};`)
+
+const intersectsCoveredRange = (node, ranges) => {
+  const start = node.source?.start?.offset
+  const end = node.source?.end?.offset
+
+  if (start === undefined || end === undefined) return false
+
+  return ranges.some(range => range.start < end && range.end > start)
+}
+
+/**
+ * Rebuild the used part of a stylesheet from its Chrome CSS coverage ranges, keeping
+ * the at-rule context of every rule it keeps.
+ *
+ * @param {object} params
+ * @param {string} params.text Full source of the stylesheet, as reported by coverage.
+ * @param {Array<{start: number, end: number}>} params.ranges Covered byte ranges.
+ * @returns {string} CSS containing only the covered rules, still wrapped in their
+ *   original at-rules, with every `@layer` statement hoisted to the top.
+ */
+export const rebuildCoveredCSS = ({text, ranges}) => {
+  const root = postcss.parse(text)
+  const keep = new Set()
+  const layerStatements = []
+  const nestedLayerStatements = []
+
+  const keepWithAncestors = node => {
+    for (let current = node; current && current.type !== 'root'; current = current.parent) keep.add(current)
+  }
+
+  root.walk(node => {
+    if (isLayerStatement(node)) {
+      if (node.parent.type === 'root') return layerStatements.push(...splitLayerStatement(node))
+
+      // A statement inside `@supports` or `@media` only registers its layers while that
+      // condition holds, and one inside another `@layer` names sublayers of it, so it
+      // cannot be hoisted out of its parent the way a top-level one can. Keep it in place.
+      return nestedLayerStatements.push(node)
+    }
+    if (holdsDeclarations(node) && intersectsCoveredRange(node, ranges)) keepWithAncestors(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 => {
+    const replacements = splitLayerStatement(node).map(statement => postcss.parse(statement).first)
+
+    node.replaceWith(...replacements)
+    replacements.forEach(keepWithAncestors)
+  })
+
+  const discarded = []
+  root.walk(node => {
+    if ((node.type === 'rule' || node.type === 'atrule') && keep.has(node) === false) discarded.push(node)
+  })
+  discarded.forEach(node => node.remove())
+
+  // Coverage entries are not guaranteed to be in document order, so a `@layer`
+  // statement could otherwise end up after a rule that already registered the layers
+  // in the wrong order. Hoisting makes the result independent of entry order.
+  return [...layerStatements, root.toString()].filter(Boolean).join('\n')
+}
+
+/**
+ * Same as `rebuildCoveredCSS`, but never throws: a stylesheet postcss cannot parse
+ * falls back to the old byte-slicing behaviour instead of failing the whole
+ * extraction. The fallback loses at-rule context, so it is logged loudly.
+ *
+ * @param {object} entry A single Chrome CSS coverage entry.
+ * @returns {string}
+ */
+export const rebuildCoveredCSSFromEntry = entry => {
+  try {
+    return rebuildCoveredCSS(entry)
+  } catch (error) {
+    console.warn(
+      `[ko] Could not parse ${entry.url ?? 'a stylesheet'} (${error.message}).`,
+      'Falling back to raw coverage ranges, so its @media and @layer context is lost.'
+    )
+
+    return entry.ranges.reduce((css, range) => css + entry.text.slice(range.start, range.end), '')
+  }
+}

packages/sui-critical-css/src/extract-from-url.js

@@ -2,6 +2,7 @@ import CleanCSS from 'clean-css'
 import {chromium} from 'playwright-chromium'
 
 import {blockedResourceTypes, skippedResources} from './config.js'
+import {rebuildCoveredCSSFromEntry} from './covered-css.js'
 
 let browser
 
@@ -84,9 +85,7 @@ export async function extractCSSFromUrl({customHeaders, height, url, userAgent,
 
     let coveredCSS = ''
     for (const entry of coverage) {
-      for (const range of entry.ranges) {
-        coveredCSS += entry.text.slice(range.start, range.end)
-      }
+      coveredCSS += rebuildCoveredCSSFromEntry(entry)
     }
 
     // Close the browser to close the connection and free up resources

packages/sui-critical-css/test/server/covered-cssSpec.js

@@ -0,0 +1,137 @@
+import {expect} from 'chai'
+import CleanCSS from 'clean-css'
+
+import {rebuildCoveredCSS, rebuildCoveredCSSFromEntry} from '../../src/covered-css.js'
+
+// Chrome reports coverage as byte ranges into the stylesheet source, and the range of a
+// used rule covers that rule ONLY — never the at-rule it is nested in. These helpers
+// build such a range from a substring so the specs read like the real input.
+const rangeOf = (text, snippet) => {
+  const start = text.indexOf(snippet)
+
+  if (start === -1) throw new Error(`Fixture does not contain "${snippet}"`)
+
+  return {start, end: start + snippet.length}
+}
+
+const rebuild = (text, ...snippets) =>
+  rebuildCoveredCSS({text, ranges: snippets.map(snippet => rangeOf(text, snippet))})
+
+describe('@s-ui/critical-css covered-css', () => {
+  it('keeps the @media a covered rule lives in', () => {
+    const text = '@media (min-width:600px){.used{color:red}}'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal(text)
+  })
+
+  it('keeps the @layer a covered rule lives in', () => {
+    const text = '@layer utilities{.used{color:red}}'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal(text)
+  })
+
+  it('keeps the whole chain of nested at-rules', () => {
+    const text = '@supports (display:grid){@media (min-width:600px){@layer base{.used{color:red}}}}'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal(text)
+  })
+
+  it('drops uncovered siblings but keeps the at-rule of the covered one', () => {
+    const text = '@media print{.used{color:red}.unused{color:blue}}'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal('@media print{.used{color:red}}')
+  })
+
+  it('drops an at-rule with no covered rule at all', () => {
+    const text = '.used{color:red}@media print{.unused{color:blue}}'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal('.used{color:red}')
+  })
+
+  it('keeps unlayered covered rules as they are', () => {
+    const text = '.used{color:red}.unused{color:blue}'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal('.used{color:red}')
+  })
+
+  it('keeps a @layer statement even though coverage never marks it as used', () => {
+    const text = '@layer base,utilities;@layer utilities{.used{color:red}}'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal(
+      '@layer base;\n@layer utilities;\n@layer utilities{.used{color:red}}'
+    )
+  })
+
+  it('keeps a @layer statement even when nothing else is covered', () => {
+    expect(rebuildCoveredCSS({text: '@layer base,utilities;.unused{color:blue}', ranges: []})).to.equal(
+      '@layer base;\n@layer utilities;'
+    )
+  })
+
+  it('hoists @layer statements above the rules, whatever their original position', () => {
+    const text = '@layer utilities{.used{color:red}}@layer base,utilities;'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal(
+      '@layer base;\n@layer utilities;\n@layer utilities{.used{color:red}}'
+    )
+  })
+
+  it('does not hoist a @layer statement out of the at-rule that guards it', () => {
+    const text = '@supports (color:lab(0 0 0)){@layer a,b;}@layer a{.used{color:red}}'
+
+    // Hoisting it would register the order even where the condition does not hold.
+    expect(rebuild(text, '.used{color:red}')).to.equal(
+      '@supports (color:lab(0 0 0)){@layer a;@layer b;}@layer a{.used{color:red}}'
+    )
+  })
+
+  it('does not hoist a @layer statement out of the layer it names sublayers of', () => {
+    const text = '@layer a{@layer b,c;}@layer a{.used{color:red}}'
+
+    expect(rebuild(text, '.used{color:red}')).to.equal('@layer a{@layer b;@layer c;}@layer a{.used{color:red}}')
+  })
+
+  it('splits a multi-name @layer statement so clean-css does not swallow the next rule', () => {
+    const text = '@layer base,utilities;:root{--brand:red}'
+    const rebuilt = rebuildCoveredCSS({text, ranges: [rangeOf(text, ':root{--brand:red}')]})
+
+    // `@layer base,utilities;:root{--brand:red}` minifies to `:root{}` on clean-css
+    // 5.3.3: the statement is dropped and the rule after it loses every declaration.
+    expect(new CleanCSS({level: 2}).minify(rebuilt).styles).to.contain('--brand:red')
+  })
+
+  it('keeps a covered at-rule that holds declarations directly, such as @font-face', () => {
+    const text = '@font-face{font-family:F;src:url(f.woff2)}.unused{color:blue}'
+
+    expect(rebuild(text, '@font-face{font-family:F;src:url(f.woff2)}')).to.equal(
+      '@font-face{font-family:F;src:url(f.woff2)}'
+    )
+  })
+
+  it('returns an empty string when nothing is covered', () => {
+    expect(rebuildCoveredCSS({text: '@media print{.unused{color:blue}}', ranges: []})).to.equal('')
+  })
+
+  it('does not lose a rule whose range only partially overlaps it', () => {
+    const text = '@layer base{.used{color:red}}'
+    const rule = rangeOf(text, '.used{color:red}')
+
+    expect(rebuildCoveredCSS({text, ranges: [{start: rule.start, end: rule.start + 3}]})).to.equal(text)
+  })
+
+  describe('rebuildCoveredCSSFromEntry', () => {
+    it('falls back to the raw coverage ranges when the stylesheet cannot be parsed', () => {
+      const text = '.used{color:red}@media{'
+      const entry = {url: 'https://example.com/broken.css', text, ranges: [rangeOf(text, '.used{color:red}')]}
+
+      expect(rebuildCoveredCSSFromEntry(entry)).to.equal('.used{color:red}')
+    })
+
+    it('rebuilds a parseable stylesheet with its at-rule context', () => {
+      const text = '@layer utilities{.used{color:red}}'
+      const entry = {url: 'https://example.com/ok.css', text, ranges: [rangeOf(text, '.used{color:red}')]}
+
+      expect(rebuildCoveredCSSFromEntry(entry)).to.equal(text)
+    })
+  })
+})