Merge pull request #154 from GoogleChrome/cls-if-fcp

Only report CLS when FCP is reported
GoogleChrome · May 28, 2021 · b19f6ee · b19f6ee
2 parents 21915c4 + 1f5f524
commit b19f6ee
Show file tree

Hide file tree

Showing 11 changed files with 107 additions and 52 deletions.
diff --git a/README.md b/README.md
@@ -444,11 +444,14 @@ The following table lists all the bundles distributed with the `web-vitals` pack
 
 Most developers will generally want to use the "standard" bundle (either the ES module or UMD version, depending on your build system), as it's the easiest to use out of the box and integrate into existing build tools.
 
-However, developers willing to manage the additional usage complexity should consider the "base+polyfill" bundle if they would like to measure FID in all browsers.
+However, there are a few good reasons to consider using the "base+polyfill" version, for example:
+
+- FID can be measured in all browsers.
+- CLS, FCP, FID, and LCP will be more accurate in some cases (since the polyfill detects the page's initial `visibilityState` earlier).
 
 ### How the polyfill works
 
-The `polyfill.js` script adds event listeners that record the event processing delay of the first input, and then removes those event listeners after the first input occurs.
+The `polyfill.js` script adds event listeners (to track FID cross-browser), and it records initial page visibility state as well as the timestamp of the first visibility change to hidden (to improve the accuracy of CLS, FCP, LCP, and FID).
 
 In order for it to work properly, the script must be the first script added to the page, and it must run before the browser renders any content to the screen. This is why it needs to be added to the `<head>` of the document.
 
@@ -527,6 +530,7 @@ If using the "base+polyfill" build, the `polyfill.js` script creates the global
 interface WebVitalsGlobal {
   firstInputPolyfill: (onFirstInput: FirstInputPolyfillCallback) => void;
   resetFirstInputPolyfill: () => void;
+  firstHiddenTime: number;
 }
 ```
 

diff --git a/src/getCLS.ts b/src/getCLS.ts
@@ -14,12 +14,12 @@
  * limitations under the License.
  */
 
-import {getVisibilityWatcher} from './lib/getVisibilityWatcher.js';
 import {initMetric} from './lib/initMetric.js';
 import {observe, PerformanceEntryHandler} from './lib/observe.js';
 import {onHidden} from './lib/onHidden.js';
 import {onBFCacheRestore} from './lib/onBFCacheRestore.js';
 import {bindReporter} from './lib/bindReporter.js';
+import {getFCP} from './getFCP.js';
 import {ReportHandler} from './types.js';
 
 
@@ -29,17 +29,22 @@ interface LayoutShift extends PerformanceEntry {
   hadRecentInput: boolean;
 }
 
+
+let isMonitoringFCP = false;
+let fcpValue = -1;
+
 export const getCLS = (onReport: ReportHandler, reportAllChanges?: boolean) => {
-  const visibilityWatcher = getVisibilityWatcher();
+  // Start monitoring FCP so we can only report CLS if FCP is also reported.
+  // Note: this is done to match the current behavior of CrUX.
+  if (!isMonitoringFCP) {
+    getFCP((metric) => {
+      fcpValue = metric.value;
+    });
+    isMonitoringFCP = true;
+  }
 
   const onReportWrapped: ReportHandler = (arg) => {
-    // Only report if the page was visible at some point in its lifecycle.
-    // Note: this doesn't technically match the current behavior of CrUX, which
-    // only includes pages that report FCP. However, we plan to change the
-    // behavior of CrUX in the future, and matching it would couple CLS to FCP
-    // in an awkward way, so in this library we only ignore CLS if the page
-    // was never visible (which should be the same as CrUX in most)
-    if (visibilityWatcher.firstVisibleTime < performance.now()) {
+    if (fcpValue > -1) {
       onReport(arg);
     }
   };
@@ -71,7 +76,6 @@ export const getCLS = (onReport: ReportHandler, reportAllChanges?: boolean) => {
       if (sessionValue > metric.value) {
         metric.value = sessionValue;
         metric.entries = sessionEntries;
-
         report();
       }
     }
@@ -88,6 +92,7 @@ export const getCLS = (onReport: ReportHandler, reportAllChanges?: boolean) => {
 
     onBFCacheRestore(() => {
       sessionValue = 0;
+      fcpValue = -1;
       metric = initMetric('CLS', 0);
       report = bindReporter(onReportWrapped, metric, reportAllChanges);
     });

diff --git a/src/lib/getVisibilityWatcher.ts b/src/lib/getVisibilityWatcher.ts
@@ -15,64 +15,51 @@
  */
 
 import {onBFCacheRestore} from './onBFCacheRestore.js';
+import {onHidden} from './onHidden.js';
 
+let firstHiddenTime = -1;
 
-interface TimeStamps {
-  hidden?: number;
-  visible?: number;
-}
-
-interface VisibilityWatcher {
-  firstHiddenTime: number;
-  firstVisibleTime: number;
-}
-
-
-let timeStamps: TimeStamps;
-
-const initTimeStamps = () => {
-  // Assume the visibilityState when this code is run was the visibilityState
-  // since page load. This isn't a perfect heuristic, but it's the best we can
-  // do until an API is available to support querying past visibilityState.
-  timeStamps = {
-    'visible': document.visibilityState === 'visible' ? 0 : Infinity,
-    'hidden': document.visibilityState === 'hidden' ? 0 : Infinity,
-  };
-}
-
-const onVisibilityChange = (event: Event) => {
-  timeStamps[document.visibilityState] = event.timeStamp;
-  if (timeStamps.hidden! + timeStamps.visible! > 0) {
-    removeEventListener('visibilitychange', onVisibilityChange, true);
-  }
+const initHiddenTime = () => {
+  return document.visibilityState === 'hidden' ? 0 : Infinity;
 }
 
 const trackChanges = () => {
-  addEventListener('visibilitychange', onVisibilityChange, true);
+  // Update the time if/when the document becomes hidden.
+  onHidden(({timeStamp}) => {
+    firstHiddenTime = timeStamp
+  }, true);
 };
 
-export const getVisibilityWatcher = () : VisibilityWatcher => {
-  if (!timeStamps) {
-    initTimeStamps();
-    trackChanges();
+export const getVisibilityWatcher = () => {
+  if (firstHiddenTime < 0) {
+    // If the document is hidden when this code runs, assume it was hidden
+    // since navigation start. This isn't a perfect heuristic, but it's the
+    // best we can do until an API is available to support querying past
+    // visibilityState.
+    if (self.__WEB_VITALS_POLYFILL__) {
+      firstHiddenTime = self.webVitals.firstHiddenTime;
+      if (firstHiddenTime === Infinity) {
+        trackChanges();
+      }
+    } else {
+      firstHiddenTime = initHiddenTime();
+      trackChanges();
+    }
 
     // Reset the time on bfcache restores.
     onBFCacheRestore(() => {
       // Schedule a task in order to track the `visibilityState` once it's
       // had an opportunity to change to visible in all browsers.
       // https://bugs.chromium.org/p/chromium/issues/detail?id=1133363
       setTimeout(() => {
-        initTimeStamps();
+        firstHiddenTime = initHiddenTime();
         trackChanges();
       }, 0);
     });
   }
   return {
     get firstHiddenTime() {
-      return timeStamps.hidden!;
-    },
-    get firstVisibleTime() {
-      return timeStamps.visible!;
-    },
-  };
+      return firstHiddenTime;
+    }
+  }
 };
diff --git a/src/lib/polyfills/getFirstHiddenTimePolyfill.ts b/src/lib/polyfills/getFirstHiddenTimePolyfill.ts
@@ -0,0 +1,30 @@
+/*
+ * 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.
+ */
+
+let firstHiddenTime =
+    document.visibilityState === 'hidden' ? 0 : Infinity;
+
+const onVisibilityChange = (event: Event) => {
+  if (document.visibilityState === 'hidden') {
+    firstHiddenTime = event.timeStamp;
+    removeEventListener('visibilitychange', onVisibilityChange, true);
+  }
+}
+
+// Note: do not add event listeners unconditionally (outside of polyfills).
+addEventListener('visibilitychange', onVisibilityChange, true);
+
+export const getFirstHiddenTime = () => firstHiddenTime;
diff --git a/src/polyfill.ts b/src/polyfill.ts
@@ -15,9 +15,13 @@
  */
 
 import {firstInputPolyfill, resetFirstInputPolyfill} from './lib/polyfills/firstInputPolyfill.js';
+import {getFirstHiddenTime} from './lib/polyfills/getFirstHiddenTimePolyfill.js';
 
 resetFirstInputPolyfill();
 self.webVitals = {
   firstInputPolyfill: firstInputPolyfill,
   resetFirstInputPolyfill: resetFirstInputPolyfill,
+  get firstHiddenTime() {
+    return getFirstHiddenTime();
+  },
 };
diff --git a/src/types.ts b/src/types.ts
@@ -66,6 +66,7 @@ export type NavigationTimingPolyfillEntry = Omit<PerformanceNavigationTiming,
 export interface WebVitalsGlobal {
   firstInputPolyfill: (onFirstInput: FirstInputPolyfillCallback) => void;
   resetFirstInputPolyfill: () => void;
+  firstHiddenTime: number;
 }
 
 declare global {

diff --git a/test/e2e/getCLS-test.js b/test/e2e/getCLS-test.js
@@ -17,12 +17,16 @@
 const assert = require('assert');
 const {beaconCountIs, clearBeacons, getBeacons} = require('../utils/beacons.js');
 const {browserSupportsEntry} = require('../utils/browserSupportsEntry.js');
+const {afterLoad} = require('../utils/afterLoad.js');
 const {imagesPainted} = require('../utils/imagesPainted.js');
 const {stubForwardBack} = require('../utils/stubForwardBack.js');
 const {stubVisibilityChange} = require('../utils/stubVisibilityChange.js');
 
 
 describe('getCLS()', async function() {
+  // Retry all tests in this suite up to 2 times.
+  this.retries(2);
+
   let browserSupportsCLS;
   before(async function() {
     browserSupportsCLS = await browserSupportsEntry('layout-shift');
@@ -432,6 +436,8 @@ describe('getCLS()', async function() {
 
     await browser.url(`/test/cls?noLayoutShifts=1`);
 
+    // Wait until the page is loaded before hiding.
+    await afterLoad();
     await stubVisibilityChange('hidden');
 
     await beaconCountIs(1);
@@ -449,6 +455,8 @@ describe('getCLS()', async function() {
 
     await browser.url(`/test/cls?reportAllChanges=1&noLayoutShifts=1`);
 
+    // Wait until the page is loaded before hiding.
+    await afterLoad();
     await stubVisibilityChange('hidden');
 
     await beaconCountIs(1);
@@ -466,6 +474,8 @@ describe('getCLS()', async function() {
 
     await browser.url(`/test/cls?noLayoutShifts=1`);
 
+    // Wait until the page is loaded before navigating away.
+    await afterLoad();
     await browser.url('about:blank');
 
     await beaconCountIs(1);
@@ -483,6 +493,8 @@ describe('getCLS()', async function() {
 
     await browser.url(`/test/cls?noLayoutShifts=1&reportAllChanges=1`);
 
+    // Wait until the page is loaded before navigating away.
+    await afterLoad();
     await browser.url('about:blank');
 
     await beaconCountIs(1);

diff --git a/test/e2e/getFCP-test.js b/test/e2e/getFCP-test.js
@@ -21,6 +21,9 @@ const {stubForwardBack} = require('../utils/stubForwardBack.js');
 const {stubVisibilityChange} = require('../utils/stubVisibilityChange.js');
 
 describe('getFCP()', async function() {
+  // Retry all tests in this suite up to 2 times.
+  this.retries(2);
+
   let browserSupportsFCP;
   before(async function() {
     browserSupportsFCP = await browserSupportsEntry('paint');

diff --git a/test/e2e/getFID-test.js b/test/e2e/getFID-test.js
@@ -22,6 +22,9 @@ const {stubVisibilityChange} = require('../utils/stubVisibilityChange.js');
 
 
 describe('getFID()', async function() {
+  // Retry all tests in this suite up to 2 times.
+  this.retries(2);
+
   let browserSupportsFID;
   before(async function() {
     browserSupportsFID = await browserSupportsEntry('first-input');

diff --git a/test/e2e/getLCP-test.js b/test/e2e/getLCP-test.js
@@ -22,6 +22,9 @@ const {stubVisibilityChange} = require('../utils/stubVisibilityChange.js');
 
 
 describe('getLCP()', async function() {
+  // Retry all tests in this suite up to 2 times.
+  this.retries(2);
+
   let browserSupportsLCP;
   before(async function() {
     browserSupportsLCP = await browserSupportsEntry('largest-contentful-paint');

diff --git a/test/e2e/getTTFB-test.js b/test/e2e/getTTFB-test.js
@@ -61,6 +61,9 @@ function assertValidEntry(entry) {
 }
 
 describe('getTTFB()', async function() {
+  // Retry all tests in this suite up to 2 times.
+  this.retries(2);
+
   beforeEach(async function() {
     await clearBeacons();
   });