From befa2e38e3f2d603e300c8dc594c38e43aa89742 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 13:37:09 -0700 Subject: [PATCH 01/13] fix bug where we call getEventListeners with empty string at the end --- core/gather/gatherers/anchor-elements.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index 15bcd4979290..1e5534b166ad 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -164,7 +164,7 @@ class AnchorElements extends BaseGatherer { const ancestorListeners = new Set(); const splitPath = anchor.node.devtoolsNodePath.split(','); const ancestorListenerPromises = []; - while (splitPath.length >= 2) { + while (splitPath.length > 2) { splitPath.length -= 2; const path = splitPath.join(','); const promise = getEventListeners(session, path).then(listeners => { From ff21eab4cfb1082b8a1b619997ca2140cdbd0be7 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 13:37:58 -0700 Subject: [PATCH 02/13] timing trace. support html files as input --- core/scripts/generate-timing-trace.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/core/scripts/generate-timing-trace.js b/core/scripts/generate-timing-trace.js index d61639f5f20f..bbe68c8471c9 100644 --- a/core/scripts/generate-timing-trace.js +++ b/core/scripts/generate-timing-trace.js @@ -43,7 +43,19 @@ function saveTraceFromCLI() { printErrorAndQuit('Lighthouse JSON results not found.'); } - const lhrObject = JSON.parse(fs.readFileSync(filename, 'utf8')); + let text = fs.readFileSync(filename, 'utf8'); + // Support reading from HTML too. + if (filename.endsWith('.html') || text.startsWith('<')) { + const startMarker = '__LIGHTHOUSE_JSON__ = '; + const start = text.indexOf(startMarker) + startMarker.length; + const end = text.indexOf(';', start); + if (start === -1 || end === -1) { + printErrorAndQuit('No Lighthouse JSON found in HTML file.'); + } + text = text.slice(start, end); + } + + const lhrObject = JSON.parse(text); const jsonStr = createTraceString(lhrObject); const pathObj = path.parse(filename); From 03978b7201c3e4cb8ec787965432b897f7cdd7ef Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 13:38:23 -0700 Subject: [PATCH 03/13] temporary instrumetation --- core/gather/session.js | 8 ++++++-- core/runner.js | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/core/gather/session.js b/core/gather/session.js index 8d9bd24ad7af..06a22c952b25 100644 --- a/core/gather/session.js +++ b/core/gather/session.js @@ -14,6 +14,10 @@ import {LighthouseError} from '../lib/lh-error.js'; const DEFAULT_PROTOCOL_TIMEOUT = 30000; const PPTR_BUFFER = 50; +let i = 0; +const calls = {}; +global.calls = calls; + /** * Puppeteer timeouts must fit into an int32 and the maximum timeout for `setTimeout` is a *signed* * int32. However, this also needs to account for the puppeteer buffer we add to the timeout later. @@ -112,7 +116,7 @@ class ProtocolSession extends CrdpEventEmitter { sendCommand(method, ...params) { const timeoutMs = this.getNextProtocolTimeout(); this._nextProtocolTimeout = undefined; - + calls[method] = (calls[method] || 0) + 1; /** @type {NodeJS.Timeout|undefined} */ let timeout; const timeoutPromise = new Promise((resolve, reject) => { @@ -127,7 +131,7 @@ class ProtocolSession extends CrdpEventEmitter { // Add 50ms to the Puppeteer timeout to ensure the Lighthouse timeout finishes first. timeout: timeoutMs + PPTR_BUFFER, }).catch((error) => { - log.formatProtocol('method <= browser ERR', {method}, 'error'); + log.formatProtocol('method <= browser ERR', {method, params}, 'error'); throw LighthouseError.fromProtocolMessage(method, error); }); const resultWithTimeoutPromise = diff --git a/core/runner.js b/core/runner.js index e1b5261bbbd2..16c4b33e6b43 100644 --- a/core/runner.js +++ b/core/runner.js @@ -133,6 +133,7 @@ class Runner { assetSaver.saveLhr(lhr, path); } + console.log(global.calls); // Create the HTML, JSON, and/or CSV string const report = ReportGenerator.generateReport(lhr, settings.output); From 6b9282ead0bab60ef137abcd498a420f5023038f Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 14:09:47 -0700 Subject: [PATCH 04/13] unique listeners only. first pass. this could be a lot better --- core/gather/gatherers/anchor-elements.js | 70 +++++++++++++++++++----- 1 file changed, 56 insertions(+), 14 deletions(-) diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index 1e5534b166ad..b0a0db1fcffc 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -155,36 +155,78 @@ class AnchorElements extends BaseGatherer { }); await session.sendCommand('DOM.enable'); - // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds if the `DOM` domain was enabled before this gatherer, invoke it to be safe. + console.time('getdoc'); + // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds + // if the `DOM` domain was enabled before this gatherer, invoke it to be safe. await session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}); - const anchorsWithEventListeners = anchors.map(async anchor => { - const listeners = await getEventListeners(session, anchor.node.devtoolsNodePath); + console.timeEnd('getdoc'); - /** @type {Set<{type: string}>} */ - const ancestorListeners = new Set(); + + console.time('listeners'); + + // Phase 1: Collect all unique node paths from anchors and their ancestors. + const allPaths = new Set(); + for (const anchor of anchors) { + allPaths.add(anchor.node.devtoolsNodePath); + const splitPath = anchor.node.devtoolsNodePath.split(','); + while (splitPath.length > 2) { + splitPath.length -= 2; + allPaths.add(splitPath.join(',')); + } + } + + // Phase 2: Fetch event listeners for all unique paths in parallel. + /** @type {Map>} */ + const listenersByPath = new Map(); + const listenerPromises = Array.from(allPaths).map(path => { + return getEventListeners(session, path) + .then(listeners => { + if (listeners.length) { + listenersByPath.set(path, listeners); + } + }) + .catch(() => {}); // Ignore errors, e.g. node not found. + }); + await Promise.all(listenerPromises); + + // Phase 3: Augment anchor data with the fetched listeners. + const result = anchors.map(anchor => { + const listeners = listenersByPath.get(anchor.node.devtoolsNodePath) || []; + + /** @type {Map} */ + const ancestorListeners = new Map(); const splitPath = anchor.node.devtoolsNodePath.split(','); - const ancestorListenerPromises = []; while (splitPath.length > 2) { splitPath.length -= 2; const path = splitPath.join(','); - const promise = getEventListeners(session, path).then(listeners => { - for (const listener of listeners) { - ancestorListeners.add(listener); + const pathListeners = listenersByPath.get(path); + if (pathListeners) { + for (const listener of pathListeners) { + ancestorListeners.set(listener.type, listener); } - }).catch(() => {}); - ancestorListenerPromises.push(promise); + } } - await Promise.all(ancestorListenerPromises); return { ...anchor, listeners, - ancestorListeners: Array.from(ancestorListeners), + ancestorListeners: Array.from(ancestorListeners.values()), }; }); - const result = await Promise.all(anchorsWithEventListeners); + console.timeEnd('listeners'); + + + console.time('getSnapshot'); + const snapshot = await session.sendCommand('DOMSnapshot.getSnapshot', { + includeEventListeners: true, + computedStyleWhitelist: [], + }); + console.log(snapshot.domNodes.length) + console.timeEnd('getSnapshot'); + + await session.sendCommand('DOM.disable'); return result; } From e47927a7aa9e8527b758897c29f99f49d23f7ba5 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 14:17:54 -0700 Subject: [PATCH 05/13] trying out a few versions getdoc: 25.291ms simpelisteners: 165.471ms listeners: 372.843ms no ancestor check basically halves it. but i think the unique thing is wayyyy larger of impact --- core/gather/gatherers/anchor-elements.js | 32 ++++++++++++++++++++---- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index b0a0db1fcffc..0fd8b132b612 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -162,6 +162,30 @@ class AnchorElements extends BaseGatherer { console.timeEnd('getdoc'); + console.time('simpelisteners'); + + + const listeners = await Promise.all(anchors.map(anchor => getEventListeners(session, anchor.node.devtoolsNodePath))); + const paralellresult = anchors.map((anchor, i) => ({ + ...anchor, + listeners: listeners[i], + })); + console.timeEnd('simpelisteners'); + + // // const anchorsWithEventListeners = anchors.map(async anchor => { + // // const listeners = getEventListeners(session, anchor.node.devtoolsNodePath); + + + // // return { + // // ...anchor, + // // listeners, + // // }; + // // }); + + // const resultsimple = await Promise.all(anchorsWithEventListeners); + + // console.timeEnd('simpelisteners'); + console.time('listeners'); // Phase 1: Collect all unique node paths from anchors and their ancestors. @@ -219,11 +243,9 @@ class AnchorElements extends BaseGatherer { console.time('getSnapshot'); - const snapshot = await session.sendCommand('DOMSnapshot.getSnapshot', { - includeEventListeners: true, - computedStyleWhitelist: [], - }); - console.log(snapshot.domNodes.length) + // It'd be faster (on complex pages like cnn.com to use `sendCommand('DOMSnapshot.getSnapshot', {includeEventListeners: true, computedStyleWhitelist: []});` + // However that'd require a large refactor of how we collect/match dom nodes. + // If this remains a performance concern we should try to get rid of the ancestor listener consideration. console.timeEnd('getSnapshot'); From 65e2fd4dc53f0d1299805a6ecf85bd85ea13c8ec Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 14:20:40 -0700 Subject: [PATCH 06/13] clean up before trying for shorter --- core/gather/gatherers/anchor-elements.js | 34 ------------------------ 1 file changed, 34 deletions(-) diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index 0fd8b132b612..f46c0194d252 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -155,37 +155,11 @@ class AnchorElements extends BaseGatherer { }); await session.sendCommand('DOM.enable'); - console.time('getdoc'); // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds // if the `DOM` domain was enabled before this gatherer, invoke it to be safe. await session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}); - console.timeEnd('getdoc'); - console.time('simpelisteners'); - - - const listeners = await Promise.all(anchors.map(anchor => getEventListeners(session, anchor.node.devtoolsNodePath))); - const paralellresult = anchors.map((anchor, i) => ({ - ...anchor, - listeners: listeners[i], - })); - console.timeEnd('simpelisteners'); - - // // const anchorsWithEventListeners = anchors.map(async anchor => { - // // const listeners = getEventListeners(session, anchor.node.devtoolsNodePath); - - - // // return { - // // ...anchor, - // // listeners, - // // }; - // // }); - - // const resultsimple = await Promise.all(anchorsWithEventListeners); - - // console.timeEnd('simpelisteners'); - console.time('listeners'); // Phase 1: Collect all unique node paths from anchors and their ancestors. @@ -241,14 +215,6 @@ class AnchorElements extends BaseGatherer { console.timeEnd('listeners'); - - console.time('getSnapshot'); - // It'd be faster (on complex pages like cnn.com to use `sendCommand('DOMSnapshot.getSnapshot', {includeEventListeners: true, computedStyleWhitelist: []});` - // However that'd require a large refactor of how we collect/match dom nodes. - // If this remains a performance concern we should try to get rid of the ancestor listener consideration. - console.timeEnd('getSnapshot'); - - await session.sendCommand('DOM.disable'); return result; } From 1d559bec4ce8a7488b50e043af048140ff2b4b7a Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 14:26:05 -0700 Subject: [PATCH 07/13] gem 2nd pass --- core/gather/gatherers/anchor-elements.js | 79 ++++++++++++------------ 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index f46c0194d252..9e1ce0495ba0 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -159,62 +159,59 @@ class AnchorElements extends BaseGatherer { // if the `DOM` domain was enabled before this gatherer, invoke it to be safe. await session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}); - - console.time('listeners'); - - // Phase 1: Collect all unique node paths from anchors and their ancestors. +console.time('listeners'); + // For each anchor, compute its ancestor paths. Store all paths in a single set + // for deduplication, and store the ancestor paths for each anchor in a map. const allPaths = new Set(); - for (const anchor of anchors) { - allPaths.add(anchor.node.devtoolsNodePath); - const splitPath = anchor.node.devtoolsNodePath.split(','); - while (splitPath.length > 2) { - splitPath.length -= 2; - allPaths.add(splitPath.join(',')); - } - } + const anchorAncestorPaths = new Map( + anchors.map(anchor => { + const anchorPath = anchor.node.devtoolsNodePath; + allPaths.add(anchorPath); + + const ancestorPaths = []; + const splitPath = anchorPath.split(','); + while (splitPath.length > 2) { + splitPath.length -= 2; + const ancestorPath = splitPath.join(','); + ancestorPaths.push(ancestorPath); + allPaths.add(ancestorPath); + } + return [anchorPath, ancestorPaths]; + }) + ); - // Phase 2: Fetch event listeners for all unique paths in parallel. - /** @type {Map>} */ + // Fetch event listeners for all unique paths in parallel. const listenersByPath = new Map(); - const listenerPromises = Array.from(allPaths).map(path => { - return getEventListeners(session, path) - .then(listeners => { - if (listeners.length) { - listenersByPath.set(path, listeners); - } - }) - .catch(() => {}); // Ignore errors, e.g. node not found. + const listenerPromises = [...allPaths].map(async path => { + try { + const listeners = await getEventListeners(session, path); + if (listeners.length) listenersByPath.set(path, listeners); + } catch { + // Ignore errors, e.g. node not found. + } }); await Promise.all(listenerPromises); - // Phase 3: Augment anchor data with the fetched listeners. + // Augment anchor data with the fetched listeners. const result = anchors.map(anchor => { - const listeners = listenersByPath.get(anchor.node.devtoolsNodePath) || []; - - /** @type {Map} */ - const ancestorListeners = new Map(); - const splitPath = anchor.node.devtoolsNodePath.split(','); - while (splitPath.length > 2) { - splitPath.length -= 2; - const path = splitPath.join(','); - const pathListeners = listenersByPath.get(path); - if (pathListeners) { - for (const listener of pathListeners) { - ancestorListeners.set(listener.type, listener); - } - } - } + const anchorPath = anchor.node.devtoolsNodePath; + const listeners = listenersByPath.get(anchorPath) || []; + const ancestorPaths = anchorAncestorPaths.get(anchorPath) || []; + const ancestorListeners = new Map( + ancestorPaths + .flatMap(path => listenersByPath.get(path) || []) + .map(listener => [listener.type, listener]) + ); return { ...anchor, listeners, - ancestorListeners: Array.from(ancestorListeners.values()), + ancestorListeners: [...ancestorListeners.values()], }; }); - console.timeEnd('listeners'); - +console.timeEnd('listeners'); await session.sendCommand('DOM.disable'); return result; } From 39ab7967977777be0f830302d6bf315ad6a52c20 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 14:35:32 -0700 Subject: [PATCH 08/13] cleaner --- core/gather/gatherers/anchor-elements.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index 9e1ce0495ba0..85c8354e0aa0 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -159,7 +159,7 @@ class AnchorElements extends BaseGatherer { // if the `DOM` domain was enabled before this gatherer, invoke it to be safe. await session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}); -console.time('listeners'); + console.time('listeners'); // For each anchor, compute its ancestor paths. Store all paths in a single set // for deduplication, and store the ancestor paths for each anchor in a map. const allPaths = new Set(); @@ -183,12 +183,8 @@ console.time('listeners'); // Fetch event listeners for all unique paths in parallel. const listenersByPath = new Map(); const listenerPromises = [...allPaths].map(async path => { - try { - const listeners = await getEventListeners(session, path); - if (listeners.length) listenersByPath.set(path, listeners); - } catch { - // Ignore errors, e.g. node not found. - } + const listeners = await getEventListeners(session, path).catch(() => {}); + if (listeners?.length) listenersByPath.set(path, listeners); }); await Promise.all(listenerPromises); @@ -211,7 +207,7 @@ console.time('listeners'); }; }); -console.timeEnd('listeners'); + console.timeEnd('listeners'); await session.sendCommand('DOM.disable'); return result; } From 2d22849c9ab92bc850a1178d6723248a17841d84 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 14:53:38 -0700 Subject: [PATCH 09/13] tried going back to earlier impl and doing a minimal cache fix. it on its own is okay but tehre's still an inner promise.all which means this is still slow. actually no its faster. cool. --- core/gather/gatherers/anchor-elements.js | 73 +++++++++++------------- 1 file changed, 33 insertions(+), 40 deletions(-) diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index 85c8354e0aa0..bed71ded3c1a 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -155,58 +155,51 @@ class AnchorElements extends BaseGatherer { }); await session.sendCommand('DOM.enable'); - // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds - // if the `DOM` domain was enabled before this gatherer, invoke it to be safe. + // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds if the `DOM` domain was enabled before this gatherer, invoke it to be safe. await session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}); console.time('listeners'); - // For each anchor, compute its ancestor paths. Store all paths in a single set - // for deduplication, and store the ancestor paths for each anchor in a map. - const allPaths = new Set(); - const anchorAncestorPaths = new Map( - anchors.map(anchor => { - const anchorPath = anchor.node.devtoolsNodePath; - allPaths.add(anchorPath); - - const ancestorPaths = []; - const splitPath = anchorPath.split(','); - while (splitPath.length > 2) { - splitPath.length -= 2; - const ancestorPath = splitPath.join(','); - ancestorPaths.push(ancestorPath); - allPaths.add(ancestorPath); - } - return [anchorPath, ancestorPaths]; - }) - ); - - // Fetch event listeners for all unique paths in parallel. - const listenersByPath = new Map(); - const listenerPromises = [...allPaths].map(async path => { - const listeners = await getEventListeners(session, path).catch(() => {}); - if (listeners?.length) listenersByPath.set(path, listeners); - }); - await Promise.all(listenerPromises); - // Augment anchor data with the fetched listeners. - const result = anchors.map(anchor => { - const anchorPath = anchor.node.devtoolsNodePath; - const listeners = listenersByPath.get(anchorPath) || []; - const ancestorPaths = anchorAncestorPaths.get(anchorPath) || []; + /** @type {Map>} */ + const listenerCache = new Map(); + + const anchorsWithEventListeners = anchors.map(async anchor => { + const listeners = await getEventListeners(session, anchor.node.devtoolsNodePath); + + // Doesn't make sense as a set, since objects with the same type are still different. + /** @type {Array<{type: string}>} */ + const ancestorListeners = []; + const splitPath = anchor.node.devtoolsNodePath.split(','); + const ancestorListenerPromises = []; + while (splitPath.length > 2) { + splitPath.length -= 2; + const path = splitPath.join(','); + // Check cache to avoid duplicate CDP calls for ancestors shared between multiple anchors. + if (listenerCache.has(path)) { + ancestorListenerPromises.push(listenerCache.get(path)?.then(listeners => { + ancestorListeners.push(...listeners); + })); + } else { + const promise = getEventListeners(session, path).then(listeners => { + ancestorListeners.push(...listeners); + return listeners; + }); + listenerCache.set(path, promise); + ancestorListenerPromises.push(promise); + } + } - const ancestorListeners = new Map( - ancestorPaths - .flatMap(path => listenersByPath.get(path) || []) - .map(listener => [listener.type, listener]) - ); + await Promise.all(ancestorListenerPromises); return { ...anchor, listeners, - ancestorListeners: [...ancestorListeners.values()], + ancestorListeners: Array.from(ancestorListeners), }; }); + + const result = await Promise.all(anchorsWithEventListeners); console.timeEnd('listeners'); await session.sendCommand('DOM.disable'); return result; From 557c342a508073ab86c5539d972140c9b8c6a299 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 16:00:13 -0700 Subject: [PATCH 10/13] remove ancestor listeners entirely. we're so loose with them the signal is 95% useless --- core/gather/gatherers/anchor-elements.js | 42 ++++-------------------- 1 file changed, 7 insertions(+), 35 deletions(-) diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index bed71ded3c1a..db77db63f7e5 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -160,42 +160,14 @@ class AnchorElements extends BaseGatherer { console.time('listeners'); - /** @type {Map>} */ - const listenerCache = new Map(); - - const anchorsWithEventListeners = anchors.map(async anchor => { - const listeners = await getEventListeners(session, anchor.node.devtoolsNodePath); - - // Doesn't make sense as a set, since objects with the same type are still different. - /** @type {Array<{type: string}>} */ - const ancestorListeners = []; - const splitPath = anchor.node.devtoolsNodePath.split(','); - const ancestorListenerPromises = []; - while (splitPath.length > 2) { - splitPath.length -= 2; - const path = splitPath.join(','); - // Check cache to avoid duplicate CDP calls for ancestors shared between multiple anchors. - if (listenerCache.has(path)) { - ancestorListenerPromises.push(listenerCache.get(path)?.then(listeners => { - ancestorListeners.push(...listeners); - })); - } else { - const promise = getEventListeners(session, path).then(listeners => { - ancestorListeners.push(...listeners); - return listeners; - }); - listenerCache.set(path, promise); - ancestorListenerPromises.push(promise); - } - } - - await Promise.all(ancestorListenerPromises); - return { - ...anchor, - listeners, - ancestorListeners: Array.from(ancestorListeners), - }; + const anchorsWithEventListeners = anchors.map( anchor => { + return getEventListeners(session, anchor.node.devtoolsNodePath).then(listeners => { + return { + ...anchor, + listeners, + }; + }); }); From 297c4ba1126cd7a6bd3f3e67c3795d0882b3376d Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 16:07:09 -0700 Subject: [PATCH 11/13] finishing removing ancestor listeners --- core/audits/seo/crawlable-anchors.js | 5 ++--- core/gather/gatherers/anchor-elements.js | 4 ---- core/test/audits/seo/crawlable-anchors-test.js | 10 +--------- types/artifacts.d.ts | 3 --- 4 files changed, 3 insertions(+), 19 deletions(-) diff --git a/core/audits/seo/crawlable-anchors.js b/core/audits/seo/crawlable-anchors.js index 1fe024c44849..1cd1f820a6bf 100644 --- a/core/audits/seo/crawlable-anchors.js +++ b/core/audits/seo/crawlable-anchors.js @@ -57,12 +57,10 @@ class CrawlableAnchors extends Audit { href, attributeNames = [], listeners = [], - ancestorListeners = [], }) => { rawHref = rawHref.replace( /\s/g, ''); name = name.trim(); role = role.trim(); - const hasListener = Boolean(listeners.length || ancestorListeners.length); if (role.length > 0) return; // Ignore mailto links even if they use one of the failing patterns. See https://github.com/GoogleChrome/lighthouse/issues/11443#issuecomment-694898412 @@ -86,7 +84,8 @@ class CrawlableAnchors extends Audit { !attributeNames.includes('href') && hrefAssociatedAttributes.every(attribute => !attributeNames.includes(attribute)) ) { - return hasListener; + // If it has an even listener (e.g. onclick) then we can't assume it's a placeholder. Therefore we consider it failing + return Boolean(listeners.length); } if (href === '') return true; diff --git a/core/gather/gatherers/anchor-elements.js b/core/gather/gatherers/anchor-elements.js index db77db63f7e5..8efabfa72757 100644 --- a/core/gather/gatherers/anchor-elements.js +++ b/core/gather/gatherers/anchor-elements.js @@ -158,9 +158,6 @@ class AnchorElements extends BaseGatherer { // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds if the `DOM` domain was enabled before this gatherer, invoke it to be safe. await session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}); - console.time('listeners'); - - const anchorsWithEventListeners = anchors.map( anchor => { return getEventListeners(session, anchor.node.devtoolsNodePath).then(listeners => { return { @@ -172,7 +169,6 @@ class AnchorElements extends BaseGatherer { const result = await Promise.all(anchorsWithEventListeners); - console.timeEnd('listeners'); await session.sendCommand('DOM.disable'); return result; } diff --git a/core/test/audits/seo/crawlable-anchors-test.js b/core/test/audits/seo/crawlable-anchors-test.js index 9efa4c79fbb4..959b0c85c1be 100644 --- a/core/test/audits/seo/crawlable-anchors-test.js +++ b/core/test/audits/seo/crawlable-anchors-test.js @@ -19,7 +19,6 @@ function runAudit({ (rawHref || href) ? 'href' : null, role ? 'role' : null, name ? 'name' : null, ].filter(Boolean), listeners = onclick.trim().length ? [{type: 'click'}] : [], - ancestorListeners = [], node = { snippet: '', devtoolsNodePath: '', @@ -34,7 +33,6 @@ function runAudit({ href, name, listeners, - ancestorListeners, onclick, role, node, @@ -105,19 +103,13 @@ describe('SEO: Crawlable anchors audit', () => { rawHref: 'javascript:void(0)', listeners: [{type: 'nope'}, {type: 'another'}, {type: 'click'}], }); - assert.equal(auditResultWithInvalidHref, 0, 'invalid href with any event listener is a faile'); + assert.equal(auditResultWithInvalidHref, 0, 'invalid href with any event listener is a fail'); const auditResultWithNoHref = runAudit({ listeners: [{type: 'nope'}, {type: 'another'}, {type: 'click'}], }); assert.equal(auditResultWithNoHref, 0, 'no href with any event listener is a fail'); - const auditResultWithParentListenerNoHref = runAudit({ - ancestorListeners: [{type: 'nope'}, {type: 'another'}, {type: 'click'}], - }); - assert.equal(auditResultWithParentListenerNoHref, 0, - 'no href with any event listener on a parent is a fail'); - const auditResultNoListener = runAudit({ rawHref: '/validPath', }); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 0a277f5b1777..dde38d0572c7 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -363,9 +363,6 @@ declare module Artifacts { listeners?: Array<{ type: Crdp.DOMDebugger.EventListener['type'] }> - ancestorListeners?: Array<{ - type: Crdp.DOMDebugger.EventListener['type'] - }> } type BFCacheReasonMap = { From 2238566a832d7e33a0cd8ddb4d4701bf8fc1a705 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 6 Oct 2025 16:25:11 -0700 Subject: [PATCH 12/13] Revert "temporary instrumetation" This reverts commit 03978b7201c3e4cb8ec787965432b897f7cdd7ef. --- core/audits/seo/crawlable-anchors.js | 2 +- core/gather/session.js | 8 ++------ core/runner.js | 1 - 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/core/audits/seo/crawlable-anchors.js b/core/audits/seo/crawlable-anchors.js index 1cd1f820a6bf..5c047349e5cc 100644 --- a/core/audits/seo/crawlable-anchors.js +++ b/core/audits/seo/crawlable-anchors.js @@ -84,7 +84,7 @@ class CrawlableAnchors extends Audit { !attributeNames.includes('href') && hrefAssociatedAttributes.every(attribute => !attributeNames.includes(attribute)) ) { - // If it has an even listener (e.g. onclick) then we can't assume it's a placeholder. Therefore we consider it failing + // If it has an even listener (e.g. onclick) then we can't assume it's a placeholder. Therefore we consider it failing. return Boolean(listeners.length); } diff --git a/core/gather/session.js b/core/gather/session.js index 06a22c952b25..8d9bd24ad7af 100644 --- a/core/gather/session.js +++ b/core/gather/session.js @@ -14,10 +14,6 @@ import {LighthouseError} from '../lib/lh-error.js'; const DEFAULT_PROTOCOL_TIMEOUT = 30000; const PPTR_BUFFER = 50; -let i = 0; -const calls = {}; -global.calls = calls; - /** * Puppeteer timeouts must fit into an int32 and the maximum timeout for `setTimeout` is a *signed* * int32. However, this also needs to account for the puppeteer buffer we add to the timeout later. @@ -116,7 +112,7 @@ class ProtocolSession extends CrdpEventEmitter { sendCommand(method, ...params) { const timeoutMs = this.getNextProtocolTimeout(); this._nextProtocolTimeout = undefined; - calls[method] = (calls[method] || 0) + 1; + /** @type {NodeJS.Timeout|undefined} */ let timeout; const timeoutPromise = new Promise((resolve, reject) => { @@ -131,7 +127,7 @@ class ProtocolSession extends CrdpEventEmitter { // Add 50ms to the Puppeteer timeout to ensure the Lighthouse timeout finishes first. timeout: timeoutMs + PPTR_BUFFER, }).catch((error) => { - log.formatProtocol('method <= browser ERR', {method, params}, 'error'); + log.formatProtocol('method <= browser ERR', {method}, 'error'); throw LighthouseError.fromProtocolMessage(method, error); }); const resultWithTimeoutPromise = diff --git a/core/runner.js b/core/runner.js index 16c4b33e6b43..e1b5261bbbd2 100644 --- a/core/runner.js +++ b/core/runner.js @@ -133,7 +133,6 @@ class Runner { assetSaver.saveLhr(lhr, path); } - console.log(global.calls); // Create the HTML, JSON, and/or CSV string const report = ReportGenerator.generateReport(lhr, settings.output); From e8c67deef22f765deaff3d5f79f3186fdadca74d Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Tue, 7 Oct 2025 13:04:56 -0700 Subject: [PATCH 13/13] smoke testfix --- cli/test/fixtures/seo/seo-failure-cases.html | 2 +- cli/test/smokehouse/test-definitions/seo-failing.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/test/fixtures/seo/seo-failure-cases.html b/cli/test/fixtures/seo/seo-failure-cases.html index efd3f536b212..bfbd84d69312 100644 --- a/cli/test/fixtures/seo/seo-failure-cases.html +++ b/cli/test/fixtures/seo/seo-failure-cases.html @@ -50,7 +50,7 @@

Anchor text

document.querySelector('.some-link').addEventListener('mousedown', () => {}); - +
Some link