From 95b28ed6d7db1390d67bf9bec98436b98bf47c17 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Wed, 2 Sep 2026 09:51:02 -0700 Subject: [PATCH] Fix linked CSS selector filtering Resolve pseudo-element selectors to live hosts, reject unresolvable linked CSS findings, and make the regression assertions independent. Also ignore comment delimiters when recovering CSS rule selectors. AI assistance disclosure: This commit was prepared with Codex under maintainer direction. --- cli/engine/browser/injected/index.mjs | 23 ++++++++++----- cli/engine/detect-antipatterns-browser.js | 28 +++++++++++++------ cli/engine/rules/checks.mjs | 5 +++- tests/detect-antipatterns-browser.test.mjs | 10 +++---- .../antipatterns/linked-url-patterns.css | 16 +++++------ .../antipatterns/linked-url-patterns.html | 2 +- 6 files changed, 53 insertions(+), 31 deletions(-) diff --git a/cli/engine/browser/injected/index.mjs b/cli/engine/browser/injected/index.mjs index de4acfd70..79777f843 100644 --- a/cli/engine/browser/injected/index.mjs +++ b/cli/engine/browser/injected/index.mjs @@ -1287,8 +1287,18 @@ if (IS_BROWSER) { const exact = Array.from(root.querySelectorAll(raw)); if (exact.length > 0) return exact; } catch { /* Dynamic/unsupported pseudos get the fallback below. */ } + + // Resolve pseudo-elements to their originating live elements. An attached + // pseudo-element (`.card::before`) belongs to the element before it, while + // a hostless pseudo-element after a combinator (`main > ::before`) belongs + // to a matching element at that position (`main > *`). Replacing every + // pseudo indiscriminately with an empty string leaves the latter as the + // invalid selector `main >` and makes absent hosts indistinguishable from + // selectors the DOM API cannot parse. const fallback = raw - .replace(/::?[a-zA-Z-]+(\([^)]*\))?/g, '') + .replace(/(^|[\s>+~,])::[a-zA-Z-]+(\([^)]*\))?/g, '$1*') + .replace(/::[a-zA-Z-]+(\([^)]*\))?/g, '') + .replace(/:[a-zA-Z-]+(\([^)]*\))?/g, '') .trim() .replace(/,\s*(?=,|$)/g, ''); if (!fallback || /^[,\s]*$/.test(fallback)) return null; @@ -1333,11 +1343,10 @@ if (IS_BROWSER) { const cssText = rule.cssText || ''; if (rule.selectorText) { const matches = selectorNodesForLiveDom(document, rule.selectorText); - // A null result means the DOM selector API cannot resolve the CSSOM - // selector (commonly a hostless pseudo-element), not that it is - // unused. Keep those valid stylesheet rules; only a definitive empty - // result proves that no live element can receive the declaration. - if (matches === null || matches.length > 0) parts.push(cssText); + // Only declarations with a resolvable live host enter the corpus. + // Unresolvable selectors are uncertain, not evidence that a pattern + // rendered, and retaining them would leak unused CSS into findings. + if (matches?.length > 0) parts.push(cssText); continue; } let nested = []; @@ -1758,7 +1767,7 @@ if (IS_BROWSER) { const scopedHtmlFindings = checkHtmlPatterns(html, corpora).filter(f => { if (!f.selector) return true; const matches = selectorNodesForLiveDom(document, f.selector); - if (!matches) return true; + if (!matches) return false; if (matches.length === 0) return false; return matches.some(el => !scopedIgnoreActive(el, f.id)); }); diff --git a/cli/engine/detect-antipatterns-browser.js b/cli/engine/detect-antipatterns-browser.js index abc720d22..3b8295832 100644 --- a/cli/engine/detect-antipatterns-browser.js +++ b/cli/engine/detect-antipatterns-browser.js @@ -1961,7 +1961,10 @@ function enclosingCssSelector(cssText, index) { // `{` belongs to some other selector. const closeBeforeIndex = cssText.lastIndexOf('}', index); if (closeBeforeIndex > open) return null; - const prevClose = Math.max(cssText.lastIndexOf('}', open - 1), cssText.lastIndexOf(';', open - 1)); + // Ignore delimiters inside comments when locating the previous declaration. + // Keeping comment length intact preserves indices into the original source. + const beforeOpen = cssText.slice(0, open).replace(/\/\*[\s\S]*?\*\//g, comment => ' '.repeat(comment.length)); + const prevClose = Math.max(beforeOpen.lastIndexOf('}'), beforeOpen.lastIndexOf(';')); const raw = cssText.slice(prevClose + 1, open).replace(/\/\*[\s\S]*?\*\//g, '').trim().replace(/\s+/g, ' '); if (!raw || raw.startsWith('@') || /^\d/.test(raw) || /[{}<]/.test(raw)) return null; // Keyframe steps: percentage steps fail the digit test above, but `from` @@ -8188,8 +8191,18 @@ if (IS_BROWSER) { const exact = Array.from(root.querySelectorAll(raw)); if (exact.length > 0) return exact; } catch { /* Dynamic/unsupported pseudos get the fallback below. */ } + + // Resolve pseudo-elements to their originating live elements. An attached + // pseudo-element (`.card::before`) belongs to the element before it, while + // a hostless pseudo-element after a combinator (`main > ::before`) belongs + // to a matching element at that position (`main > *`). Replacing every + // pseudo indiscriminately with an empty string leaves the latter as the + // invalid selector `main >` and makes absent hosts indistinguishable from + // selectors the DOM API cannot parse. const fallback = raw - .replace(/::?[a-zA-Z-]+(\([^)]*\))?/g, '') + .replace(/(^|[\s>+~,])::[a-zA-Z-]+(\([^)]*\))?/g, '$1*') + .replace(/::[a-zA-Z-]+(\([^)]*\))?/g, '') + .replace(/:[a-zA-Z-]+(\([^)]*\))?/g, '') .trim() .replace(/,\s*(?=,|$)/g, ''); if (!fallback || /^[,\s]*$/.test(fallback)) return null; @@ -8234,11 +8247,10 @@ if (IS_BROWSER) { const cssText = rule.cssText || ''; if (rule.selectorText) { const matches = selectorNodesForLiveDom(document, rule.selectorText); - // A null result means the DOM selector API cannot resolve the CSSOM - // selector (commonly a hostless pseudo-element), not that it is - // unused. Keep those valid stylesheet rules; only a definitive empty - // result proves that no live element can receive the declaration. - if (matches === null || matches.length > 0) parts.push(cssText); + // Only declarations with a resolvable live host enter the corpus. + // Unresolvable selectors are uncertain, not evidence that a pattern + // rendered, and retaining them would leak unused CSS into findings. + if (matches?.length > 0) parts.push(cssText); continue; } let nested = []; @@ -8659,7 +8671,7 @@ if (IS_BROWSER) { const scopedHtmlFindings = checkHtmlPatterns(html, corpora).filter(f => { if (!f.selector) return true; const matches = selectorNodesForLiveDom(document, f.selector); - if (!matches) return true; + if (!matches) return false; if (matches.length === 0) return false; return matches.some(el => !scopedIgnoreActive(el, f.id)); }); diff --git a/cli/engine/rules/checks.mjs b/cli/engine/rules/checks.mjs index b8cc7102f..a24631ce9 100644 --- a/cli/engine/rules/checks.mjs +++ b/cli/engine/rules/checks.mjs @@ -688,7 +688,10 @@ function enclosingCssSelector(cssText, index) { // `{` belongs to some other selector. const closeBeforeIndex = cssText.lastIndexOf('}', index); if (closeBeforeIndex > open) return null; - const prevClose = Math.max(cssText.lastIndexOf('}', open - 1), cssText.lastIndexOf(';', open - 1)); + // Ignore delimiters inside comments when locating the previous declaration. + // Keeping comment length intact preserves indices into the original source. + const beforeOpen = cssText.slice(0, open).replace(/\/\*[\s\S]*?\*\//g, comment => ' '.repeat(comment.length)); + const prevClose = Math.max(beforeOpen.lastIndexOf('}'), beforeOpen.lastIndexOf(';')); const raw = cssText.slice(prevClose + 1, open).replace(/\/\*[\s\S]*?\*\//g, '').trim().replace(/\s+/g, ' '); if (!raw || raw.startsWith('@') || /^\d/.test(raw) || /[{}<]/.test(raw)) return null; // Keyframe steps: percentage steps fail the digit test above, but `from` diff --git a/tests/detect-antipatterns-browser.test.mjs b/tests/detect-antipatterns-browser.test.mjs index 5d6fe56b3..3a54834ba 100644 --- a/tests/detect-antipatterns-browser.test.mjs +++ b/tests/detect-antipatterns-browser.test.mjs @@ -1405,16 +1405,16 @@ describe('detectUrl — browser-only fixtures', () => { }))); const linkedFindings = await linkedPage.evaluate(() => window.impeccableDetect({ serialize: true }) .flatMap(group => group.findings || [])); - const stripes = linkedFindings.filter(finding => finding.type === 'repeating-stripes-gradient'); - assert.equal(stripes.length, 1, JSON.stringify({ linkedFindings, linkedCssom })); - assert.equal(stripes[0].severity, 'advisory'); - assert.equal(stripes[0].advisory, true); + const grids = linkedFindings.filter(finding => finding.type === 'codex-grid-background'); + assert.equal(grids.length, 1, JSON.stringify({ linkedFindings, linkedCssom })); + assert.equal(grids[0].severity, 'advisory'); + assert.equal(grids[0].advisory, true); assert.equal( linkedFindings.some(finding => finding.type === 'bounce-easing'), true, JSON.stringify({ linkedFindings, linkedCssom }), ); - assert.equal(linkedFindings.some(finding => finding.type === 'codex-grid-background'), false); + assert.equal(linkedFindings.some(finding => finding.type === 'repeating-stripes-gradient'), false); assert.equal(linkedFindings.some(finding => finding.type === 'layout-transition'), false); await linkedPage.close(); diff --git a/tests/fixtures/antipatterns/linked-url-patterns.css b/tests/fixtures/antipatterns/linked-url-patterns.css index 9db4551d8..30f8e942c 100644 --- a/tests/fixtures/antipatterns/linked-url-patterns.css +++ b/tests/fixtures/antipatterns/linked-url-patterns.css @@ -1,8 +1,9 @@ @media (min-width: 1px) { - .flag-linked-stripes { + .flag-linked-grid { width: 160px; height: 80px; - background: repeating-linear-gradient(45deg, #eee, #eee 10px, #fafafa 10px, #fafafa 20px); + background: linear-gradient(90deg, #d9d9d9 1px, transparent 1px), linear-gradient(180deg, #d9d9d9 1px, transparent 1px); + background-size: 72px 72px; } } @@ -19,13 +20,10 @@ main > ::before { 50% { transform: translateY(2px); } } -/* Shipped but unused selectors must remain outside live URL findings, even - inside grouping rules or when their check does not serialize a selector. */ -@supports (display: grid) { - .unused-linked-grid { - background: linear-gradient(90deg, #d9d9d9 1px, transparent 1px), linear-gradient(180deg, #d9d9d9 1px, transparent 1px); - background-size: 72px 72px; - } +/* A pseudo-element whose originating element is absent must remain outside + live URL findings instead of being retained as an unresolvable selector. */ +.absent > ::before { + background: repeating-linear-gradient(45deg, #eee, #eee 10px, #fafafa 10px, #fafafa 20px); } /* These selectors exist in the live DOM, but their conditions are inactive. diff --git a/tests/fixtures/antipatterns/linked-url-patterns.html b/tests/fixtures/antipatterns/linked-url-patterns.html index b75beeb01..7d83662e0 100644 --- a/tests/fixtures/antipatterns/linked-url-patterns.html +++ b/tests/fixtures/antipatterns/linked-url-patterns.html @@ -8,7 +8,7 @@

Linked stylesheet pattern

-
Rendered repeating stripes
+
Rendered decorative grid
Inactive media stripes
Inactive supports stripes