From fd67b88c5edf24e1378d3828d5edc491d7e93dc2 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 14:39:27 +0000 Subject: [PATCH] design-lint: adversarial review fixes Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/design-lint/css.ts | 32 ++++++++++++++++++++++++++++---- lib/design-lint/rules/assets.ts | 2 +- lib/design-lint/rules/motion.ts | 16 +++++++++------- lib/design-lint/static.ts | 28 ++++++++++++++-------------- 4 files changed, 52 insertions(+), 26 deletions(-) diff --git a/lib/design-lint/css.ts b/lib/design-lint/css.ts index 667649a95..5f0618749 100644 --- a/lib/design-lint/css.ts +++ b/lib/design-lint/css.ts @@ -58,13 +58,37 @@ export function lineIndex(src: string, lineOffset = 0): (pos: number) => number }; } +/** The index just past the string literal opening at i, escape-aware (`"\""` is one string); end when it never closes. One skipper for the block scanner, the body flattener and the declaration split, so an escaped quote cannot rewrite the rest of a sheet. */ +function skipString(src: string, i: number, end: number): number { + const q = src[i]; + let k = i + 1; + while (k < end && src[k] !== q) { if (src[k] === '\\') k++; k++; } + return k >= end ? end : k + 1; +} + +/** A body split on the `;` outside string literals. */ +function splitDecls(body: string): string[] { + const parts: string[] = []; + let at = 0; + for (let i = 0; i < body.length; i++) { + if (body[i] === '"' || body[i] === "'") i = skipString(body, i, body.length) - 1; + else if (body[i] === ';') { parts.push(body.slice(at, i)); at = i + 1; } + } + parts.push(body.slice(at)); + return parts; +} + /** Replace nested blocks inside [s, e) with spaces so the remaining text holds only this rule's own declarations. */ function flatBody(src: string, s: number, e: number): string { - const out = src.slice(s, e).split(''); + const body = src.slice(s, e), out = body.split(''); let depth = 0, edge = -1; for (let i = 0; i < out.length; i++) { const c = out[i]; - if (c === '{') { + if (c === '"' || c === "'") { + const j = skipString(body, i, body.length); + if (depth > 0) for (let k = i; k < j; k++) if (out[k] !== '\n') out[k] = ' '; + i = j - 1; + } else if (c === '{') { if (depth === 0) for (let k = edge + 1; k < i; k++) if (out[k] !== '\n') out[k] = ' '; depth++; out[i] = ' '; } else if (c === '}') { @@ -126,7 +150,7 @@ export function parseCss(source: string, file: string, lineOffset = 0): CssRule[ const body = flatBody(src, s, e); const decls: CssDecl[] = []; let off = 0; - for (const part of body.split(';')) { + for (const part of splitDecls(body)) { const idx = part.indexOf(':'); if (idx > 0) { const prop = part.slice(0, idx).trim().toLowerCase(); @@ -151,7 +175,7 @@ export function parseCss(source: string, file: string, lineOffset = 0): CssRule[ let depth = 1, k = blockStart; while (k < end && depth > 0) { const ch = src[k]; - if (ch === '"' || ch === "'") { const q = src.indexOf(ch, k + 1); k = q < 0 || q >= end ? end : q + 1; continue; } + if (ch === '"' || ch === "'") { k = skipString(src, k, end); continue; } if (ch === '{') depth++; else if (ch === '}') depth--; k++; diff --git a/lib/design-lint/rules/assets.ts b/lib/design-lint/rules/assets.ts index a3fb1d828..0fdb59223 100644 --- a/lib/design-lint/rules/assets.ts +++ b/lib/design-lint/rules/assets.ts @@ -95,7 +95,7 @@ function checkIconLibraries(ctx: LintContext, r: Reporter): void { // sloplint check33 (gate 33) function checkUnlabeledDecoration(ctx: LintContext, r: Reporter): void { - const labeled = (a: string): boolean => hasAttr(a, 'aria-hidden') || getAttr(a, 'aria-label') != null + const labeled = (a: string): boolean => (getAttr(a, 'aria-hidden') || '').trim().toLowerCase() === 'true' || getAttr(a, 'aria-label') != null || getAttr(a, 'aria-labelledby') != null || ['img', 'presentation', 'none'].includes((getAttr(a, 'role') || '').toLowerCase()); for (const doc of ctx.docs) { diff --git a/lib/design-lint/rules/motion.ts b/lib/design-lint/rules/motion.ts index dda1e7135..627163c6f 100644 --- a/lib/design-lint/rules/motion.ts +++ b/lib/design-lint/rules/motion.ts @@ -332,24 +332,26 @@ function controlHeight(ctx: LintContext, r: Reporter): void { const HIDE_OK = /tooltip|popover|menu|dropdown|modal|dialog|toast|overlay|backdrop|sheet|drawer|panel|:hover|:focus|:checked|:target|\[open|\[aria-(?:expanded|selected|current|pressed)|::before|::after|\.is-|\[hidden|\[aria-hidden|sr-only|visually-hidden|skip|placeholder|::marker|\[data-slot/i; const REVEALISH = /reveal|fade|animate|aos|in-view|inview|appear|enter|rise|slide|stagger|observe|lazy/i; -// sloplint startingStyleText (gate 62): every @starting-style block with the selector it sits under; a block inside one already taken is part of it, so an unclosed block costs the rest of the sheet once -function startingStyleText(css: string): string { - let out = '', taken = 0; +// sloplint startingStyleText (gate 62): the selectors every @starting-style block declares, plus the selector it sits under, as whole comma parts (a substring test let `.reveal-wrap` in one block silence `.reveal`); a block inside one already taken is part of it, so an unclosed block costs the rest of the sheet once +function startingSelectors(css: string): Set { + const out = new Set(); + const add = (text: string) => { for (const part of splitTop(text)) if (part.trim()) out.add(part.trim().replace(/\s+/g, ' ')); }; + let taken = 0; for (const m of css.matchAll(/@starting-style\s*\{/gi)) { if (m.index! < taken) continue; let depth = 1, k = m.index! + m[0].length; while (k < css.length && depth > 0) { if (css[k] === '{') depth++; else if (css[k] === '}') depth--; k++; } taken = k; - out += css.slice(m.index!, k) + ' '; + for (const s of css.slice(m.index! + m[0].length, k).matchAll(/([^{};]+)\{/g)) add(s[1]); const open = css.lastIndexOf('{', m.index! - 1), close = css.lastIndexOf('}', m.index! - 1); - if (open > close) out += css.slice(Math.max(close + 1, 0), open) + ' '; + if (open > close) add(css.slice(Math.max(close + 1, 0), open)); } return out; } // sloplint check62 (gate 62) function hiddenAtRest(ctx: LintContext, r: Reporter): void { - const starting = startingStyleText(ctx.allCss); + const starting = startingSelectors(ctx.allCss); const jsScoped = (sel: string) => /(^|[\s,])(html|body)?\.js\b|\[data-js|\.no-js|html\.has-js/i.test(sel); const animated = new Map }>>(); for (const rule of ctx.rules) if (rule.keyframes) { @@ -362,7 +364,7 @@ function hiddenAtRest(ctx: LintContext, r: Reporter): void { if (rule.keyframes || (rule.media && /\bprint\b/i.test(rule.media)) || rule.selector === '@font-face') continue; const hidden = rule.decls.find((d) => (d.prop === 'opacity' && parseFloat(resolveVars(d.value, ctx.tokens)) === 0) || (d.prop === 'visibility' && /hidden/.test(d.value))); if (!hidden) continue; - if (HIDE_OK.test(rule.selector.replace(/:not\([^)]*\)/g, '')) || jsScoped(rule.selector) || (starting && starting.includes(rule.selector))) continue; + if (HIDE_OK.test(rule.selector.replace(/:not\([^)]*\)/g, '')) || jsScoped(rule.selector) || (starting.size > 0 && splitTop(rule.selector).some((part) => starting.has(part.trim().replace(/\s+/g, ' '))))) continue; const revealish = REVEALISH.test(rule.selector) || rule.decls.some((d) => d.prop === 'transform' && /translate|scale/.test(d.value)); if (!revealish) continue; const anim = rule.decls.find((d) => d.prop === 'animation' || d.prop === 'animation-name'); diff --git a/lib/design-lint/static.ts b/lib/design-lint/static.ts index 509f00c36..7824ee2c0 100644 --- a/lib/design-lint/static.ts +++ b/lib/design-lint/static.ts @@ -83,7 +83,7 @@ export function collectFiles(paths: string[], warn: (l: string) => void = () => * relative to the lint root the page was found under. Schemes and * protocol-relative URLs are not local. */ -export function resolveRef(href: string | null, from: string, root: string | null): string | null { +export function resolveRef(href: string | null, from: string, root: string | null, bound: string | null = null): string | null { if (!href || /^(?:[a-z]+:|\/\/)/i.test(href)) return null; const ref = href.split(/[?#]/)[0]; if (!ref) return null; @@ -98,24 +98,24 @@ export function resolveRef(href: string | null, from: string, root: string | nul return inside(abs, root) ? abs : null; } const abs = path.resolve(from, ref); - const project = projectRoot(from); - return project && !inside(abs, project) ? null : abs; + const top = projectRoot(from) ?? bound; + return top && !inside(abs, top) ? null : abs; } -export function importsOf(css: string, from: string, root: string | null = null): string[] { +export function importsOf(css: string, from: string, root: string | null = null, bound: string | null = null): string[] { const out: string[] = []; for (const m of stripComments(css).matchAll(/@import\s+(?:url\(\s*)?["']?([^"')\s;]+)["']?\s*\)?/gi)) { - const ref = resolveRef(m[1], from, root); + const ref = resolveRef(m[1], from, root, bound); if (ref) out.push(ref); } return out; } -export function linksOf(doc: Doc, from: string, root: string | null = null): string[] { +export function linksOf(doc: Doc, from: string, root: string | null = null, bound: string | null = null): string[] { const out: string[] = []; for (const t of doc.tags) { if (t.close || t.name !== 'link' || !/stylesheet/i.test(getAttr(t.attrs, 'rel') || '')) continue; - const ref = resolveRef(getAttr(t.attrs, 'href'), from, root); + const ref = resolveRef(getAttr(t.attrs, 'href'), from, root, bound); if (ref) out.push(ref); } return out; @@ -130,7 +130,7 @@ interface CssRead { raw: string; rules: CssRule[]; imports: string[] } /** Stylesheets read once per run: a bundle shared by forty pages is parsed once, not forty times. */ class CssCache { private readonly files = new Map(); - constructor(readonly roots: string[], readonly warn: (l: string) => void, readonly skipped: Skipped[] = []) {} + constructor(readonly roots: string[], readonly warn: (l: string) => void, readonly skipped: Skipped[] = [], readonly bound: string | null = null) {} read(f: string): CssRead | null { if (this.files.has(f)) return this.files.get(f)!; let out: CssRead | null = null; @@ -138,7 +138,7 @@ class CssCache { const size = fs.statSync(f).size; if (size > MAX_FILE_BYTES) { const why = `${Math.round(size / 1024 / 1024)} MB, over the ${MAX_FILE_BYTES / 1024 / 1024} MB limit`; this.warn(`design-lint: skipped ${f} (${why})`); this.skipped.push({ file: f, kind: 'oversize', why }); this.files.set(f, null); return null; } const raw = fs.readFileSync(f, 'utf8'); - out = { raw, rules: parseCss(raw, f), imports: importsOf(raw, path.dirname(f), rootOf(f, this.roots)) }; + out = { raw, rules: parseCss(raw, f), imports: importsOf(raw, path.dirname(f), rootOf(f, this.roots), this.bound) }; } catch (e) { const code = (e as NodeJS.ErrnoException).code; if (code === 'ENOENT') this.warn(`design-lint: skipped ${f}, not found`); @@ -177,9 +177,9 @@ export function buildContext(key: string, cssList: string[], docList: Doc[], opt const root = rootOf(doc.file, cache.roots); for (const st of doc.styles) { try { for (const rule of parseCss(st.css, doc.file, st.lineOffset)) rules.push(rule); } catch (e) { cache.warn(`design-lint: skipped a