diff --git a/.changeset/review-css-source-order.md b/.changeset/review-css-source-order.md new file mode 100644 index 00000000..dbb8e9cd --- /dev/null +++ b/.changeset/review-css-source-order.md @@ -0,0 +1,5 @@ +--- +"@forkpoint/agent-lighthouse-core": patch +--- + +Preserve document order between linked stylesheets and inline style blocks when resolving CSS-hidden content. diff --git a/.changeset/review-ghost-controls.md b/.changeset/review-ghost-controls.md new file mode 100644 index 00000000..f66d607c --- /dev/null +++ b/.changeset/review-ghost-controls.md @@ -0,0 +1,5 @@ +--- +"@forkpoint/agent-lighthouse-core": patch +--- + +Keep missing-role findings for decorative descendants and for inline handlers that stop propagation inside controls. An observing handler, such as an analytics call, stays part of its control. Roles resolve case-insensitively to the first recognized token, and gridcell and scrollbar wrappers count as holding a control. diff --git a/.changeset/review-inline-visibility.md b/.changeset/review-inline-visibility.md new file mode 100644 index 00000000..c64a9be7 --- /dev/null +++ b/.changeset/review-inline-visibility.md @@ -0,0 +1,5 @@ +--- +"@forkpoint/agent-lighthouse-core": patch +--- + +Resolve inline display and visibility declaration order and !important before hiding answer spans or content. Invalid values are dropped as a browser drops them, a visibility:visible descendant of a visibility:hidden block counts as rendered for text fragments, and CSS-hidden ghost content uses the same resolver for inline markers and stylesheet display rules. diff --git a/.changeset/review-lastmod-evidence.md b/.changeset/review-lastmod-evidence.md new file mode 100644 index 00000000..5aaf684d --- /dev/null +++ b/.changeset/review-lastmod-evidence.md @@ -0,0 +1,5 @@ +--- +"@forkpoint/agent-lighthouse-core": patch +--- + +Do not fail corroborated recent content edits as build stamps. Require page-date disagreement before reporting that pattern. A deploy-time Last-Modified header no longer vouches for a recent run of stamps; those URLs are unverifiable and warn. diff --git a/.changeset/review-unread-sitemap-children.md b/.changeset/review-unread-sitemap-children.md new file mode 100644 index 00000000..51cbc030 --- /dev/null +++ b/.changeset/review-unread-sitemap-children.md @@ -0,0 +1,5 @@ +--- +"@forkpoint/agent-lighthouse-core": patch +--- + +Decline discovery coverage findings when an index has unread child sitemaps, including an empty partial read and a child that already failed as a root. diff --git a/docs/evidence/audits/answer-readiness/text-fragment-addressability.md b/docs/evidence/audits/answer-readiness/text-fragment-addressability.md index 410d2684..d3905f64 100644 --- a/docs/evidence/audits/answer-readiness/text-fragment-addressability.md +++ b/docs/evidence/audits/answer-readiness/text-fragment-addressability.md @@ -122,6 +122,26 @@ flyouts or a search panel; `stripe-com-pricing` pass → fail, whose three FAQPage answers are only in `display:none` accordion panels. That last move corrects a false pass. +### Inline declaration order (2026-10-06) + +The shared visibility helper resolves duplicate inline `display` and +`visibility` declarations before deciding whether text is hidden. The last +declaration at the winning `!important` priority wins. Thus +`display:none;display:block` keeps an answer searchable, while an earlier +`display:none!important` still hides it from a later plain declaration. +This does not add stylesheet specificity or block-layout computation. + +A declaration with an invalid value is dropped, as a browser drops it, so +`display:none;display:nonee` stays hidden. `visibility` inherits and a +descendant can set it back: an answer with `visibility:visible` inside a +`visibility:hidden` block is rendered, and find-in-page reaches it. The +nearest element that declares `visibility` decides. `visibility:collapse` +hides like `hidden`. `display:none` still hides everything under it. +Each element is judged on its own chain, so a visible answer under a hidden +wrapper stays addressable. A value with `var()` is valid when parsed and +overrides an earlier declaration; its result is unknown here, so it never +counts as hiding. + ## Deferred - The headless variant — re-running the matcher against the post-JS DOM to catch diff --git a/docs/evidence/audits/content-extraction/css-hidden-ghost-content.md b/docs/evidence/audits/content-extraction/css-hidden-ghost-content.md index ba268b10..945c130b 100644 --- a/docs/evidence/audits/content-extraction/css-hidden-ghost-content.md +++ b/docs/evidence/audits/content-extraction/css-hidden-ghost-content.md @@ -112,9 +112,22 @@ never ranked against each other. A collapsed panel hidden by counted, as the mechanism requires. An override inside a media query does not cancel a base rule, so parallel desktop and mobile copies stay counted. -Source order is the gatherer's order: inline ``, + sheet(`.ghost { display: ${linked} }`), + ); + expect(result.status).toBe(status); + }, +); + describe("CssHiddenGhostContentAudit", () => { const audit = new CssHiddenGhostContentAudit(); @@ -231,6 +246,39 @@ describe("CssHiddenGhostContentAudit", () => { expect(result.status).toBe("pass"); }); + // Readability reads the resolved inline style, so an overridden or invalid + // declaration decides nothing. + it.each([ + ["display:none;display:block", "fail"], + ["display:block;display:none", "pass"], + ["display:none;display:nonee", "pass"], + ])("resolves the inline marker %s", async (style, status) => { + const result = await run( + `

${VISIBLE}

${BULK}
`, + SHEET_LINK, + sheet(".ghost { display: none }"), + ); + expect(result.status).toBe(status); + }); + + it("lets a var() display value override an earlier display:none", async () => { + const result = await run( + `

${VISIBLE}

${BULK}
`, + SHEET_LINK, + sheet(".ghost { display: none; display: var(--layout, block) }"), + ); + expect(result.status).toBe("pass"); + }); + + it("drops an invalid stylesheet display value", async () => { + const result = await run( + `

${VISIBLE}

${BULK}
`, + SHEET_LINK, + sheet(".ghost { display: none; display: nonee }"), + ); + expect(result.status).toBe("fail"); + }); + it("reports near-duplicate hidden text as duplication, not novel content", async () => { const result = await run( `

${BULK}

${BULK}
`, diff --git a/packages/core/src/audits/content-extraction/css-hidden-ghost-content.ts b/packages/core/src/audits/content-extraction/css-hidden-ghost-content.ts index bae0f70d..20735dfb 100644 --- a/packages/core/src/audits/content-extraction/css-hidden-ghost-content.ts +++ b/packages/core/src/audits/content-extraction/css-hidden-ghost-content.ts @@ -11,6 +11,11 @@ import { Audit } from "../../audit"; import { weightForGrade } from "../../scorer"; import type { CheckContext, PageContext } from "../../check-context"; import { collectPageCss, type CssRule } from "../../gatherers/css-rules"; +import { + declaredValue, + styleHidesFromReaders, + type Declaration, +} from "../../dom-visibility"; /** The repo-wide rough token estimator; no tokenizer dependency is carried. */ const CHARS_PER_TOKEN = 4; @@ -43,30 +48,6 @@ const HIDING_DECLARATIONS: ReadonlyArray<{ pattern: RegExp; label: string }> = [ }, ]; -interface DisplayDeclaration { - value: string; - important: boolean; -} - -/** - * The `display` declaration a block applies: the last one wins, unless an - * earlier one is `!important` and the later one is not. The gatherer has - * already lowercased and whitespace-collapsed the block. - */ -function blockDisplay(declarations: string): DisplayDeclaration | undefined { - let winner: DisplayDeclaration | undefined; - for (const part of declarations.split(";")) { - const colon = part.indexOf(":"); - if (colon === -1 || part.slice(0, colon).trim() !== "display") continue; - const raw = part.slice(colon + 1).trim(); - const important = /!\s*important$/.test(raw); - const value = raw.replace(/!\s*important$/, "").trim(); - if (!winner || important || !winner.important) - winner = { value, important }; - } - return winner; -} - /** A selector list split on its top-level commas, so `:is(.a, .b)` stays whole. */ function splitSelectorList(list: string): string[] { const out: string[] = []; @@ -85,9 +66,6 @@ function splitSelectorList(list: string): string[] { return out.filter(Boolean); } -/** Inline markers Readability already honours, so their text is not ingested. */ -const INLINE_HIDDEN = /display\s*:\s*none|visibility\s*:\s*hidden/i; - /** Claims that contradict visible copy when a stale hidden variant survives. */ const CONTRADICTION = /(\$|€|£|\bUSD\b|\bEUR\b|\bGBP\b)\s?\d|\b(in|out of) stock\b|\bsold out\b|\bback ?order\b|\b(19|20)\d{2}\b/i; @@ -144,7 +122,8 @@ function inlineHidden(page: PageContext, el: unknown): boolean { const $n = $el(page.$, node); if ($n.attr("hidden") !== undefined) found = true; if (($n.attr("aria-hidden") ?? "").toLowerCase() === "true") found = true; - if (INLINE_HIDDEN.test($n.attr("style") ?? "")) found = true; + // Readability's own style test, so it is not ingested either. + if (styleHidesFromReaders($n.attr("style") ?? "")) found = true; }); return found; } @@ -174,10 +153,10 @@ function hidingRules(rules: CssRule[]): HidingSelector[] { const display = new Map< string, - { selector: string; reported: string; decl: DisplayDeclaration } + { selector: string; reported: string; decl: Declaration } >(); for (const rule of live) { - const decl = blockDisplay(rule.declarations); + const decl = declaredValue(rule.declarations, "display"); if (!decl) continue; for (const selector of splitSelectorList(rule.selector)) { const key = `${rule.atRule ?? ""}\u0000${selector}`; diff --git a/packages/core/src/audits/machine-discovery/discovery-index-coverage.test.ts b/packages/core/src/audits/machine-discovery/discovery-index-coverage.test.ts index f4abeb3d..acf942a9 100644 --- a/packages/core/src/audits/machine-discovery/discovery-index-coverage.test.ts +++ b/packages/core/src/audits/machine-discovery/discovery-index-coverage.test.ts @@ -44,6 +44,53 @@ const sitemapIndex = (locs: string[]) => const page = (url: string, index = 0) => mockPageContext(url, "Page", index); +it.each([true, false])( + "declines an unread child sitemap (other entries: %s)", + async (hasEntries) => { + const ctx = mockCheckContext([page("https://example.com/")], { + "/sitemap.xml": mockFetchResult( + sitemapIndex([ + "https://example.com/products.xml", + "https://example.com/pages.xml", + ]), + 200, + "application/xml", + ), + "/products.xml": mockFetchResult( + sitemap(hasEntries ? ["https://example.com/product"] : []), + 200, + "application/xml", + ), + "/pages.xml": mockFetchResult("", 503), + }); + expect((await new DiscoveryIndexCoverageAudit().audit(ctx)).status).toBe( + "na", + ); + }, +); + +it("passes a listed page even when another child cannot be read", async () => { + const ctx = mockCheckContext([page("https://example.com/")], { + "/sitemap.xml": mockFetchResult( + sitemapIndex([ + "https://example.com/products.xml", + "https://example.com/pages.xml", + ]), + 200, + "application/xml", + ), + "/products.xml": mockFetchResult( + sitemap(["https://example.com/"]), + 200, + "application/xml", + ), + "/pages.xml": mockFetchResult("", 503), + }); + expect((await new DiscoveryIndexCoverageAudit().audit(ctx)).status).toBe( + "pass", + ); +}); + describe("DiscoveryIndexCoverageAudit", () => { const audit = new DiscoveryIndexCoverageAudit(); diff --git a/packages/core/src/audits/machine-discovery/discovery-index-coverage.ts b/packages/core/src/audits/machine-discovery/discovery-index-coverage.ts index 8ff8e8bb..059033e3 100644 --- a/packages/core/src/audits/machine-discovery/discovery-index-coverage.ts +++ b/packages/core/src/audits/machine-discovery/discovery-index-coverage.ts @@ -139,7 +139,7 @@ export class DiscoveryIndexCoverageAudit extends Audit { // The missing sitemap is sitemap-exists' (1.7) finding. Charging for it here // too levied two penalties for one missing file. - if (indexKeys.size === 0) { + if (indexKeys.size === 0 && !incomplete) { return this.warn( "No sitemap URLs or llms.txt links to compare the scanned pages against.", expected, diff --git a/packages/core/src/audits/machine-discovery/sitemap-lastmod-verifiability.test.ts b/packages/core/src/audits/machine-discovery/sitemap-lastmod-verifiability.test.ts index 9f0b5a43..4e272e41 100644 --- a/packages/core/src/audits/machine-discovery/sitemap-lastmod-verifiability.test.ts +++ b/packages/core/src/audits/machine-discovery/sitemap-lastmod-verifiability.test.ts @@ -223,9 +223,7 @@ describe("SitemapLastmodVerifiabilityAudit", () => { expect(result.status).toBe("warn"); }); - // One value on nearly every URL, stamped at deploy time, is a build date - // rather than a content date. - it("fails when one recent lastmod value covers over 90% of sampled URLs", async () => { + it("passes a shared recent lastmod when every page corroborates it", async () => { const stamp = iso(1); const urls = Array.from({ length: 10 }, (_v, i) => ({ loc: `https://example.com/d-${i}`, @@ -233,23 +231,86 @@ describe("SitemapLastmodVerifiabilityAudit", () => { dateModified: stamp, })); const result = await run(urls); - expect(result.status).toBe("fail"); - expect(result.message).toContain("build stamp"); + expect(result.status).toBe("pass"); + expect(result.message).not.toContain("build stamp"); }); // A generator that writes the clock per URL spreads one run over seconds. // Exact-string matching saw every value as distinct and missed the stamp. - it("fails when recent lastmods spread over seconds cover over 90% of sampled URLs", async () => { + it("fails a recent lastmod run that contradicts the content dates", async () => { const base = Date.now() - DAY; const urls = Array.from({ length: 10 }, (_v, i) => ({ loc: `https://example.com/s-${i}`, lastmod: new Date(base + i * 2_000).toISOString(), + dateModified: iso(100 + i), })); const result = await run(urls); expect(result.status).toBe("fail"); expect(result.message).toContain("one lastmod run"); }); + it("passes distinct recent edits within a single hour", async () => { + const base = Date.now() - DAY; + const result = await run( + Array.from({ length: 6 }, (_, i) => { + const stamp = new Date(base + i * 5 * 60_000).toISOString(); + return { + loc: `https://example.com/edit-${i}`, + lastmod: stamp, + dateModified: stamp, + }; + }), + ); + expect(result.status).toBe("pass"); + expect(result.found).toContain("6 corroborated, 0 divergent"); + }); + + it("does not infer a bad build stamp without page dates", async () => { + const stamp = iso(1); + const result = await run( + Array.from({ length: 6 }, (_, i) => ({ + loc: `https://example.com/unknown-${i}`, + lastmod: stamp, + })), + ); + expect(result.status).toBe("warn"); + expect(result.message).not.toContain("build stamp"); + }); + + // A static host stamps lastmod at build time and Last-Modified at deploy + // time. Their agreement says when the build ran, not when content changed. + it("does not let a deploy-time header vouch for a recent stamp run", async () => { + const base = Date.now() - DAY; + const result = await run( + Array.from({ length: 6 }, (_, i) => ({ + loc: `https://example.com/static-${i}`, + lastmod: new Date(base + i * 2_000).toISOString(), + lastModified: new Date(base + 5 * 60_000).toUTCString(), + })), + ); + expect(result.status).toBe("warn"); + expect(result.found).toContain( + "0 corroborated, 0 divergent, 6 unverifiable", + ); + expect(result.message).not.toContain("build stamp"); + }); + + it("still lets content dates corroborate a recent stamp run", async () => { + const base = Date.now() - DAY; + const result = await run( + Array.from({ length: 6 }, (_, i) => { + const stamp = new Date(base + i * 2_000).toISOString(); + return { + loc: `https://example.com/batch-${i}`, + lastmod: stamp, + dateModified: stamp, + lastModified: new Date(base).toUTCString(), + }; + }), + ); + expect(result.status).toBe("pass"); + }); + it("does not call lastmods hours apart a build stamp", async () => { const urls = Array.from({ length: 10 }, (_v, i) => ({ loc: `https://example.com/h-${i}`, diff --git a/packages/core/src/audits/machine-discovery/sitemap-lastmod-verifiability.ts b/packages/core/src/audits/machine-discovery/sitemap-lastmod-verifiability.ts index 1c7c8128..57bdb22c 100644 --- a/packages/core/src/audits/machine-discovery/sitemap-lastmod-verifiability.ts +++ b/packages/core/src/audits/machine-discovery/sitemap-lastmod-verifiability.ts @@ -29,7 +29,10 @@ const DAY_MS = 86_400_000; const SAMPLE_SIZE = 6; /** Clock skew allowed before a lastmod counts as future-dated. */ const FUTURE_SKEW_MS = 60 * 60 * 1000; -/** One value on this share of the sample is a stamp, not a set of content dates. */ +/** + * One run on this share of the sample has the shape of a build stamp. The + * shape alone is not a defect; it stops a deploy-time header vouching for it. + */ const MODAL_SHARE = 0.9; /** * Values this close together are one stamp. A generator that writes the @@ -190,7 +193,7 @@ export class SitemapLastmodVerifiabilityAudit extends Audit { title: "Sitemap lastmod values are verifiable against the pages", failureTitle: "Sitemap lastmod values contradict the pages they describe", description: - "Cross-validates sampled sitemap values against three independent page-level modification signals — the Last-Modified response header, JSON-LD dateModified/datePublished, and article:modified_time — and scores agreement rather than presence. Detects the two dominant failure modes: the build stamp (every URL updated on every deploy) and the frozen value (the CMS never updates it).", + "Cross-validates sampled sitemap values against three independent page-level modification signals — the Last-Modified response header, JSON-LD dateModified/datePublished, and article:modified_time — and scores agreement rather than presence. Catches the two dominant failure modes when the page dates disagree: the build stamp (every URL updated on every deploy) and the frozen value (the CMS never updates it). A deploy-time Last-Modified header never vouches for a recent run of identical stamps.", scoreDisplayMode: "ternary", weight: weightForGrade("A", "scored"), evidenceGrade: "A", @@ -206,7 +209,7 @@ export class SitemapLastmodVerifiabilityAudit extends Audit { defaultPriority: "medium", guidance: { impact: - 'Google states it uses "if it\'s consistently and verifiably (for example by comparing to the last modification of the page) accurate". lastmod is therefore a conditional signal an engine silently discards on divergence — and it is the only freshness hint a pull-based AI crawler gets from a sitemap. If sampled values disagree with every available page-level signal for a material share of URLs, the freshness channel is inert and re-crawl scheduling degrades to organic rediscovery. Two specific pathologies are detectable without guessing: over 90% of URLs sharing one lastmod equal to the last deploy date — a build stamp, exactly the pattern Google\'s "copyright date is not significant" rule disqualifies — and a lastmod in the future relative to the scan, which is never valid.', + 'Google states it uses "if it\'s consistently and verifiably (for example by comparing to the last modification of the page) accurate". lastmod is therefore a conditional signal an engine silently discards on divergence — and it is the only freshness hint a pull-based AI crawler gets from a sitemap. If sampled values disagree with every available page-level signal for a material share of URLs, the freshness channel is inert and re-crawl scheduling degrades to organic rediscovery. A recent cluster of lastmod values is consistent with a build stamp only when page dates also disagree. Clustered edits alone do not show a defect. A lastmod in the future relative to the scan is invalid.', fix: "Stamp lastmod from the content record, not from the build. Emit the timestamp of the last substantive edit to that document, and leave it alone when a deploy only rebuilds the page. Publish the same instant on the page — JSON-LD dateModified is the most widely read of the three signals — so the value is checkable; a lastmod nothing on the page supports is a lastmod the crawler drops. Never emit a future date, and use W3C Datetime (YYYY-MM-DD or a full RFC 3339 timestamp) for every value.", code: SAMPLE, effort: "moderate", @@ -250,6 +253,35 @@ export class SitemapLastmodVerifiabilityAudit extends Audit { if (key && !byKey.has(key)) byKey.set(key, page); } + // The modal test counts the whole sample, compared or not. It takes the + // largest group of values inside one STAMP_WINDOW_MS window. The shape + // alone proves nothing: a batch of real edits produces it too. What it + // changes is how much a deploy-time header can be trusted. + const ordered = sample + .map((entry) => ({ + lastmod: entry.lastmod, + time: Date.parse(entry.lastmod), + })) + .sort((a, b) => a.time - b.time); + let modalCount = 0; + let modalFirst = ""; + let modalLast = ""; + for (let start = 0, end = 0; end < ordered.length; end++) { + while (ordered[end]!.time - ordered[start]!.time > STAMP_WINDOW_MS) + start += 1; + if (end - start + 1 > modalCount) { + modalCount = end - start + 1; + modalFirst = ordered[start]!.lastmod; + modalLast = ordered[end]!.lastmod; + } + } + const modalRecent = + (now - Date.parse(modalLast)) / DAY_MS <= MODAL_RECENCY_DAYS; + const stampShape = + sample.length > 1 && + modalCount / sample.length > MODAL_SHARE && + modalRecent; + let corroborated = 0; let divergent = 0; let noSignal = 0; @@ -268,7 +300,12 @@ export class SitemapLastmodVerifiabilityAudit extends Audit { if (result) signals = signalsFromFetch(result); } - const all = [...signals.content, ...signals.header]; + // A recent run of lastmod values and a Last-Modified header can both be + // the deploy time. Their agreement then shows when the build ran, not + // when the content changed, so only the content dates are compared. + const all = stampShape + ? signals.content + : [...signals.content, ...signals.header]; if (all.length === 0) { noSignal += 1; continue; @@ -293,35 +330,13 @@ export class SitemapLastmodVerifiabilityAudit extends Audit { } } - // The modal test counts the whole sample, compared or not: a stamp is a - // property of the sitemap, and needs no page signal to be seen. It takes - // the largest group of values inside one STAMP_WINDOW_MS window. - const ordered = sample - .map((entry) => ({ - lastmod: entry.lastmod, - time: Date.parse(entry.lastmod), - })) - .sort((a, b) => a.time - b.time); - let modalCount = 0; - let modalFirst = ""; - let modalLast = ""; - for (let start = 0, end = 0; end < ordered.length; end++) { - while (ordered[end]!.time - ordered[start]!.time > STAMP_WINDOW_MS) - start += 1; - if (end - start + 1 > modalCount) { - modalCount = end - start + 1; - modalFirst = ordered[start]!.lastmod; - modalLast = ordered[end]!.lastmod; - } - } - const modalRecent = - (now - Date.parse(modalLast)) / DAY_MS <= MODAL_RECENCY_DAYS; - const buildStamp = - sample.length > 1 && - modalCount / sample.length > MODAL_SHARE && - modalRecent; - const compared = sample.length - noSignal; + const contentDiverges = + compared > 0 && divergent / compared > DIVERGENT_SHARE; + // Nearby edits can be real, including a batch publication. The shape + // only names the cause when the content dates disagree. + const buildStamp = stampShape && contentDiverges; + const problems: string[] = []; if (future.length > 0) { @@ -331,10 +346,10 @@ export class SitemapLastmodVerifiabilityAudit extends Audit { } if (buildStamp) { problems.push( - `${modalCount} of ${sample.length} sampled URLs (${pct(modalCount, sample.length)}%) share ${modalFirst === modalLast ? `the single lastmod ${modalFirst}` : `one lastmod run, ${modalFirst} to ${modalLast}`}, within ${MODAL_RECENCY_DAYS} days of this scan — the signature of a build stamp rather than a content date`, + `${modalCount} of ${sample.length} sampled URLs (${pct(modalCount, sample.length)}%) share ${modalFirst === modalLast ? `the single lastmod ${modalFirst}` : `one lastmod run, ${modalFirst} to ${modalLast}`}, within ${MODAL_RECENCY_DAYS} days of this scan; disagreement with the page dates makes this consistent with a build stamp`, ); } - if (compared > 0 && divergent / compared > DIVERGENT_SHARE) { + if (contentDiverges) { problems.push( `${divergent} of ${compared} comparable URLs (${pct(divergent, compared)}%) carry a lastmod more than ${DIVERGENCE_DAYS} days from every signal the page publishes: ${worst.join("; ")}`, ); diff --git a/packages/core/src/audits/operability-safety/_agent-affordances.ts b/packages/core/src/audits/operability-safety/_agent-affordances.ts index dd500e02..73baee36 100644 --- a/packages/core/src/audits/operability-safety/_agent-affordances.ts +++ b/packages/core/src/audits/operability-safety/_agent-affordances.ts @@ -47,6 +47,22 @@ export function hasInlineHandler(el: Element): boolean { return INLINE_HANDLERS.some((handler) => Boolean(attribs[handler])); } +/** Calls that keep a click from reaching the enclosing element. */ +const STOPS_PROPAGATION = + /\b(stopPropagation|stopImmediatePropagation|cancelBubble)\b/; + +/** + * Whether one of the element's inline handlers stops the event there. Only + * then is its action independent of an enclosing control: any other handler, + * such as an analytics call, runs and lets the click reach the control. + */ +export function stopsPropagation(el: Element): boolean { + const attribs = el.attribs ?? {}; + return INLINE_HANDLERS.some((handler) => + STOPS_PROPAGATION.test(attribs[handler] ?? ""), + ); +} + /** A declaration block that sets the hand cursor, whatever else it carries. */ const CURSOR_POINTER = /(^|;)\s*cursor\s*:\s*pointer\b/; diff --git a/packages/core/src/audits/operability-safety/ghost-clickable-element-ratio.test.ts b/packages/core/src/audits/operability-safety/ghost-clickable-element-ratio.test.ts index 799bd765..c881ba58 100644 --- a/packages/core/src/audits/operability-safety/ghost-clickable-element-ratio.test.ts +++ b/packages/core/src/audits/operability-safety/ghost-clickable-element-ratio.test.ts @@ -25,6 +25,76 @@ function page(body: string, css = ""): CheckContext { const semantic = (n: number) => Array.from({ length: n }, (_v, i) => ``).join(""); +it.each(["presentation", "none", "img", "status"])( + "does not treat a nested %s role as a control", + async (role) => { + const result = await new GhostClickableElementRatioAudit().audit( + page( + `${semantic(3)}
Buy now
`, + ), + ); + expect(result.details?.["ghostCount"]).toBe(1); + }, +); + +it("keeps an inner element's independent inline action", async () => { + const result = await new GhostClickableElementRatioAudit().audit( + page( + '', + ), + ); + expect(result.details?.["ghostCount"]).toBe(1); +}); + +it.each([ + 'Mug', + '', +])("keeps an observing inline handler inside its control: %s", async (html) => { + const result = await new GhostClickableElementRatioAudit().audit(page(html)); + expect(result.details?.["ghostCount"]).toBe(0); +}); + +it.each([ + 'role="Button"', + 'role="link button"', + 'role="gridcell"', + 'role="scrollbar"', +])("exempts a wrapper around %s", async (role) => { + const result = await new GhostClickableElementRatioAudit().audit( + page( + `
Buy now
`, + ), + ); + expect(result.details?.["ghostCount"]).toBe(0); +}); + +it("resolves the first recognized role, not any fallback token", async () => { + const result = await new GhostClickableElementRatioAudit().audit( + page( + '
Buy now
', + ), + ); + expect(result.details?.["ghostCount"]).toBe(1); +}); + +it("skips an unknown first role token for the next one", async () => { + const result = await new GhostClickableElementRatioAudit().audit( + page( + '
Buy now
', + ), + ); + expect(result.details?.["ghostCount"]).toBe(0); +}); + +it("still exempts a wrapper around an ARIA button", async () => { + const result = await new GhostClickableElementRatioAudit().audit( + page( + '
Buy now
', + ), + ); + expect(result.details?.["ghostCount"]).toBe(0); +}); + describe("GhostClickableElementRatioAudit", () => { const audit = new GhostClickableElementRatioAudit(); diff --git a/packages/core/src/audits/operability-safety/ghost-clickable-element-ratio.ts b/packages/core/src/audits/operability-safety/ghost-clickable-element-ratio.ts index 2a77c59f..ace0c90e 100644 --- a/packages/core/src/audits/operability-safety/ghost-clickable-element-ratio.ts +++ b/packages/core/src/audits/operability-safety/ghost-clickable-element-ratio.ts @@ -20,7 +20,9 @@ import { hasClickSignal, hasInlineHandler, isElement, + stopsPropagation, } from "./_agent-affordances"; +import { ariaRoles } from "./engine/standards"; /** Below this share of addressable click targets the page fails. */ const RATIO_FLOOR = 0.9; @@ -82,9 +84,70 @@ interface Survey { crossOrigin: number; } -/** Native controls and declared roles an agent's snapshot exposes. */ -const CONTROL_DESCENDANT = - "a[href], button, input:not([type=hidden]), select, textarea, summary, [role]"; +/** + * Widget roles that are themselves an action an agent can address, matched + * against `effectiveRole`. `progressbar` is a widget role in the ARIA table + * but takes no input, so it is left out. + */ +const ACTION_ROLES = [ + "button", + "link", + "menuitem", + "menuitemcheckbox", + "menuitemradio", + "tab", + "option", + "checkbox", + "radio", + "switch", + "treeitem", +]; + +/** + * Widget roles a wrapper can hold as its control, but which do not absorb a + * click on their contents: a field takes input, and a grid cell often holds + * a control of its own. + */ +const HELD_ROLES = [ + "gridcell", + "textbox", + "searchbox", + "combobox", + "slider", + "spinbutton", + "scrollbar", +]; + +/** + * The role a user agent applies: the first token of the attribute that names + * a concrete ARIA role. `role="img button"` is an `img`; the later token is a + * fallback for agents that do not know the first. Matching is + * case-insensitive, as `declaredRole` reads it. + */ +function effectiveRole(el: Element): string | undefined { + for (const token of (el.attribs?.["role"] ?? "").toLowerCase().split(/\s+/)) { + const entry = ariaRoles[token]; + if (token && entry && entry.type !== "abstract") return token; + } + return undefined; +} + +/** Native controls an agent can address as actions. */ +const NATIVE_CONTROL = + "a[href], button, input:not([type=hidden]), select, textarea, summary"; +const HELD_CONTROL_ROLES = new Set([...ACTION_ROLES, ...HELD_ROLES]); + +/** Whether `el` is a native control or carries one of `roles`. */ +function isControl( + el: Element, + $: CheerioAPI, + native: string, + roles: Set, +): boolean { + if ($(el).is(native)) return true; + const role = effectiveRole(el); + return role !== undefined && roles.has(role); +} /** * A wrapper whose only click signal is a name or a cursor, and which holds a @@ -95,20 +158,27 @@ const CONTROL_DESCENDANT = */ function wrapsControl(el: Element, $: CheerioAPI): boolean { if (hasInlineHandler(el)) return false; - return $(el).find(CONTROL_DESCENDANT).length > 0; + return ($(el).find(`${NATIVE_CONTROL}, [role]`).toArray() as Element[]).some( + (child) => isControl(child, $, NATIVE_CONTROL, HELD_CONTROL_ROLES), + ); } /** Controls whose clicks an inner element's clicks reach and are announced as. */ -const CONTROL_ANCESTOR = - 'a[href], button, summary, [role="button"], [role="link"], [role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="tab"], [role="option"], [role="checkbox"], [role="radio"], [role="switch"], [role="treeitem"]'; +const NATIVE_ANCESTOR = "a[href], button, summary"; +const ANCESTOR_ROLES = new Set(ACTION_ROLES); /** * An icon or label inside a link or button is part of that control: a click * on it is the control's click, and the snapshot names the control. Its own - * click-signal class is a styling hook, not a second control. + * click-signal class is a styling hook, not a second control. An inline + * handler that only observes the click, such as an analytics call, leaves it + * the control's click. One that stops propagation is a second action. */ function insideControl(el: Element, $: CheerioAPI): boolean { - return $(el).parents(CONTROL_ANCESTOR).length > 0; + if (stopsPropagation(el)) return false; + return ( + $(el).parents(`${NATIVE_ANCESTOR}, [role]`).toArray() as Element[] + ).some((parent) => isControl(parent, $, NATIVE_ANCESTOR, ANCESTOR_ROLES)); } /** The role attribute, once `presentation`/`none` are treated as no role. */ diff --git a/packages/core/src/dom-visibility.test.ts b/packages/core/src/dom-visibility.test.ts index f8aefcdc..4eedc8f5 100644 --- a/packages/core/src/dom-visibility.test.ts +++ b/packages/core/src/dom-visibility.test.ts @@ -9,6 +9,31 @@ function check(html: string): boolean { return hiddenFromReaders($, el); } +it.each([ + ["display:none;display:block", false], + ["display:block;display:none", true], + ["display:none!important;display:block", true], + ["display:none!important;display:block!important", false], + ["DISPLAY: NONE; DISPLAY: block ! IMPORTANT", false], + ["visibility:hidden;visibility:visible", false], + ["visibility:hidden!important;visibility:visible", true], + ["visibility:visible!important;visibility:hidden", false], + // A browser drops an invalid declaration; the earlier valid one applies. + ["display:none;display:nonee", true], + ["display:none;display:block\\9", true], + ["display:none;display:", true], + ["visibility:hidden;visibility:hiddn", true], + ["display:none;display:inline flex", false], + // var() is valid at parse time, so it overrides; its value is unknown here. + ["display:none;display:var(--layout, block)", false], + ["visibility:hidden;visibility:var(--v)", false], +])("resolves inline declaration precedence: %s", (style, hidden) => { + const $ = cheerio.load(`

Answer text.

`); + const el = $("#t").get(0)!; + expect(notRendered($, el)).toBe(hidden); + expect(hiddenFromReaders($, el)).toBe(hidden); +}); + describe("hiddenFromReaders", () => { it("is false for ordinary visible content", () => { expect(check('

Hi

')).toBe(false); @@ -64,7 +89,35 @@ describe("notRendered", () => { // until-found stays searchable by find-in-page and text fragments. ['', false], ['

x

', false], + ['

x

', true], + // visibility inherits, and a descendant may set it back. + [ + '

x

', + false, + ], + [ + '

x

', + false, + ], + [ + '

x

', + true, + ], + // display:none takes the subtree out whatever it declares. + [ + '

x

', + true, + ], ])("%s -> %s", (html, expected) => { expect(rendered(html)).toBe(expected); }); + + it("leaves readers' view of a hidden subtree unchanged", () => { + // Readability removes the hidden node with everything under it. + expect( + check( + '

x

', + ), + ).toBe(true); + }); }); diff --git a/packages/core/src/dom-visibility.ts b/packages/core/src/dom-visibility.ts index 1d4f1476..4e4cb39b 100644 --- a/packages/core/src/dom-visibility.ts +++ b/packages/core/src/dom-visibility.ts @@ -1,8 +1,132 @@ import type { CheerioAPI } from "cheerio"; import type { AnyNode } from "domhandler"; -/** Inline declarations that take a subtree out of rendering. */ -const INLINE_HIDDEN = /(^|;)\s*(display\s*:\s*none|visibility\s*:\s*hidden)/i; +/** CSS-wide keywords every property accepts. */ +const GLOBAL_KEYWORDS = new Set([ + "inherit", + "initial", + "unset", + "revert", + "revert-layer", +]); + +/** Keywords `display` accepts, alone or in its multi-keyword form. */ +const DISPLAY_KEYWORDS = new Set([ + "none", + "contents", + "block", + "inline", + "run-in", + "flow", + "flow-root", + "table", + "flex", + "grid", + "ruby", + "math", + "list-item", + "inline-block", + "inline-table", + "inline-flex", + "inline-grid", + "table-row-group", + "table-header-group", + "table-footer-group", + "table-row", + "table-cell", + "table-column-group", + "table-column", + "table-caption", + "ruby-base", + "ruby-text", + "ruby-base-container", + "ruby-text-container", + "-webkit-box", + "-webkit-inline-box", + "-moz-box", + "-moz-inline-box", + "-ms-flexbox", + "-ms-inline-flexbox", + "-ms-grid", + "-ms-inline-grid", +]); + +const VISIBILITY_KEYWORDS = new Set(["visible", "hidden", "collapse"]); + +export type HidingProperty = "display" | "visibility"; + +/** Cheap pre-checks, so a style that names neither property is never parsed. */ +const MENTIONS: Record = { + display: /display/i, + visibility: /visibility/i, +}; + +/** + * A browser drops a declaration whose value it cannot parse, so the earlier + * valid one keeps applying. `display:none;display:nonee` stays hidden, and so + * does the `block\9` hack. jsdom, which Readability runs on, agrees. + */ +function validValue(property: HidingProperty, value: string): boolean { + if (GLOBAL_KEYWORDS.has(value)) return true; + // A value with var() is valid at parse time and resolved later, so it + // overrides an earlier declaration. Its result is unknown here; callers + // compare against `none` / `hidden` and treat it as neither. + if (/\bvar\(/.test(value)) return true; + if (property === "visibility") return VISIBILITY_KEYWORDS.has(value); + const keywords = value.split(/\s+/); + return ( + keywords.length <= 3 && keywords.every((word) => DISPLAY_KEYWORDS.has(word)) + ); +} + +export interface Declaration { + value: string; + important: boolean; +} + +/** + * The value a declaration block applies to `property`: the last valid + * declaration wins, unless an earlier one is `!important` and the later one + * is not. Invalid values are dropped, as a browser drops them. Works on an + * inline `style` attribute and on a stylesheet rule's declarations alike. + */ +export function declaredValue( + declarations: string, + property: HidingProperty, +): Declaration | undefined { + if (!MENTIONS[property].test(declarations)) return undefined; + let winner: Declaration | undefined; + for (const part of declarations.replace(/\/\*[\s\S]*?\*\//g, "").split(";")) { + const colon = part.indexOf(":"); + if (colon === -1) continue; + if (part.slice(0, colon).trim().toLowerCase() !== property) continue; + const raw = part + .slice(colon + 1) + .trim() + .toLowerCase(); + const important = /!\s*important$/.test(raw); + const value = raw + .replace(/!\s*important$/, "") + .trim() + .replace(/\s+/g, " "); + if (!validValue(property, value)) continue; + if (!winner || important || !winner.important) + winner = { value, important }; + } + return winner; +} + +/** + * Whether an inline style hides its subtree the way Readability reads it: + * `style.display == "none"` or `style.visibility == "hidden"`. Readability + * removes the node, so a `visibility:visible` descendant goes with it. + */ +export function styleHidesFromReaders(style: string): boolean { + return ( + declaredValue(style, "display")?.value === "none" || + declaredValue(style, "visibility")?.value === "hidden" + ); +} type Marker = ($n: ReturnType, tag: string) => boolean | undefined; @@ -28,20 +152,41 @@ function unrenderedMarker($n: ReturnType, tag: string): boolean { // reveal it. if (hidden !== undefined && hidden.trim().toLowerCase() !== "until-found") return true; - return INLINE_HIDDEN.test($n.attr("style") ?? ""); + return declaredValue($n.attr("style") ?? "", "display")?.value === "none"; +} + +/** + * `visibility` inherits, and a descendant can set it back to `visible`, so + * the nearest element that declares it decides. `display:none` is different: + * nothing under it renders, whatever it declares. + */ +function visibilityHidden($: CheerioAPI, el: AnyNode): boolean { + let node: AnyNode | null = el; + while (node) { + if ((node as { tagName?: string }).tagName) { + const value = declaredValue( + $(node).attr("style") ?? "", + "visibility", + )?.value; + if (value && value !== "inherit" && value !== "unset") + return value === "hidden" || value === "collapse"; + } + node = node.parent as AnyNode | null; + } + return false; } /** * Whether the served markup keeps an element from being rendered: the element * or an ancestor carries `hidden` (other than `until-found`), an inline - * `display:none` / `visibility:hidden`, is a `