diff --git a/package.json b/package.json index f1e6933cb..080a0142e 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,7 @@ "dev": "bun run server/index.js", "preview": "bun run build && wrangler pages dev", "deploy": "bun run build && wrangler pages deploy build/", - "test": "bun test tests/build.test.js tests/detect-antipatterns.test.js && node --test tests/detect-antipatterns-fixtures.test.mjs", + "test": "bun test tests/build.test.js tests/detect-antipatterns.test.js && node --test tests/detect-antipatterns-fixtures.test.mjs && node --test tests/detect-antipatterns-browser.test.mjs", "prepack": "cp README.md README.repo.md && cp README.npm.md README.md", "postpack": "cp README.repo.md README.md && rm README.repo.md", "screenshot": "bun run scripts/screenshot-antipatterns.js", diff --git a/src/detect-antipatterns-browser.js b/src/detect-antipatterns-browser.js index 7e42132af..5609ba708 100644 --- a/src/detect-antipatterns-browser.js +++ b/src/detect-antipatterns-browser.js @@ -988,42 +988,80 @@ function checkElementAIPaletteDOM(el) { const QUALITY_TEXT_TAGS = new Set(['p', 'li', 'td', 'th', 'dd', 'blockquote', 'figcaption']); -function checkElementQualityDOM(el) { - const tag = el.tagName.toLowerCase(); +// Resolve a CSS font-size value to pixels by walking up the parent chain. +// Browsers resolve em/rem/% to px in getComputedStyle, but jsdom returns the +// specified value verbatim — so for the Node path we walk parents ourselves. +function resolveFontSizePx(el, win) { + const chain = []; // raw font-size strings, leaf → root + let cur = el; + while (cur && cur.nodeType === 1) { + const fs = (win ? win.getComputedStyle(cur) : getComputedStyle(cur)).fontSize; + chain.push(fs || ''); + cur = cur.parentElement; + } + // Walk root → leaf, resolving each value relative to its parent context. + let px = 16; // root default + for (let i = chain.length - 1; i >= 0; i--) { + const v = chain[i]; + if (!v || v === 'inherit') continue; + const num = parseFloat(v); + if (isNaN(num)) continue; + if (v.endsWith('px')) px = num; + else if (v.endsWith('rem')) px = num * 16; + else if (v.endsWith('em')) px = num * px; + else if (v.endsWith('%')) px = (num / 100) * px; + else px = num; // unitless — already resolved + } + return px; +} + +// Resolve a CSS length value (line-height, letter-spacing, etc.) given a +// known font-size context. Returns null for "normal" / unparseable values. +function resolveLengthPx(value, fontSizePx) { + if (!value || value === 'normal' || value === 'auto' || value === 'inherit') return null; + const num = parseFloat(value); + if (isNaN(num)) return null; + if (value.endsWith('px')) return num; + if (value.endsWith('rem')) return num * 16; + if (value.endsWith('em')) return num * fontSizePx; + if (value.endsWith('%')) return (num / 100) * fontSizePx; + // Unitless line-height = multiplier, return px equivalent + return num * fontSizePx; +} + +// Pure quality checks. Most run on computed CSS and DOM-only inputs (work in +// jsdom and the browser). Two checks (line-length, cramped-padding) gate on +// element rect dimensions, which jsdom can't compute — pass `rect: null` from +// the Node adapter to skip those. +// +// Both adapters resolve font-size, line-height and letter-spacing to pixels +// before calling this so the pure function only deals with numbers. +function checkQuality(opts) { + const { el, tag, style, hasDirectText, textLen, fontSize, lineHeightPx, letterSpacingPx, rect, lineMax = 80 } = opts; + const findings = []; // Skip browser extension injected elements const elId = el.id || ''; - if (elId.startsWith('claude-') || elId.startsWith('cic-')) return []; - const style = getComputedStyle(el); - const findings = []; + if (elId.startsWith('claude-') || elId.startsWith('cic-')) return findings; - const hasDirectText = [...el.childNodes].some(n => n.nodeType === 3 && n.textContent.trim().length > 10); - const textLen = el.textContent?.trim().length || 0; - const fontSize = parseFloat(style.fontSize) || 16; - const rect = el.getBoundingClientRect(); - - // --- Line length too long --- - // Threshold is configurable via window.__IMPECCABLE_CONFIG__.lineLengthMax (default 80) - const lineMax = (typeof window !== 'undefined' && window.__IMPECCABLE_CONFIG__?.lineLengthMax) || 80; - if (hasDirectText && QUALITY_TEXT_TAGS.has(tag) && rect.width > 0 && textLen > lineMax) { + // --- Line length too long --- (browser-only: needs rect.width) + if (rect && hasDirectText && QUALITY_TEXT_TAGS.has(tag) && rect.width > 0 && textLen > lineMax) { const charsPerLine = rect.width / (fontSize * 0.5); if (charsPerLine > lineMax + 5) { findings.push({ id: 'line-length', snippet: `~${Math.round(charsPerLine)} chars/line (aim for <${lineMax})` }); } } - // --- Cramped padding (skip small elements like labels/badges) --- - if (hasDirectText && textLen > 20 && rect.width > 100 && rect.height > 30) { + // --- Cramped padding --- (browser-only: needs rect to skip small badges/labels) + if (rect && hasDirectText && textLen > 20 && rect.width > 100 && rect.height > 30) { const borders = { top: parseFloat(style.borderTopWidth) || 0, right: parseFloat(style.borderRightWidth) || 0, bottom: parseFloat(style.borderBottomWidth) || 0, left: parseFloat(style.borderLeftWidth) || 0, }; - // Need at least 2 borders (a container), or a non-transparent background const borderCount = Object.values(borders).filter(w => w > 0).length; const hasBg = style.backgroundColor && style.backgroundColor !== 'rgba(0, 0, 0, 0)'; if (borderCount >= 2 || hasBg) { - // Only check padding on sides that have borders or where bg creates containment const paddings = []; if (hasBg || borders.top > 0) paddings.push(parseFloat(style.paddingTop) || 0); if (hasBg || borders.right > 0) paddings.push(parseFloat(style.paddingRight) || 0); @@ -1040,16 +1078,14 @@ function checkElementQualityDOM(el) { // --- Tight line height --- if (hasDirectText && textLen > 50 && !['h1','h2','h3','h4','h5','h6'].includes(tag)) { - const lineHeight = parseFloat(style.lineHeight); - if (lineHeight && lineHeight !== NaN) { - const ratio = lineHeight / fontSize; - if (ratio < 1.3 && ratio > 0) { + if (lineHeightPx != null && fontSize > 0) { + const ratio = lineHeightPx / fontSize; + if (ratio > 0 && ratio < 1.3) { findings.push({ id: 'tight-leading', snippet: `line-height ${ratio.toFixed(2)}x (need >=1.3)` }); } } } - // --- Justified text (without hyphens) --- if (hasDirectText && style.textAlign === 'justify') { const hyphens = style.hyphens || style.webkitHyphens || ''; @@ -1062,7 +1098,7 @@ function checkElementQualityDOM(el) { // Only flag actual body content, not UI labels (buttons, tabs, badges, captions, footer text, etc.) if (hasDirectText && textLen > 20 && fontSize < 12) { const skipTags = ['sub', 'sup', 'code', 'kbd', 'samp', 'var', 'caption', 'figcaption']; - const inUIContext = el.closest('button, a, label, summary, [role="button"], [role="link"], [role="tab"], [role="menuitem"], [role="option"], nav, footer, [class*="badge" i], [class*="chip" i], [class*="pill" i], [class*="tag" i], [class*="label" i], [class*="caption" i]'); + const inUIContext = el.closest && el.closest('button, a, label, summary, [role="button"], [role="link"], [role="tab"], [role="menuitem"], [role="option"], nav, footer, [class*="badge" i], [class*="chip" i], [class*="pill" i], [class*="tag" i], [class*="label" i], [class*="caption" i]'); const isUppercase = style.textTransform === 'uppercase'; if (!skipTags.includes(tag) && !inUIContext && !isUppercase) { findings.push({ id: 'tiny-text', snippet: `${fontSize}px body text` }); @@ -1077,10 +1113,9 @@ function checkElementQualityDOM(el) { } // --- Wide letter spacing on body text --- - if (hasDirectText && textLen > 20) { - const tracking = parseFloat(style.letterSpacing); - if (tracking > 0 && style.textTransform !== 'uppercase') { - const trackingEm = tracking / fontSize; + if (hasDirectText && textLen > 20 && style.textTransform !== 'uppercase') { + if (letterSpacingPx != null && letterSpacingPx > 0 && fontSize > 0) { + const trackingEm = letterSpacingPx / fontSize; if (trackingEm > 0.05) { findings.push({ id: 'wide-tracking', snippet: `letter-spacing: ${trackingEm.toFixed(2)}em on body text` }); } @@ -1090,25 +1125,57 @@ function checkElementQualityDOM(el) { return findings; } -function checkPageQualityDOM() { - const findings = []; +function checkElementQualityDOM(el) { + const tag = el.tagName.toLowerCase(); + const style = getComputedStyle(el); + const hasDirectText = [...el.childNodes].some(n => n.nodeType === 3 && n.textContent.trim().length > 10); + const textLen = el.textContent?.trim().length || 0; + // Browser getComputedStyle resolves everything to px — direct parseFloat + // works. + const fontSize = parseFloat(style.fontSize) || 16; + const lineHeightPx = resolveLengthPx(style.lineHeight, fontSize); + const letterSpacingPx = resolveLengthPx(style.letterSpacing, fontSize); + const rect = el.getBoundingClientRect(); + const lineMax = (typeof window !== 'undefined' && window.__IMPECCABLE_CONFIG__?.lineLengthMax) || 80; + return checkQuality({ el, tag, style, hasDirectText, textLen, fontSize, lineHeightPx, letterSpacingPx, rect, lineMax }); +} - // --- Skipped heading levels --- - const headings = document.querySelectorAll('h1, h2, h3, h4, h5, h6'); +// Pure page-level skipped-heading walk. Takes a Document so it works in both +// the browser and jsdom. +function checkPageQualityFromDoc(doc) { + const findings = []; + const headings = doc.querySelectorAll('h1, h2, h3, h4, h5, h6'); let prevLevel = 0; for (const h of headings) { const level = parseInt(h.tagName[1]); if (prevLevel > 0 && level > prevLevel + 1) { - findings.push({ type: 'skipped-heading', detail: `h${prevLevel} followed by h${level} (missing h${prevLevel + 1})` }); + findings.push({ id: 'skipped-heading', snippet: `h${prevLevel} followed by h${level} (missing h${prevLevel + 1})` }); } prevLevel = level; } - return findings; } +// Browser adapter (returns the legacy { type, detail } shape used by the overlay loop) +function checkPageQualityDOM() { + return checkPageQualityFromDoc(document).map(f => ({ type: f.id, detail: f.snippet })); +} + // Node adapters — take pre-extracted jsdom computed style +// jsdom doesn't lay out OR resolve em/rem/% to px — so we pre-resolve every +// CSS length the rule needs ourselves (walking the parent chain for +// font-size inheritance), and pass `rect: null` to skip the two rules that +// genuinely need element rects (line-length, cramped-padding). +function checkElementQuality(el, style, tag, window) { + const hasDirectText = [...el.childNodes].some(n => n.nodeType === 3 && n.textContent.trim().length > 10); + const textLen = el.textContent?.trim().length || 0; + const fontSize = resolveFontSizePx(el, window); + const lineHeightPx = resolveLengthPx(style.lineHeight, fontSize); + const letterSpacingPx = resolveLengthPx(style.letterSpacing, fontSize); + return checkQuality({ el, tag, style, hasDirectText, textLen, fontSize, lineHeightPx, letterSpacingPx, rect: null }); +} + function checkElementBorders(tag, style) { const sides = ['Top', 'Right', 'Bottom', 'Left']; const widths = {}, colors = {}; @@ -1926,23 +1993,79 @@ if (IS_BROWSER) { overlays.push(banner); }; + // Heuristic for skipping CSS-in-JS hashed class names like "css-1a2b3c" or "_2x4hG_". + // These change between builds and produce brittle, ugly selectors. + function isLikelyHashedClass(c) { + if (!c) return true; + if (/^(css|sc|emotion|jsx|module)-[\w-]{4,}$/i.test(c)) return true; + if (/^_[\w-]{5,}$/.test(c)) return true; + if (/^[a-z0-9]{6,}$/i.test(c) && /\d/.test(c)) return true; + return false; + } + + function buildSelectorSegment(el) { + const tag = el.tagName.toLowerCase(); + let sel = tag; + + if (el.classList && el.classList.length > 0) { + const classes = [...el.classList] + .filter(c => !c.startsWith('impeccable-') && !isLikelyHashedClass(c)) + .slice(0, 2); + if (classes.length > 0) { + sel += '.' + classes.map(c => CSS.escape(c)).join('.'); + } + } + + // Disambiguate among siblings only if the parent has multiple matches + const parent = el.parentElement; + if (parent) { + try { + const matching = parent.querySelectorAll(':scope > ' + sel); + if (matching.length > 1) { + const sameType = [...parent.children].filter(c => c.tagName === el.tagName); + const idx = sameType.indexOf(el) + 1; + sel += `:nth-of-type(${idx})`; + } + } catch { + const idx = [...parent.children].indexOf(el) + 1; + sel = `${tag}:nth-child(${idx})`; + } + } + return sel; + } + function generateSelector(el) { if (el === document.body) return 'body'; if (el === document.documentElement) return 'html'; if (el.id) return '#' + CSS.escape(el.id); + const parts = []; let current = el; - while (current && current !== document.body) { - let sel = current.tagName.toLowerCase(); - if (current.id) { parts.unshift('#' + CSS.escape(current.id)); break; } - const siblings = current.parentElement?.children; - if (siblings && siblings.length > 1) { - const index = [...siblings].indexOf(current) + 1; - sel += ':nth-child(' + index + ')'; + let depth = 0; + const MAX_DEPTH = 10; + + while (current && current !== document.body && current !== document.documentElement && depth < MAX_DEPTH) { + parts.unshift(buildSelectorSegment(current)); + + // Anchor on an ancestor's ID and stop walking up + if (current.id) { + parts[0] = '#' + CSS.escape(current.id); + break; } - parts.unshift(sel); + + // Stop as soon as the partial selector uniquely identifies the target + const trySelector = parts.join(' > '); + try { + const matches = document.querySelectorAll(trySelector); + if (matches.length === 1 && matches[0] === el) { + return trySelector; + } + } catch { /* invalid selector — keep walking */ } + current = current.parentElement; + depth++; } + return parts.join(' > '); } diff --git a/src/detect-antipatterns.mjs b/src/detect-antipatterns.mjs index 103752d83..36d6682b5 100644 --- a/src/detect-antipatterns.mjs +++ b/src/detect-antipatterns.mjs @@ -983,42 +983,80 @@ function checkElementAIPaletteDOM(el) { const QUALITY_TEXT_TAGS = new Set(['p', 'li', 'td', 'th', 'dd', 'blockquote', 'figcaption']); -function checkElementQualityDOM(el) { - const tag = el.tagName.toLowerCase(); +// Resolve a CSS font-size value to pixels by walking up the parent chain. +// Browsers resolve em/rem/% to px in getComputedStyle, but jsdom returns the +// specified value verbatim — so for the Node path we walk parents ourselves. +function resolveFontSizePx(el, win) { + const chain = []; // raw font-size strings, leaf → root + let cur = el; + while (cur && cur.nodeType === 1) { + const fs = (win ? win.getComputedStyle(cur) : getComputedStyle(cur)).fontSize; + chain.push(fs || ''); + cur = cur.parentElement; + } + // Walk root → leaf, resolving each value relative to its parent context. + let px = 16; // root default + for (let i = chain.length - 1; i >= 0; i--) { + const v = chain[i]; + if (!v || v === 'inherit') continue; + const num = parseFloat(v); + if (isNaN(num)) continue; + if (v.endsWith('px')) px = num; + else if (v.endsWith('rem')) px = num * 16; + else if (v.endsWith('em')) px = num * px; + else if (v.endsWith('%')) px = (num / 100) * px; + else px = num; // unitless — already resolved + } + return px; +} + +// Resolve a CSS length value (line-height, letter-spacing, etc.) given a +// known font-size context. Returns null for "normal" / unparseable values. +function resolveLengthPx(value, fontSizePx) { + if (!value || value === 'normal' || value === 'auto' || value === 'inherit') return null; + const num = parseFloat(value); + if (isNaN(num)) return null; + if (value.endsWith('px')) return num; + if (value.endsWith('rem')) return num * 16; + if (value.endsWith('em')) return num * fontSizePx; + if (value.endsWith('%')) return (num / 100) * fontSizePx; + // Unitless line-height = multiplier, return px equivalent + return num * fontSizePx; +} + +// Pure quality checks. Most run on computed CSS and DOM-only inputs (work in +// jsdom and the browser). Two checks (line-length, cramped-padding) gate on +// element rect dimensions, which jsdom can't compute — pass `rect: null` from +// the Node adapter to skip those. +// +// Both adapters resolve font-size, line-height and letter-spacing to pixels +// before calling this so the pure function only deals with numbers. +function checkQuality(opts) { + const { el, tag, style, hasDirectText, textLen, fontSize, lineHeightPx, letterSpacingPx, rect, lineMax = 80 } = opts; + const findings = []; // Skip browser extension injected elements const elId = el.id || ''; - if (elId.startsWith('claude-') || elId.startsWith('cic-')) return []; - const style = getComputedStyle(el); - const findings = []; + if (elId.startsWith('claude-') || elId.startsWith('cic-')) return findings; - const hasDirectText = [...el.childNodes].some(n => n.nodeType === 3 && n.textContent.trim().length > 10); - const textLen = el.textContent?.trim().length || 0; - const fontSize = parseFloat(style.fontSize) || 16; - const rect = el.getBoundingClientRect(); - - // --- Line length too long --- - // Threshold is configurable via window.__IMPECCABLE_CONFIG__.lineLengthMax (default 80) - const lineMax = (typeof window !== 'undefined' && window.__IMPECCABLE_CONFIG__?.lineLengthMax) || 80; - if (hasDirectText && QUALITY_TEXT_TAGS.has(tag) && rect.width > 0 && textLen > lineMax) { + // --- Line length too long --- (browser-only: needs rect.width) + if (rect && hasDirectText && QUALITY_TEXT_TAGS.has(tag) && rect.width > 0 && textLen > lineMax) { const charsPerLine = rect.width / (fontSize * 0.5); if (charsPerLine > lineMax + 5) { findings.push({ id: 'line-length', snippet: `~${Math.round(charsPerLine)} chars/line (aim for <${lineMax})` }); } } - // --- Cramped padding (skip small elements like labels/badges) --- - if (hasDirectText && textLen > 20 && rect.width > 100 && rect.height > 30) { + // --- Cramped padding --- (browser-only: needs rect to skip small badges/labels) + if (rect && hasDirectText && textLen > 20 && rect.width > 100 && rect.height > 30) { const borders = { top: parseFloat(style.borderTopWidth) || 0, right: parseFloat(style.borderRightWidth) || 0, bottom: parseFloat(style.borderBottomWidth) || 0, left: parseFloat(style.borderLeftWidth) || 0, }; - // Need at least 2 borders (a container), or a non-transparent background const borderCount = Object.values(borders).filter(w => w > 0).length; const hasBg = style.backgroundColor && style.backgroundColor !== 'rgba(0, 0, 0, 0)'; if (borderCount >= 2 || hasBg) { - // Only check padding on sides that have borders or where bg creates containment const paddings = []; if (hasBg || borders.top > 0) paddings.push(parseFloat(style.paddingTop) || 0); if (hasBg || borders.right > 0) paddings.push(parseFloat(style.paddingRight) || 0); @@ -1035,16 +1073,14 @@ function checkElementQualityDOM(el) { // --- Tight line height --- if (hasDirectText && textLen > 50 && !['h1','h2','h3','h4','h5','h6'].includes(tag)) { - const lineHeight = parseFloat(style.lineHeight); - if (lineHeight && lineHeight !== NaN) { - const ratio = lineHeight / fontSize; - if (ratio < 1.3 && ratio > 0) { + if (lineHeightPx != null && fontSize > 0) { + const ratio = lineHeightPx / fontSize; + if (ratio > 0 && ratio < 1.3) { findings.push({ id: 'tight-leading', snippet: `line-height ${ratio.toFixed(2)}x (need >=1.3)` }); } } } - // --- Justified text (without hyphens) --- if (hasDirectText && style.textAlign === 'justify') { const hyphens = style.hyphens || style.webkitHyphens || ''; @@ -1057,7 +1093,7 @@ function checkElementQualityDOM(el) { // Only flag actual body content, not UI labels (buttons, tabs, badges, captions, footer text, etc.) if (hasDirectText && textLen > 20 && fontSize < 12) { const skipTags = ['sub', 'sup', 'code', 'kbd', 'samp', 'var', 'caption', 'figcaption']; - const inUIContext = el.closest('button, a, label, summary, [role="button"], [role="link"], [role="tab"], [role="menuitem"], [role="option"], nav, footer, [class*="badge" i], [class*="chip" i], [class*="pill" i], [class*="tag" i], [class*="label" i], [class*="caption" i]'); + const inUIContext = el.closest && el.closest('button, a, label, summary, [role="button"], [role="link"], [role="tab"], [role="menuitem"], [role="option"], nav, footer, [class*="badge" i], [class*="chip" i], [class*="pill" i], [class*="tag" i], [class*="label" i], [class*="caption" i]'); const isUppercase = style.textTransform === 'uppercase'; if (!skipTags.includes(tag) && !inUIContext && !isUppercase) { findings.push({ id: 'tiny-text', snippet: `${fontSize}px body text` }); @@ -1072,10 +1108,9 @@ function checkElementQualityDOM(el) { } // --- Wide letter spacing on body text --- - if (hasDirectText && textLen > 20) { - const tracking = parseFloat(style.letterSpacing); - if (tracking > 0 && style.textTransform !== 'uppercase') { - const trackingEm = tracking / fontSize; + if (hasDirectText && textLen > 20 && style.textTransform !== 'uppercase') { + if (letterSpacingPx != null && letterSpacingPx > 0 && fontSize > 0) { + const trackingEm = letterSpacingPx / fontSize; if (trackingEm > 0.05) { findings.push({ id: 'wide-tracking', snippet: `letter-spacing: ${trackingEm.toFixed(2)}em on body text` }); } @@ -1085,25 +1120,57 @@ function checkElementQualityDOM(el) { return findings; } -function checkPageQualityDOM() { - const findings = []; +function checkElementQualityDOM(el) { + const tag = el.tagName.toLowerCase(); + const style = getComputedStyle(el); + const hasDirectText = [...el.childNodes].some(n => n.nodeType === 3 && n.textContent.trim().length > 10); + const textLen = el.textContent?.trim().length || 0; + // Browser getComputedStyle resolves everything to px — direct parseFloat + // works. + const fontSize = parseFloat(style.fontSize) || 16; + const lineHeightPx = resolveLengthPx(style.lineHeight, fontSize); + const letterSpacingPx = resolveLengthPx(style.letterSpacing, fontSize); + const rect = el.getBoundingClientRect(); + const lineMax = (typeof window !== 'undefined' && window.__IMPECCABLE_CONFIG__?.lineLengthMax) || 80; + return checkQuality({ el, tag, style, hasDirectText, textLen, fontSize, lineHeightPx, letterSpacingPx, rect, lineMax }); +} - // --- Skipped heading levels --- - const headings = document.querySelectorAll('h1, h2, h3, h4, h5, h6'); +// Pure page-level skipped-heading walk. Takes a Document so it works in both +// the browser and jsdom. +function checkPageQualityFromDoc(doc) { + const findings = []; + const headings = doc.querySelectorAll('h1, h2, h3, h4, h5, h6'); let prevLevel = 0; for (const h of headings) { const level = parseInt(h.tagName[1]); if (prevLevel > 0 && level > prevLevel + 1) { - findings.push({ type: 'skipped-heading', detail: `h${prevLevel} followed by h${level} (missing h${prevLevel + 1})` }); + findings.push({ id: 'skipped-heading', snippet: `h${prevLevel} followed by h${level} (missing h${prevLevel + 1})` }); } prevLevel = level; } - return findings; } +// Browser adapter (returns the legacy { type, detail } shape used by the overlay loop) +function checkPageQualityDOM() { + return checkPageQualityFromDoc(document).map(f => ({ type: f.id, detail: f.snippet })); +} + // Node adapters — take pre-extracted jsdom computed style +// jsdom doesn't lay out OR resolve em/rem/% to px — so we pre-resolve every +// CSS length the rule needs ourselves (walking the parent chain for +// font-size inheritance), and pass `rect: null` to skip the two rules that +// genuinely need element rects (line-length, cramped-padding). +function checkElementQuality(el, style, tag, window) { + const hasDirectText = [...el.childNodes].some(n => n.nodeType === 3 && n.textContent.trim().length > 10); + const textLen = el.textContent?.trim().length || 0; + const fontSize = resolveFontSizePx(el, window); + const lineHeightPx = resolveLengthPx(style.lineHeight, fontSize); + const letterSpacingPx = resolveLengthPx(style.letterSpacing, fontSize); + return checkQuality({ el, tag, style, hasDirectText, textLen, fontSize, lineHeightPx, letterSpacingPx, rect: null }); +} + function checkElementBorders(tag, style) { const sides = ['Top', 'Right', 'Bottom', 'Left']; const widths = {}, colors = {}; @@ -1921,23 +1988,79 @@ if (IS_BROWSER) { overlays.push(banner); }; + // Heuristic for skipping CSS-in-JS hashed class names like "css-1a2b3c" or "_2x4hG_". + // These change between builds and produce brittle, ugly selectors. + function isLikelyHashedClass(c) { + if (!c) return true; + if (/^(css|sc|emotion|jsx|module)-[\w-]{4,}$/i.test(c)) return true; + if (/^_[\w-]{5,}$/.test(c)) return true; + if (/^[a-z0-9]{6,}$/i.test(c) && /\d/.test(c)) return true; + return false; + } + + function buildSelectorSegment(el) { + const tag = el.tagName.toLowerCase(); + let sel = tag; + + if (el.classList && el.classList.length > 0) { + const classes = [...el.classList] + .filter(c => !c.startsWith('impeccable-') && !isLikelyHashedClass(c)) + .slice(0, 2); + if (classes.length > 0) { + sel += '.' + classes.map(c => CSS.escape(c)).join('.'); + } + } + + // Disambiguate among siblings only if the parent has multiple matches + const parent = el.parentElement; + if (parent) { + try { + const matching = parent.querySelectorAll(':scope > ' + sel); + if (matching.length > 1) { + const sameType = [...parent.children].filter(c => c.tagName === el.tagName); + const idx = sameType.indexOf(el) + 1; + sel += `:nth-of-type(${idx})`; + } + } catch { + const idx = [...parent.children].indexOf(el) + 1; + sel = `${tag}:nth-child(${idx})`; + } + } + return sel; + } + function generateSelector(el) { if (el === document.body) return 'body'; if (el === document.documentElement) return 'html'; if (el.id) return '#' + CSS.escape(el.id); + const parts = []; let current = el; - while (current && current !== document.body) { - let sel = current.tagName.toLowerCase(); - if (current.id) { parts.unshift('#' + CSS.escape(current.id)); break; } - const siblings = current.parentElement?.children; - if (siblings && siblings.length > 1) { - const index = [...siblings].indexOf(current) + 1; - sel += ':nth-child(' + index + ')'; + let depth = 0; + const MAX_DEPTH = 10; + + while (current && current !== document.body && current !== document.documentElement && depth < MAX_DEPTH) { + parts.unshift(buildSelectorSegment(current)); + + // Anchor on an ancestor's ID and stop walking up + if (current.id) { + parts[0] = '#' + CSS.escape(current.id); + break; } - parts.unshift(sel); + + // Stop as soon as the partial selector uniquely identifies the target + const trySelector = parts.join(' > '); + try { + const matches = document.querySelectorAll(trySelector); + if (matches.length === 1 && matches[0] === el) { + return trySelector; + } + } catch { /* invalid selector — keep walking */ } + current = current.parentElement; + depth++; } + return parts.join(' > '); } @@ -2233,6 +2356,9 @@ async function detectHtml(filePath) { for (const f of checkElementIconTile(el, tag, window)) { findings.push(finding(f.id, filePath, f.snippet)); } + for (const f of checkElementQuality(el, style, tag, window)) { + findings.push(finding(f.id, filePath, f.snippet)); + } } // Page-level checks (only for full pages, not partials) @@ -2243,6 +2369,9 @@ async function detectHtml(filePath) { for (const f of checkPageLayout(document, window)) { findings.push(finding(f.id, filePath, f.snippet)); } + for (const f of checkPageQualityFromDoc(document)) { + findings.push(finding(f.id, filePath, f.snippet)); + } for (const f of checkHtmlPatterns(html)) { findings.push(finding(f.id, filePath, f.snippet)); } diff --git a/tests/detect-antipatterns-browser.test.mjs b/tests/detect-antipatterns-browser.test.mjs new file mode 100644 index 000000000..bfd8f3a08 --- /dev/null +++ b/tests/detect-antipatterns-browser.test.mjs @@ -0,0 +1,91 @@ +/** + * Puppeteer-backed fixture tests for browser-only detection rules. + * + * Some detection rules (cramped-padding, line-length, tight-leading, + * skipped-heading, justified-text, tiny-text, all-caps-body, wide-tracking, + * small-target) need real browser layout — they read getBoundingClientRect + * and getComputedStyle results that jsdom can't compute. Those rules can't + * be tested with the jsdom suite in detect-antipatterns-fixtures.test.mjs. + * + * This file uses detectUrl() (Puppeteer) to load fixtures in headless Chrome + * via a temporary static HTTP server, so the fixtures can use absolute + * + + diff --git a/tests/fixtures/antipatterns/quality-should-flag.html b/tests/fixtures/antipatterns/quality-should-flag.html deleted file mode 100644 index 9549429c0..000000000 --- a/tests/fixtures/antipatterns/quality-should-flag.html +++ /dev/null @@ -1,340 +0,0 @@ - - - - - - General Design Quality Issues That Should Be Flagged - - - -

Design Quality Issues: Should Flag

-

Every example on this page has a common design quality problem.

- - -

1. Line Length Too Long

-
-

This paragraph has no max-width constraint at all, which means on a wide monitor or ultrawide display, each line of text can stretch to 150 or even 200 characters wide. Research consistently shows that line lengths beyond 75 characters significantly reduce reading speed and comprehension. The eye has to travel too far to find the beginning of the next line, causing readers to lose their place. This is one of the most common and easily fixable typographic issues on the web, yet it persists because developers test on narrow browser windows and never see the problem. A simple max-width of 65ch to 75ch on the paragraph or its container would fix this entirely. But without that constraint, this text will just keep going and going across the full width of whatever viewport or container it finds itself in, making it genuinely unpleasant to read.

-
- - -

2. Cramped Padding

-
-
This text is crammed against the border with only 4px padding. It feels claustrophobic and hard to read.
-
-
-
Cramped background padding
-
-
-
Zero padding on a bordered element. The text is literally touching the border.
-
- - -

3. Tiny Body Text

-
-

This body text is only 10px. While this might be fine for a disclaimer or legal footnote, it's too small for primary content that users need to actually read. Accessibility guidelines generally recommend a minimum of 16px for body text, with 12px as an absolute floor.

-
-
-

This is 11px body text. Still too small for comfortable reading, especially on high-DPI screens where the physical size is even smaller than the pixel count suggests.

-
- - -

4. Tight Line Height

-
-

This paragraph has a line-height of 1.0, which means the lines are touching. Multi-line body text needs breathing room between lines for readability. A line-height of 1.5 to 1.7 is generally recommended for body text.

-
-
-

This paragraph uses line-height: 16px with font-size: 16px, giving an effective ratio of 1.0. Same problem expressed differently.

-
- - -

5. Justified Text

-
-

This paragraph uses text-align: justify, which forces each line to stretch to fill the full width. Without hyphenation support, this creates uneven gaps between words known as "rivers of white space" that flow vertically through the text. These rivers make the text harder to read because the inconsistent spacing disrupts the reading rhythm. Left-aligned text with a ragged right edge is almost always more readable on the web.

-
- - -

6. Missing Focus Styles

-
- - -
- - -

7. Small Touch Targets

-
- - Small link - - -
- - -

8. Skipped Heading Levels

-
-

This H3 follows the H2 above (OK)

-
But this H5 skips H4 entirely (bad for accessibility and document structure)
-

Screen readers use heading levels to build a document outline. Skipping levels breaks that navigation.

-
- - -

9. Z-Index Abuse

-
-
z-index: 99999 (why?)
-
z-index: 2147483647 (the maximum 32-bit integer)
-
- - -

10. Fixed Pixel Widths

-
-
This element has width: 800px. On any screen narrower than 800px, it will overflow and cause horizontal scrolling.
-
- - -

11. All-Caps Body Text

-
-

This entire paragraph is in uppercase via text-transform. While all-caps works for short labels, headings, or navigation items, longer body text in uppercase is significantly harder to read because we lose the word shape cues that come from ascenders and descenders in mixed-case text.

-
- - -

12. !important Overuse

-
-

This element has 5 !important declarations. It's a sign of specificity wars and unmaintainable CSS.

-
- - -

13. Inconsistent Border Radius

-
-
-
2px radius
-
8px radius
-
16px radius
-
pill
-
-
- - -

14. Wide Letter Spacing

-
-

This body text has letter-spacing: 0.15em applied to it. While subtle tracking adjustments can improve readability for headings or all-caps text, adding significant letter spacing to body text actually makes it harder to read by disrupting natural character groupings.

-
- - - - diff --git a/tests/fixtures/antipatterns/quality-should-pass.html b/tests/fixtures/antipatterns/quality-should-pass.html deleted file mode 100644 index aae801d66..000000000 --- a/tests/fixtures/antipatterns/quality-should-pass.html +++ /dev/null @@ -1,183 +0,0 @@ - - - - - - Good Design Quality Patterns That Should Pass - - - -

Design Quality: Should Pass

-

None of these should trigger quality warnings.

- -

Good Line Length

-
-

This paragraph has a max-width of 65ch, keeping the line length comfortable for reading. The eye can easily track from the end of one line to the beginning of the next.

-
- -

Short Text in Wide Container (OK)

-
-

This is a short sentence in a wide container.

-

Just a few words here.

-
- -

Good Padding

-
-
This container has 16px padding, giving the text room to breathe within its border.
-
- -

Good Text Sizes

-
-

This is 16px body text with 1.6 line-height. Comfortable to read.

-

This is a 12px caption. Small but appropriate for its purpose.

-
- -

Good Touch Targets

-
- -
- -

Proper Heading Hierarchy

-
-

This H3 follows H2 correctly

-

No skipped levels.

-
- -

Short Labels in Caps (OK)

-
- Category Label -
- -

Custom Focus Style

-
- -
- -

Consistent Border Radius

-
-
-
Card A
-
Card B
-
Card C
-
-
- -

Reasonable Z-Index

-
-
z-index: 10
-
- -

Responsive Width

-
-
max-width: 800px, width: 100%. Adapts to any screen.
-
- - - - diff --git a/tests/fixtures/antipatterns/quality.html b/tests/fixtures/antipatterns/quality.html new file mode 100644 index 000000000..5736d3cef --- /dev/null +++ b/tests/fixtures/antipatterns/quality.html @@ -0,0 +1,227 @@ + + + + + + Quality (Typography & Readability) — Should Flag vs Should Pass + + + +
+ + +
+

Should flag

+ +

Line length too long

+
+ no max-width on a paragraph +

This paragraph has no max-width constraint at all, which means on a wide monitor or ultrawide display, each line of text can stretch to 150 or even 200 characters wide. Research consistently shows that line lengths beyond 75 characters significantly reduce reading speed and comprehension. The eye has to travel too far to find the beginning of the next line, causing readers to lose their place. A simple max-width of 65ch to 75ch on the paragraph or its container would fix this entirely.

+
+ +

Tight line height

+
+ line-height: 1.0 +

This paragraph has a line-height of 1.0, which means the lines are touching. Multi-line body text needs breathing room between lines for readability. A line-height of 1.5 to 1.7 is generally recommended for body text.

+
+ +

Tiny body text

+
+ 10px body text +

This body text is only 10px. While this might be fine for a disclaimer or legal footnote, it's too small for primary content that users need to actually read. Aim for at least 14px for body content, 16px is ideal.

+
+ +

Justified text without hyphens

+
+ text-align: justify, no hyphens: auto +

This paragraph uses text-align: justify, which forces each line to stretch to fill the full width. Without hyphenation support, this creates uneven gaps between words known as "rivers of white space" that flow vertically through the text. Left-aligned text is almost always more readable on the web.

+
+ +

All-caps body text

+
+ text-transform: uppercase on a long passage +

This entire paragraph is in uppercase via text-transform. While all-caps works for short labels, headings, or navigation items, longer body text in uppercase is significantly harder to read because we lose the word shape cues that come from ascenders and descenders in mixed-case text.

+
+ +

Wide letter spacing on body text

+
+ letter-spacing: 0.15em on body text +

This body text has letter-spacing: 0.15em applied to it. While subtle tracking adjustments can improve readability for headings or all-caps text, adding significant letter spacing to body text actually makes it harder to read by disrupting natural character groupings.

+
+ +

Skipped heading levels

+
+ h1 → h3 (missing h2) +

Top heading

+

Skips straight to h3

+
+
+ + +
+

Should pass

+ +

Comfortable line length

+
+ max-width: 65ch on the paragraph +

This paragraph has a max-width of 65ch, keeping the line length comfortable for reading. The eye can easily track from the end of one line to the beginning of the next without losing its place.

+
+ +

Comfortable line height

+
+ line-height: 1.6 +

This paragraph has a line-height of 1.6, which gives multi-line text plenty of room to breathe and improves the rhythm of the page.

+
+ +

Comfortable body text size

+
+ 16px body text +

This is 16px body text — the recommended baseline for comfortable reading on modern displays.

+
+ +

Justified text with hyphens

+
+ text-align: justify + hyphens: auto +

When justified text is paired with hyphens: auto, the browser can break long words across lines, eliminating the rivers of white space and making the justification look intentional rather than awkward.

+
+ +

Short label in all-caps

+
+ short label, uppercase + Featured +
+ +

Wide tracking on a short uppercase label

+
+ letter-spacing on a short uppercase label + Beta +
+ +

Proper heading hierarchy

+
+ h1 → h2 → h3 +

Top heading

+

Second level

+

Third level

+
+
+
+ + +