From 4814b460a9a92a84a0b7ad6541ffb839e441608c Mon Sep 17 00:00:00 2001 From: Philip Walton Date: Wed, 2 Oct 2024 14:41:28 -0700 Subject: [PATCH 1/4] Ensure one-time event listeners only run once --- src/lib/onHidden.ts | 23 ----------------------- src/lib/whenIdle.ts | 6 +----- src/onCLS.ts | 9 +++++---- src/onINP.ts | 9 +++++---- src/onLCP.ts | 17 +++++++++-------- 5 files changed, 20 insertions(+), 44 deletions(-) delete mode 100644 src/lib/onHidden.ts diff --git a/src/lib/onHidden.ts b/src/lib/onHidden.ts deleted file mode 100644 index f59d4c90..00000000 --- a/src/lib/onHidden.ts +++ /dev/null @@ -1,23 +0,0 @@ -/* - * Copyright 2020 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -export const onHidden = (cb: () => void) => { - document.addEventListener('visibilitychange', () => { - if (document.visibilityState === 'hidden') { - cb(); - } - }); -}; diff --git a/src/lib/whenIdle.ts b/src/lib/whenIdle.ts index d63b399a..11418719 100644 --- a/src/lib/whenIdle.ts +++ b/src/lib/whenIdle.ts @@ -14,9 +14,6 @@ * limitations under the License. */ -import {onHidden} from './onHidden.js'; -import {runOnce} from './runOnce.js'; - /** * Runs the passed callback during the next idle period, or immediately * if the browser's visibility state is (or becomes) hidden. @@ -25,14 +22,13 @@ export const whenIdle = (cb: () => void): number => { const rIC = globalThis.requestIdleCallback || setTimeout; let handle = -1; - cb = runOnce(cb); // If the document is hidden, run the callback immediately, otherwise // race an idle callback with the next `visibilitychange` event. if (document.visibilityState === 'hidden') { cb(); } else { handle = rIC(cb); - onHidden(cb); + document.addEventListener('visibilitychange', cb, {once: true}); } return handle; }; diff --git a/src/onCLS.ts b/src/onCLS.ts index a4668506..60a30eb6 100644 --- a/src/onCLS.ts +++ b/src/onCLS.ts @@ -19,7 +19,6 @@ import {initMetric} from './lib/initMetric.js'; import {observe} from './lib/observe.js'; import {bindReporter} from './lib/bindReporter.js'; import {doubleRAF} from './lib/doubleRAF.js'; -import {onHidden} from './lib/onHidden.js'; import {runOnce} from './lib/runOnce.js'; import {onFCP} from './onFCP.js'; import {CLSMetric, MetricRatingThresholds, ReportOpts} from './types.js'; @@ -107,9 +106,11 @@ export const onCLS = ( opts!.reportAllChanges, ); - onHidden(() => { - handleEntries(po.takeRecords() as CLSMetric['entries']); - report(true); + document.addEventListener('visibilitychange', () => { + if (document.visibilityState === 'hidden') { + handleEntries(po.takeRecords() as CLSMetric['entries']); + report(true); + } }); // Only report after a bfcache restore if the `PerformanceObserver` diff --git a/src/onINP.ts b/src/onINP.ts index 63205d25..276e5ccf 100644 --- a/src/onINP.ts +++ b/src/onINP.ts @@ -24,7 +24,6 @@ import { resetInteractions, } from './lib/interactions.js'; import {observe} from './lib/observe.js'; -import {onHidden} from './lib/onHidden.js'; import {initInteractionCountPolyfill} from './lib/polyfills/interactionCountPolyfill.js'; import {whenActivated} from './lib/whenActivated.js'; import {whenIdle} from './lib/whenIdle.js'; @@ -126,9 +125,11 @@ export const onINP = ( // where the first interaction is less than the `durationThreshold`. po.observe({type: 'first-input', buffered: true}); - onHidden(() => { - handleEntries(po.takeRecords() as INPMetric['entries']); - report(true); + document.addEventListener('visibilitychange', () => { + if (document.visibilityState === 'hidden') { + handleEntries(po.takeRecords() as INPMetric['entries']); + report(true); + } }); // Only report after a bfcache restore if the `PerformanceObserver` diff --git a/src/onLCP.ts b/src/onLCP.ts index 68557711..33fde686 100644 --- a/src/onLCP.ts +++ b/src/onLCP.ts @@ -21,7 +21,6 @@ import {getActivationStart} from './lib/getActivationStart.js'; import {getVisibilityWatcher} from './lib/getVisibilityWatcher.js'; import {initMetric} from './lib/initMetric.js'; import {observe} from './lib/observe.js'; -import {onHidden} from './lib/onHidden.js'; import {runOnce} from './lib/runOnce.js'; import {whenActivated} from './lib/whenActivated.js'; import {whenIdle} from './lib/whenIdle.js'; @@ -94,18 +93,20 @@ export const onLCP = ( } }); - // Stop listening after input. Note: while scrolling is an input that - // stops LCP observation, it's unreliable since it can be programmatically - // generated. See: https://github.com/GoogleChrome/web-vitals/issues/75 - for (const type of ['keydown', 'click']) { + // Stop listening after input or visibilitychange. + // Note: while scrolling is an input that stops LCP observation, it's + // unreliable since it can be programmatically generated. + // See: https://github.com/GoogleChrome/web-vitals/issues/75 + for (const type of ['keydown', 'click', 'visibilitychange']) { // Wrap in a setTimeout so the callback is run in a separate task // to avoid extending the keyboard/click handler to reduce INP impact // https://github.com/GoogleChrome/web-vitals/issues/383 - addEventListener(type, () => whenIdle(stopListening), true); + addEventListener(type, () => whenIdle(stopListening), { + capture: true, + once: true, + }); } - onHidden(stopListening); - // Only report after a bfcache restore if the `PerformanceObserver` // successfully registered. onBFCacheRestore((event) => { From 833eb5bf0f731f8d355a3fe50cde74de15fd9d43 Mon Sep 17 00:00:00 2001 From: Philip Walton Date: Wed, 2 Oct 2024 15:18:30 -0700 Subject: [PATCH 2/4] Refactor whenIdle to make the logic more clear --- src/attribution/onINP.ts | 12 +++++----- src/lib/{whenIdle.ts => whenIdleOrHidden.ts} | 6 ++--- src/onINP.ts | 4 ++-- src/onLCP.ts | 25 +++++++++++--------- 4 files changed, 24 insertions(+), 23 deletions(-) rename src/lib/{whenIdle.ts => whenIdleOrHidden.ts} (90%) diff --git a/src/attribution/onINP.ts b/src/attribution/onINP.ts index 41b1c903..e60f346e 100644 --- a/src/attribution/onINP.ts +++ b/src/attribution/onINP.ts @@ -22,7 +22,7 @@ import { longestInteractionMap, } from '../lib/interactions.js'; import {observe} from '../lib/observe.js'; -import {whenIdle} from '../lib/whenIdle.js'; +import {whenIdleOrHidden} from '../lib/whenIdleOrHidden.js'; import {onINP as unattributedOnINP} from '../onINP.js'; import { INPAttribution, @@ -79,7 +79,7 @@ export const interactionTargetMap: Map = new Map(); // A reference to the idle task used to clean up entries from the above // variables. If the value is -1 it means no task is queue, and if it's // greater than -1 the value corresponds to the idle callback handle. -let idleHandle: number = -1; +let cleanupPending = false; /** * Adds new LoAF entries to the `pendingLoAFs` list. @@ -159,8 +159,9 @@ const groupEntriesByRenderTime = (entry: PerformanceEventTiming) => { const queueCleanup = () => { // Queue cleanup of entries that are not part of any INP candidates. - if (idleHandle < 0) { - idleHandle = whenIdle(cleanupEntries); + if (!cleanupPending) { + whenIdleOrHidden(cleanupEntries); + cleanupPending = true; } }; @@ -206,8 +207,7 @@ const cleanupEntries = () => { return loafsToKeep.has(loaf); }); - // Reset the idle callback handle so it can be queued again. - idleHandle = -1; + cleanupPending = false; }; entryPreProcessingCallbacks.push( diff --git a/src/lib/whenIdle.ts b/src/lib/whenIdleOrHidden.ts similarity index 90% rename from src/lib/whenIdle.ts rename to src/lib/whenIdleOrHidden.ts index 11418719..edd79b2a 100644 --- a/src/lib/whenIdle.ts +++ b/src/lib/whenIdleOrHidden.ts @@ -18,17 +18,15 @@ * Runs the passed callback during the next idle period, or immediately * if the browser's visibility state is (or becomes) hidden. */ -export const whenIdle = (cb: () => void): number => { +export const whenIdleOrHidden = (cb: () => void) => { const rIC = globalThis.requestIdleCallback || setTimeout; - let handle = -1; // If the document is hidden, run the callback immediately, otherwise // race an idle callback with the next `visibilitychange` event. if (document.visibilityState === 'hidden') { cb(); } else { - handle = rIC(cb); + rIC(cb); document.addEventListener('visibilitychange', cb, {once: true}); } - return handle; }; diff --git a/src/onINP.ts b/src/onINP.ts index 276e5ccf..ad5b530e 100644 --- a/src/onINP.ts +++ b/src/onINP.ts @@ -26,7 +26,7 @@ import { import {observe} from './lib/observe.js'; import {initInteractionCountPolyfill} from './lib/polyfills/interactionCountPolyfill.js'; import {whenActivated} from './lib/whenActivated.js'; -import {whenIdle} from './lib/whenIdle.js'; +import {whenIdleOrHidden} from './lib/whenIdleOrHidden.js'; import {INPMetric, MetricRatingThresholds, ReportOpts} from './types.js'; @@ -88,7 +88,7 @@ export const onINP = ( // have been dispatched. Note: there is currently an experiment // running in Chrome (EventTimingKeypressAndCompositionInteractionId) // 123+ that if rolled out fully may make this no longer necessary. - whenIdle(() => { + whenIdleOrHidden(() => { for (const entry of entries) { processInteractionEntry(entry); } diff --git a/src/onLCP.ts b/src/onLCP.ts index 33fde686..728983da 100644 --- a/src/onLCP.ts +++ b/src/onLCP.ts @@ -23,7 +23,7 @@ import {initMetric} from './lib/initMetric.js'; import {observe} from './lib/observe.js'; import {runOnce} from './lib/runOnce.js'; import {whenActivated} from './lib/whenActivated.js'; -import {whenIdle} from './lib/whenIdle.js'; +import {whenIdleOrHidden} from './lib/whenIdleOrHidden.js'; import {LCPMetric, MetricRatingThresholds, ReportOpts} from './types.js'; /** Thresholds for LCP. See https://web.dev/articles/lcp#what_is_a_good_lcp_score */ @@ -84,13 +84,19 @@ export const onLCP = ( opts!.reportAllChanges, ); + // Ensure this logic only runs once, and wrap it in an idle callback + // so the callback is run in a separate task to avoid extending the + // keyboard/click handler to reduce INP impact. + // https://github.com/GoogleChrome/web-vitals/issues/383 const stopListening = runOnce(() => { - if (!reportedMetricIDs[metric.id]) { - handleEntries(po!.takeRecords() as LCPMetric['entries']); - po!.disconnect(); - reportedMetricIDs[metric.id] = true; - report(true); - } + whenIdleOrHidden(() => { + if (!reportedMetricIDs[metric.id]) { + handleEntries(po!.takeRecords() as LCPMetric['entries']); + po!.disconnect(); + reportedMetricIDs[metric.id] = true; + report(true); + } + }); }); // Stop listening after input or visibilitychange. @@ -98,10 +104,7 @@ export const onLCP = ( // unreliable since it can be programmatically generated. // See: https://github.com/GoogleChrome/web-vitals/issues/75 for (const type of ['keydown', 'click', 'visibilitychange']) { - // Wrap in a setTimeout so the callback is run in a separate task - // to avoid extending the keyboard/click handler to reduce INP impact - // https://github.com/GoogleChrome/web-vitals/issues/383 - addEventListener(type, () => whenIdle(stopListening), { + addEventListener(type, () => stopListening(), { capture: true, once: true, }); From 9800588e54c2efe85ca9acb735bbba3edafa782d Mon Sep 17 00:00:00 2001 From: Philip Walton Date: Wed, 2 Oct 2024 16:35:01 -0700 Subject: [PATCH 3/4] Restore original runOnce and whenIdle order --- src/onLCP.ts | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/onLCP.ts b/src/onLCP.ts index 728983da..2c065a0c 100644 --- a/src/onLCP.ts +++ b/src/onLCP.ts @@ -88,16 +88,17 @@ export const onLCP = ( // so the callback is run in a separate task to avoid extending the // keyboard/click handler to reduce INP impact. // https://github.com/GoogleChrome/web-vitals/issues/383 - const stopListening = runOnce(() => { - whenIdleOrHidden(() => { - if (!reportedMetricIDs[metric.id]) { - handleEntries(po!.takeRecords() as LCPMetric['entries']); - po!.disconnect(); - reportedMetricIDs[metric.id] = true; - report(true); - } - }); - }); + const stopListening = () => + whenIdleOrHidden( + runOnce(() => { + if (!reportedMetricIDs[metric.id]) { + handleEntries(po!.takeRecords() as LCPMetric['entries']); + po!.disconnect(); + reportedMetricIDs[metric.id] = true; + report(true); + } + }), + ); // Stop listening after input or visibilitychange. // Note: while scrolling is an input that stops LCP observation, it's From 9e16c574821b5b2b165faaddb5eb9fb154adffc4 Mon Sep 17 00:00:00 2001 From: Philip Walton Date: Thu, 3 Oct 2024 09:15:06 -0700 Subject: [PATCH 4/4] Fix comment --- src/attribution/onINP.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/attribution/onINP.ts b/src/attribution/onINP.ts index e60f346e..c7351340 100644 --- a/src/attribution/onINP.ts +++ b/src/attribution/onINP.ts @@ -76,9 +76,7 @@ const entryToEntriesGroupMap: WeakMap< // A mapping of interactionIds to the target Node. export const interactionTargetMap: Map = new Map(); -// A reference to the idle task used to clean up entries from the above -// variables. If the value is -1 it means no task is queue, and if it's -// greater than -1 the value corresponds to the idle callback handle. +// A boolean flag indicating whether or not a cleanup task has been queued. let cleanupPending = false; /**