mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-16 08:06:24 +03:00
Detect side-tab borders built with CSS variables
jsdom's CSSOM silently drops any border shorthand containing var(), leaving the computed style empty — which hid the canonical real-world side-tab pattern (border-left: Npx solid var(--brand)) from the Node detector path. Real browsers resolve var() natively, so this only affected the jsdom path. Add a pre-pass that walks the stylesheets, reads border shorthands off rule.style (jsdom preserves them there even when it drops them from cssText), resolves var() against :root custom properties via the documentElement's computed style, and attaches the result to a per- element override map. checkElementBorders consults the map whenever jsdom returned an empty width, or substitutes a resolved color when jsdom kept a literal var() string. Hex and named colors are normalized to rgb() so isNeutralColor can classify them correctly — without that, --line:#e5e7eb slipped through as non-neutral. Adds four flag cases and three pass cases to modern-color-borders.html covering shorthand, mixed neutral+colored, border-right, card-shaped label, neutral-resolving var, thin var, and uniform all-sides var. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Paul Bakaus
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
648eb036ea
commit
6e12f215c8
@@ -1269,12 +1269,25 @@ function checkElementQuality(el, style, tag, window) {
|
||||
return checkQuality({ el, tag, style, hasDirectText, textLen, fontSize, lineHeightPx, letterSpacingPx, rect: null });
|
||||
}
|
||||
|
||||
function checkElementBorders(tag, style) {
|
||||
function checkElementBorders(tag, style, overrides) {
|
||||
const sides = ['Top', 'Right', 'Bottom', 'Left'];
|
||||
const widths = {}, colors = {};
|
||||
for (const s of sides) {
|
||||
widths[s] = parseFloat(style[`border${s}Width`]) || 0;
|
||||
colors[s] = style[`border${s}Color`] || '';
|
||||
// jsdom silently drops any border shorthand containing var(), leaving
|
||||
// both width and color empty on the computed style. When the detectHtml
|
||||
// pre-pass pulled a resolved value off the rule, use it to fill in the
|
||||
// missing side so the side-tab check can run. Real browsers resolve
|
||||
// var() natively, so this fallback is a no-op in the browser path.
|
||||
if (widths[s] === 0 && overrides && overrides[s]) {
|
||||
widths[s] = overrides[s].width;
|
||||
colors[s] = overrides[s].color;
|
||||
} else if (colors[s] && colors[s].startsWith('var(') && overrides && overrides[s]) {
|
||||
// Longhand case: jsdom kept the width but left the color as the
|
||||
// literal `var(...)` string. Substitute the resolved color.
|
||||
colors[s] = overrides[s].color;
|
||||
}
|
||||
}
|
||||
return checkBorders(tag, widths, colors, parseFloat(style.borderRadius) || 0);
|
||||
}
|
||||
|
||||
+192
-2
@@ -1264,12 +1264,25 @@ function checkElementQuality(el, style, tag, window) {
|
||||
return checkQuality({ el, tag, style, hasDirectText, textLen, fontSize, lineHeightPx, letterSpacingPx, rect: null });
|
||||
}
|
||||
|
||||
function checkElementBorders(tag, style) {
|
||||
function checkElementBorders(tag, style, overrides) {
|
||||
const sides = ['Top', 'Right', 'Bottom', 'Left'];
|
||||
const widths = {}, colors = {};
|
||||
for (const s of sides) {
|
||||
widths[s] = parseFloat(style[`border${s}Width`]) || 0;
|
||||
colors[s] = style[`border${s}Color`] || '';
|
||||
// jsdom silently drops any border shorthand containing var(), leaving
|
||||
// both width and color empty on the computed style. When the detectHtml
|
||||
// pre-pass pulled a resolved value off the rule, use it to fill in the
|
||||
// missing side so the side-tab check can run. Real browsers resolve
|
||||
// var() natively, so this fallback is a no-op in the browser path.
|
||||
if (widths[s] === 0 && overrides && overrides[s]) {
|
||||
widths[s] = overrides[s].width;
|
||||
colors[s] = overrides[s].color;
|
||||
} else if (colors[s] && colors[s].startsWith('var(') && overrides && overrides[s]) {
|
||||
// Longhand case: jsdom kept the width but left the color as the
|
||||
// literal `var(...)` string. Substitute the resolved color.
|
||||
colors[s] = overrides[s].color;
|
||||
}
|
||||
}
|
||||
return checkBorders(tag, widths, colors, parseFloat(style.borderRadius) || 0);
|
||||
}
|
||||
@@ -2393,6 +2406,178 @@ function isFullPage(content) {
|
||||
return /<!doctype\s|<html[\s>]|<head[\s>]/i.test(stripped);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// jsdom CSS-variable border override map
|
||||
// ---------------------------------------------------------------------------
|
||||
//
|
||||
// jsdom's CSSOM silently drops any border shorthand that contains a var()
|
||||
// reference — the computed style for the element then shows empty width,
|
||||
// empty style, and a default black color. That's enough to hide the most
|
||||
// common real-world side-tab pattern in AI-generated pages:
|
||||
//
|
||||
// :root { --brand: #87a8ff; }
|
||||
// .card { border-left: 5px solid var(--brand); border-radius: 4px; }
|
||||
//
|
||||
// Real browsers (and therefore the browser detector path) resolve var()
|
||||
// natively, so this only affects the Node jsdom path.
|
||||
//
|
||||
// This pre-pass walks the stylesheets, finds any rule whose per-side or
|
||||
// all-sides border property contains var(), resolves the var() against
|
||||
// :root-level custom properties (read from the documentElement's computed
|
||||
// style, which jsdom DOES handle correctly), and attaches the resolved
|
||||
// width+color to every element that matches the rule's selector. The
|
||||
// Node-side `checkElementBorders` adapter consumes that map as a fallback
|
||||
// whenever jsdom's computed style came back empty.
|
||||
//
|
||||
// Limitations (intentional, to keep the pass simple):
|
||||
// * Only :root-level custom properties are resolved. Scoped overrides on
|
||||
// descendants are not tracked — uncommon in practice and would require
|
||||
// a per-element cascade walk.
|
||||
// * @media / @supports wrapped rules are ignored (jsdom often mishandles
|
||||
// these anyway).
|
||||
// * The fallback only fills sides that jsdom left empty, so any rule
|
||||
// whose border parses normally still wins via the computed style.
|
||||
|
||||
const BORDER_SHORTHAND_RE = /^(\d+(?:\.\d+)?)px\s+(solid|dashed|dotted|double|groove|ridge|inset|outset)\s+(.+)$/i;
|
||||
|
||||
// isNeutralColor only understands rgba()/oklch()/lch()/lab()/hsl()/hwb().
|
||||
// CSS variables typically hold hex or named colors, so normalize those to
|
||||
// rgb() before handing the value off to the shared check. Anything we don't
|
||||
// recognise is passed through unchanged — isNeutralColor then treats it as
|
||||
// non-neutral, which is the safer default (matches the oklch-era bugfix).
|
||||
const NAMED_COLORS = {
|
||||
white: [255, 255, 255], black: [0, 0, 0], gray: [128, 128, 128],
|
||||
grey: [128, 128, 128], silver: [192, 192, 192], red: [255, 0, 0],
|
||||
green: [0, 128, 0], blue: [0, 0, 255], yellow: [255, 255, 0],
|
||||
};
|
||||
|
||||
function normalizeColorForCheck(value) {
|
||||
if (!value) return value;
|
||||
const v = value.trim();
|
||||
const hex6 = v.match(/^#([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})$/i);
|
||||
if (hex6) {
|
||||
const [r, g, b] = [parseInt(hex6[1], 16), parseInt(hex6[2], 16), parseInt(hex6[3], 16)];
|
||||
return `rgb(${r}, ${g}, ${b})`;
|
||||
}
|
||||
const hex3 = v.match(/^#([0-9a-f])([0-9a-f])([0-9a-f])$/i);
|
||||
if (hex3) {
|
||||
const [r, g, b] = [
|
||||
parseInt(hex3[1] + hex3[1], 16),
|
||||
parseInt(hex3[2] + hex3[2], 16),
|
||||
parseInt(hex3[3] + hex3[3], 16),
|
||||
];
|
||||
return `rgb(${r}, ${g}, ${b})`;
|
||||
}
|
||||
const named = NAMED_COLORS[v.toLowerCase()];
|
||||
if (named) return `rgb(${named[0]}, ${named[1]}, ${named[2]})`;
|
||||
return v;
|
||||
}
|
||||
|
||||
function buildBorderOverrideMap(document, window) {
|
||||
const map = new Map();
|
||||
const rootStyle = window.getComputedStyle(document.documentElement);
|
||||
|
||||
function resolveVar(value, depth = 0) {
|
||||
if (!value || depth > 10 || !value.includes('var(')) return value;
|
||||
return value.replace(
|
||||
/var\(\s*(--[\w-]+)\s*(?:,\s*([^)]+))?\s*\)/g,
|
||||
(_, name, fallback) => {
|
||||
const v = rootStyle.getPropertyValue(name).trim();
|
||||
if (v) return resolveVar(v, depth + 1);
|
||||
if (fallback) return resolveVar(fallback.trim(), depth + 1);
|
||||
return '';
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
function parseShorthand(text) {
|
||||
const m = text.trim().match(BORDER_SHORTHAND_RE);
|
||||
if (!m) return null;
|
||||
return { width: parseFloat(m[1]), color: normalizeColorForCheck(m[3]) };
|
||||
}
|
||||
|
||||
// Read from the per-property accessors on rule.style. jsdom preserves
|
||||
// each border-* shorthand it parsed, even when the overall cssText has
|
||||
// been truncated (e.g. a `border: 1px solid var(...)` followed by a
|
||||
// `border-left: ...` loses the first declaration but keeps the second).
|
||||
const SIDE_PROPS = [
|
||||
['borderLeft', 'Left'],
|
||||
['borderRight', 'Right'],
|
||||
['borderTop', 'Top'],
|
||||
['borderBottom', 'Bottom'],
|
||||
['borderInlineStart', 'Left'],
|
||||
['borderInlineEnd', 'Right'],
|
||||
];
|
||||
|
||||
for (const sheet of document.styleSheets) {
|
||||
let rules;
|
||||
try { rules = sheet.cssRules || []; } catch { continue; }
|
||||
for (const rule of rules) {
|
||||
// CSSStyleRule only; skip @media / @keyframes / @supports wrappers.
|
||||
if (rule.type !== 1 || !rule.style || !rule.selectorText) continue;
|
||||
|
||||
const perSide = {};
|
||||
|
||||
for (const [prop, side] of SIDE_PROPS) {
|
||||
const val = rule.style[prop];
|
||||
if (!val || !val.includes('var(')) continue;
|
||||
const parsed = parseShorthand(resolveVar(val));
|
||||
if (parsed && parsed.color) perSide[side] = parsed;
|
||||
}
|
||||
|
||||
// Uniform `border: <w> <style> var(...)` applies to every side the
|
||||
// per-side map didn't already claim.
|
||||
const borderAll = rule.style.border;
|
||||
if (borderAll && borderAll.includes('var(')) {
|
||||
const parsed = parseShorthand(resolveVar(borderAll));
|
||||
if (parsed && parsed.color) {
|
||||
for (const s of ['Top', 'Right', 'Bottom', 'Left']) {
|
||||
if (!perSide[s]) perSide[s] = parsed;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Longhand `border-*-color: var(...)` with width/style in separate
|
||||
// declarations. Rare in AI-generated pages, but cheap to cover.
|
||||
for (const [prop, side] of [
|
||||
['borderLeftColor', 'Left'],
|
||||
['borderRightColor', 'Right'],
|
||||
['borderTopColor', 'Top'],
|
||||
['borderBottomColor', 'Bottom'],
|
||||
]) {
|
||||
const val = rule.style[prop];
|
||||
if (!val || !val.includes('var(')) continue;
|
||||
const resolved = resolveVar(val).trim();
|
||||
if (!resolved) continue;
|
||||
// Width may or may not come from this rule — that's fine; the
|
||||
// adapter only substitutes the color when jsdom left it as a
|
||||
// literal var() string.
|
||||
if (!perSide[side]) perSide[side] = { width: 0, color: normalizeColorForCheck(resolved) };
|
||||
}
|
||||
|
||||
if (Object.keys(perSide).length === 0) continue;
|
||||
|
||||
let matched;
|
||||
try { matched = document.querySelectorAll(rule.selectorText); }
|
||||
catch { continue; }
|
||||
|
||||
for (const el of matched) {
|
||||
const existing = map.get(el);
|
||||
if (existing) {
|
||||
// Later rules overwrite earlier ones — approximates source-order
|
||||
// cascade for equal-specificity rules and is good enough for the
|
||||
// uncontested var()-dropped sides we're trying to recover.
|
||||
Object.assign(existing, perSide);
|
||||
} else {
|
||||
map.set(el, { ...perSide });
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return map;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// jsdom detection (default for HTML files)
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -2437,11 +2622,16 @@ async function detectHtml(filePath) {
|
||||
|
||||
const findings = [];
|
||||
|
||||
// Pre-pass: recover border declarations that jsdom dropped because they
|
||||
// contained a var() reference. The map is keyed by element and consulted
|
||||
// by the border check adapter as a fallback.
|
||||
const borderOverrides = buildBorderOverrideMap(document, window);
|
||||
|
||||
// Element-level checks (borders + colors + motion)
|
||||
for (const el of document.querySelectorAll('*')) {
|
||||
const tag = el.tagName.toLowerCase();
|
||||
const style = window.getComputedStyle(el);
|
||||
for (const f of checkElementBorders(tag, style)) {
|
||||
for (const f of checkElementBorders(tag, style, borderOverrides.get(el))) {
|
||||
findings.push(finding(f.id, filePath, f.snippet));
|
||||
}
|
||||
for (const f of checkElementColors(el, style, tag, window)) {
|
||||
|
||||
@@ -95,23 +95,29 @@ describe('detectHtml — jsdom fixtures', () => {
|
||||
// shaped labels are checked while plain inline form labels still pass.
|
||||
const f = await detectHtml(path.join(FIXTURES, 'modern-color-borders.html'));
|
||||
const sideTabs = f.filter(r => r.antipattern === 'side-tab');
|
||||
// Eight FLAG cases: oklch x3, oklab, lch, lab — all colored border-left
|
||||
// Twelve FLAG cases: oklch x3, oklab, lch, lab — all colored border-left
|
||||
// with a non-zero border-radius — plus two card-shaped <label> cases
|
||||
// (one oklch, one rgb). Each must produce exactly one side-tab.
|
||||
// (one oklch, one rgb), plus four var()-based cases (shorthand, mixed
|
||||
// neutral+colored, border-right, and a card-shaped <label>). Each must
|
||||
// produce exactly one side-tab.
|
||||
assert.equal(
|
||||
sideTabs.length, 8,
|
||||
`expected 8 side-tab findings from the FLAG column, got ${sideTabs.length}: ${sideTabs.map(r => r.snippet).join('; ')}`
|
||||
sideTabs.length, 12,
|
||||
`expected 12 side-tab findings from the FLAG column, got ${sideTabs.length}: ${sideTabs.map(r => r.snippet).join('; ')}`
|
||||
);
|
||||
// Every finding must be a border-left (never right/top/bottom) since
|
||||
// that's the only side the fixture decorates.
|
||||
for (const r of sideTabs) {
|
||||
assert.match(r.snippet || '', /border-left:/, `expected border-left, got ${r.snippet}`);
|
||||
}
|
||||
// Eleven findings must be border-left; exactly one is border-right
|
||||
// (the #flag-var-right case). The fixture doesn't decorate top/bottom
|
||||
// on any flag element.
|
||||
const leftFindings = sideTabs.filter(r => /border-left/.test(r.snippet || ''));
|
||||
const rightFindings = sideTabs.filter(r => /border-right/.test(r.snippet || ''));
|
||||
assert.equal(leftFindings.length, 11, `expected 11 border-left findings, got ${leftFindings.length}`);
|
||||
assert.equal(rightFindings.length, 1, `expected 1 border-right finding, got ${rightFindings.length}`);
|
||||
// PASS column must contribute zero border findings of either flavor.
|
||||
// There are 10 pass cases: 6 structural neutrals plus 4 labels (plain
|
||||
// There are 13 pass cases: 6 structural neutrals plus 4 labels (plain
|
||||
// inline form label, label with a neutral gray border, label in a form
|
||||
// row, and a label with a thin 1px colored left border). If any leaks
|
||||
// through, the label exception is over-broad.
|
||||
// row, and a label with a thin 1px colored left border), plus 3 var()
|
||||
// pass cases (neutral-resolving var, thin var, uniform all-sides var).
|
||||
// If any leaks through, the label exception or var() fallback is
|
||||
// over-broad.
|
||||
const borderAccent = f.filter(r => r.antipattern === 'border-accent-on-rounded');
|
||||
assert.equal(
|
||||
borderAccent.length, 0,
|
||||
|
||||
@@ -12,6 +12,19 @@
|
||||
unique number of FLAG elements (each a distinct side-tab) and verify
|
||||
the side-tab count. */
|
||||
|
||||
/* CSS custom properties — variables used by the var()-case fixtures.
|
||||
jsdom's CSSOM drops any border shorthand containing var(), which used
|
||||
to make every side-tab built with CSS variables invisible to the
|
||||
detector. These cases regression-test the fallback pass that parses
|
||||
border shorthands directly off the rule (not the computed style). */
|
||||
:root {
|
||||
--brand: #87a8ff;
|
||||
--warn: #8a6a2f;
|
||||
--terracotta: #aa674d;
|
||||
--line: #e5e7eb; /* neutral gray — must NOT trigger */
|
||||
--rule: #f3f4f6; /* near-white — must NOT trigger */
|
||||
}
|
||||
|
||||
body { font-family: system-ui, sans-serif; margin: 0; padding: 24px; background: #fafafa; }
|
||||
.grid { display: grid; grid-template-columns: 1fr 1fr; gap: 32px; max-width: 1120px; margin: 0 auto; }
|
||||
.col h2 { font-size: 14px; text-transform: uppercase; letter-spacing: 0.05em; margin: 0 0 16px; color: #475569; }
|
||||
@@ -96,6 +109,48 @@
|
||||
cursor: pointer;
|
||||
}
|
||||
|
||||
/* 9: CSS variable in shorthand — the canonical real-page pattern from
|
||||
evals (e.g. 09-physics-study: border-left:8px solid var(--brand-2)).
|
||||
jsdom silently drops this entire declaration, so the computed-style
|
||||
path used to miss it. */
|
||||
#flag-var-shorthand {
|
||||
width: 400px;
|
||||
background: #ffffff;
|
||||
border-radius: 4px;
|
||||
border-left: 5px solid var(--brand);
|
||||
}
|
||||
|
||||
/* 10: CSS variable + neutral 1px all-around border — mirrors the
|
||||
medication-card pattern with a colored accent on the left. */
|
||||
#flag-var-mixed {
|
||||
width: 400px;
|
||||
background: #ffffff;
|
||||
border: 1px solid var(--line);
|
||||
border-left: 4px solid var(--warn);
|
||||
border-radius: 4px;
|
||||
}
|
||||
|
||||
/* 11: CSS variable shorthand, right side. Ensures the fallback works
|
||||
for both inline axes, not just border-left. */
|
||||
#flag-var-right {
|
||||
width: 400px;
|
||||
background: #ffffff;
|
||||
border-radius: 4px;
|
||||
border-right: 4px solid var(--terracotta);
|
||||
}
|
||||
|
||||
/* 12: card-shaped <label> with var() side border + radius —
|
||||
the checklist-row shape, built with tokens. */
|
||||
label#flag-label-var {
|
||||
display: block;
|
||||
width: 400px;
|
||||
padding: 16px;
|
||||
background: #ffffff;
|
||||
border-radius: 6px;
|
||||
border-left: 4px solid var(--brand);
|
||||
cursor: pointer;
|
||||
}
|
||||
|
||||
/* ── PASS cases: legitimate or neutral borders, must NOT fire ── */
|
||||
|
||||
/* 1: oklch chroma ~0 — true neutral gray border should not fire */
|
||||
@@ -183,6 +238,32 @@
|
||||
border-radius: 4px;
|
||||
border-left: 1px solid #3b82f6;
|
||||
}
|
||||
|
||||
/* 11: CSS variable resolving to a neutral gray — must NOT fire,
|
||||
even though the border shorthand contains var(). */
|
||||
#pass-var-neutral {
|
||||
width: 400px;
|
||||
background: #ffffff;
|
||||
border-radius: 4px;
|
||||
border-left: 3px solid var(--line);
|
||||
}
|
||||
|
||||
/* 12: thin (1px) var-based left border — under the side-tab width
|
||||
threshold even when resolved. */
|
||||
#pass-var-thin {
|
||||
width: 400px;
|
||||
background: #ffffff;
|
||||
border-radius: 4px;
|
||||
border-left: 1px solid var(--brand);
|
||||
}
|
||||
|
||||
/* 13: var-based uniform border on all four sides. Not a side-tab. */
|
||||
#pass-var-allsides {
|
||||
width: 400px;
|
||||
background: #ffffff;
|
||||
border: 3px solid var(--brand);
|
||||
border-radius: 4px;
|
||||
}
|
||||
</style>
|
||||
</head>
|
||||
<body>
|
||||
@@ -203,6 +284,13 @@
|
||||
<input type="checkbox">
|
||||
<span>Card-shaped label with rgb side border</span>
|
||||
</label>
|
||||
<div class="case" id="flag-var-shorthand"><h3>var() shorthand</h3><p>border-left 5px + var(--brand)</p></div>
|
||||
<div class="case" id="flag-var-mixed"><h3>var() mixed</h3><p>neutral all + colored var left</p></div>
|
||||
<div class="case" id="flag-var-right"><h3>var() right side</h3><p>border-right 4px + var(--terracotta)</p></div>
|
||||
<label class="case" id="flag-label-var">
|
||||
<input type="checkbox">
|
||||
<span>Card-shaped label with var() side border</span>
|
||||
</label>
|
||||
</div>
|
||||
<div class="col" data-col="pass">
|
||||
<h2>Should pass</h2>
|
||||
@@ -228,6 +316,9 @@
|
||||
<input type="checkbox">
|
||||
<span>Label with 1px colored left border (too thin)</span>
|
||||
</label>
|
||||
<div class="case" id="pass-var-neutral"><h3>var() neutral</h3><p>--line resolves to gray</p></div>
|
||||
<div class="case" id="pass-var-thin"><h3>var() thin</h3><p>1px too thin to qualify</p></div>
|
||||
<div class="case" id="pass-var-allsides"><h3>var() all sides</h3><p>uniform, not a side-tab</p></div>
|
||||
</div>
|
||||
</div>
|
||||
<script src="/js/detect-antipatterns-browser.js"></script>
|
||||
|
||||
Reference in New Issue
Block a user