new_audit: add no-unload-listeners audit #11085

brendankenny · 2020-07-10T18:27:18Z

Fixes #10848. ~~WIP for now~~ All should feel free to comment on the details to highlight and advice the audit should provide.

Right now the audit fails if any 'unload' listener is registered, and recommends using 'pagehide' or 'visibilitychange' instead (leaving it up to the (eventually) linked docs to explain which of the two you might choose to use). The audit table is ~~just using the plain old violations-style table~~ using source-location, happy to iterate:

live example:
https://googlechrome.github.io/lighthouse/viewer/?gist=12c8d8d522e867564a4c3a526f4981bd

old example

live example (scroll down to Best Practices)
https://googlechrome.github.io/lighthouse/viewer/?gist=aeb90c472c6c079e0429faf370bb83b1

for the new gatherer, this information didn't really fit into an existing one, and I didn't really want to open us up to just collecting all the event listeners, so I kept it somewhat narrow in scope. I did include other unload-adjacent events because I get the feeling from talking to @philipwalton that there is more analysis we can do here about bfcache-worthiness, but if that doesn't materialize I can also narrow it down even more. I'm also open to completely different ideas :)

brendankenny · 2020-07-10T18:31:29Z

lighthouse-core/audits/no-unload-listeners.js

+  /** Descriptive title of a Lighthouse audit that checks if a web page has 'unload' event listeners and finds that it is using them. */
+  failureTitle: 'Listens for the `unload` event',
+  /** Description of a Lighthouse audit that tells the user why pages should not use the 'unload' event. This is displayed after a user expands the section to see more. 'Learn More' becomes link text to additional documentation. */
+  description: 'The `unload` event does not fire reliably and listening for it can prevent browser optimizations like the back/forward cache. Consider using the `pagehide` or `visibilitychange` events instead. [Learn More](https://developers.google.com/web/updates/2018/07/page-lifecycle-api#legacy-lifecycle-apis-to-avoid)',


url will be updated when we have an article

brendankenny · 2020-07-10T18:32:34Z

lighthouse-core/audits/no-unload-listeners.js

+    /** @type {LH.Audit.Details.Table['items']} */
+    const tableItems = unloadListeners.map(listener => {
+      // Look up scriptId to script URL via the JsUsage artifact.
+      const usageEntry = jsUsageValues.find(usage => usage.scriptId === listener.scriptId);


The protocol doesn't give script URLs for event listeners, just scriptIds, so JsUsage seemed like the best place to get the mapping.

brendankenny · 2020-07-10T18:35:00Z

lighthouse-core/gather/gatherers/unload-listeners.js

+    const {result: {objectId}} = await driver.sendCommand('Runtime.evaluate', {
+      expression: 'window',
+      returnByValue: false,
+    });


if anyone has an easier way of getting window's RemoteObjectId, please let me know :)

brendankenny · 2020-07-10T18:36:03Z

lighthouse-core/gather/gatherers/unload-listeners.js

+    return listener.type === 'pagehide' ||
+      listener.type === 'unload' ||
+      listener.type === 'visibilitychange';


as mentioned in the PR description, this can go back to just unload before landing, but there may be more analysis that we can do with these so leaving it like this for now

lighthouse-core/audits/no-unload-listeners.js

patrickhulce

seems like this could be upgraded to non-draft, nothing showstopping here looks good to me 👍

lighthouse-core/gather/gatherers/unload-listeners.js

lighthouse-core/audits/no-unload-listeners.js

brendankenny · 2020-07-10T19:52:10Z

switched to source-location and updated the PR description with a better example with multiple (first and third party) unload handlers

brendankenny · 2020-07-10T20:21:42Z

done and done

patrickhulce

a basic unit test for the audit linking up the script ID to the network would be nice

LGTM otherwise 🎉

lighthouse-core/audits/no-unload-listeners.js

lighthouse-core/test/results/sample_v2.json

brendankenny

a basic unit test for the audit linking up the script ID to the network would be nice

yes, definitely, in progress :)

lighthouse-core/audits/no-unload-listeners.js

lighthouse-core/test/results/sample_v2.json

lighthouse-core/audits/no-unload-listeners.js

connorjclark · 2020-07-10T21:32:16Z

lighthouse-core/audits/no-unload-listeners.js

+  /** Descriptive title of a Lighthouse audit that checks if a web page has 'unload' event listeners and finds that it is using them. */
+  failureTitle: 'Listens for the `unload` event',
+  /** Description of a Lighthouse audit that tells the user why pages should not use the 'unload' event. This is displayed after a user expands the section to see more. 'Learn More' becomes link text to additional documentation. */
+  description: 'The `unload` event does not fire reliably and listening for it can prevent browser optimizations like the back/forward cache. Consider using the `pagehide` or `visibilitychange` events instead. [Learn More](https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-unload-event)',


Suggested change

description: 'The `unload` event does not fire reliably and listening for it can prevent browser optimizations like the back/forward cache. Consider using the `pagehide` or `visibilitychange` events instead. [Learn More](https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-unload-event)',

description: 'The `unload` event does not fire reliably and listening for it prevents browser optimizations like the back/forward cache. Consider using the `pagehide` or `visibilitychange` events instead. [Learn More](https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-unload-event)',

?

wellllll, that may be complicated :) I'll wait for @philipwalton and others to come weigh in on everything and we can nail down the exact wording all at once

patrickhulce

LGTM

patrickhulce · 2020-07-13T21:54:20Z

lighthouse-core/config/default-config.js

@@ -552,6 +554,7 @@ const defaultConfig = {
        {id: 'doctype', weight: 1, group: 'best-practices-browser-compat'},
        {id: 'charset', weight: 1, group: 'best-practices-browser-compat'},
        // General Group
+        {id: 'no-unload-listeners', weight: 1, group: 'best-practices-general'},


feels like it could be a diagnostic too, any particular decision insights behind best practices we can leave for future git blame archeologists? :)

feels like it could be a diagnostic too, any particular decision insights behind best practices we can leave for future git blame archeologists? :)

I guess it kind of falls in with uses-long-cache-ttl as something diagnosing performance, just not the performance of any of the metrics just measured (since a Lighthouse run isn't (usually) with a warm cache or on a hit of the back button). I guess I could see the argument that it's still worthwhile to have there, but I chose this as the uncontroversial spot to put it. I don't feel strongly :)

patrickhulce · 2020-07-13T21:56:19Z

lighthouse-core/gather/gatherers/global-listeners.js

+    return listeners
+      .filter(GlobalListeners._filterForUnloadTypes)
+      .map(listener => {
+        const {type, scriptId, lineNumber, columnNumber} = listener;


I'm just noticing this is already back in node-land so serialization isn't the reason for reexposing, just trying to limit what we return?

just trying to limit what we return?

yeah

lighthouse-core/audits/no-unload-listeners.js

lighthouse-core/lib/i18n/locales/en-US.json

paulirish · 2020-07-13T22:44:08Z

lighthouse-core/audits/no-unload-listeners.js

+  /** Descriptive title of a Lighthouse audit that checks if a web page has 'unload' event listeners and finds that it is using them. */
+  failureTitle: 'Listens for the `unload` event',
+  /** Description of a Lighthouse audit that tells the user why pages should not use the 'unload' event. This is displayed after a user expands the section to see more. 'Learn More' becomes link text to additional documentation. */
+  description: 'The `unload` event does not fire reliably and listening for it can prevent browser optimizations like the back/forward cache. Consider using the `pagehide` or `visibilitychange` events instead. [Learn More](https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-unload-event)',


https://webkit.org/blog/427/webkit-page-cache-i-the-basics/ and https://webkit.org/blog/516/webkit-page-cache-ii-the-unload-event/ cover it as well. also shout out to Dojo for addressing this audit 11 years ago. :)

paulirish · 2020-07-13T22:45:55Z

lighthouse-core/audits/no-unload-listeners.js

+  /** Descriptive title of a Lighthouse audit that checks if a web page has 'unload' event listeners and finds none. */
+  title: 'Avoids `unload` event listeners',
+  /** Descriptive title of a Lighthouse audit that checks if a web page has 'unload' event listeners and finds that it is using them. */
+  failureTitle: 'Listens for the `unload` event',


Alt wording would be "Registers an unload listener". I like it a tad more, but, not a huge deal.

Alt wording would be "Registers an unload listener". I like it a tad more, but, not a huge deal.

I guess it's technically still true, but a little awkward when the page registers multiple unload listeners (plural).

lighthouse-core/gather/gatherers/global-listeners.js

ref GoogleChrome/web-vitals#68 won't fail the new [`no-unload-listeners`](GoogleChrome/lighthouse#11085) Lighthouse audit.

brendankenny requested a review from a team as a code owner July 10, 2020 18:27

brendankenny requested review from patrickhulce and removed request for a team July 10, 2020 18:27

googlebot added the cla: yes label Jul 10, 2020

devtools-bot assigned patrickhulce Jul 10, 2020

devtools-bot added the waiting4reviewer label Jul 10, 2020

brendankenny marked this pull request as draft July 10, 2020 18:29

brendankenny commented Jul 10, 2020

View reviewed changes

connorjclark reviewed Jul 10, 2020

View reviewed changes

lighthouse-core/audits/no-unload-listeners.js Outdated Show resolved Hide resolved

vercel bot deployed to Preview July 10, 2020 19:00 View deployment

patrickhulce reviewed Jul 10, 2020

View reviewed changes

lighthouse-core/gather/gatherers/unload-listeners.js Outdated Show resolved Hide resolved

lighthouse-core/audits/no-unload-listeners.js Outdated Show resolved Hide resolved

patrickhulce added waiting4committer and removed waiting4reviewer labels Jul 10, 2020

vercel bot deployed to Preview July 10, 2020 19:46 View deployment

brendankenny marked this pull request as ready for review July 10, 2020 19:47

vercel bot deployed to Preview July 10, 2020 20:21 View deployment

patrickhulce reviewed Jul 10, 2020

View reviewed changes

lighthouse-core/audits/no-unload-listeners.js Outdated Show resolved Hide resolved

lighthouse-core/test/results/sample_v2.json Outdated Show resolved Hide resolved

brendankenny commented Jul 10, 2020

View reviewed changes

lighthouse-core/audits/no-unload-listeners.js Outdated Show resolved Hide resolved

lighthouse-core/test/results/sample_v2.json Outdated Show resolved Hide resolved

vercel bot deployed to Preview July 10, 2020 21:10 View deployment

connorjclark reviewed Jul 10, 2020

View reviewed changes

lighthouse-core/audits/no-unload-listeners.js Outdated Show resolved Hide resolved

connorjclark reviewed Jul 10, 2020

View reviewed changes

vercel bot deployed to Preview July 10, 2020 21:42 View deployment

brendankenny added waiting4reviewer and removed waiting4committer labels Jul 10, 2020

brendankenny force-pushed the unloadlisteners branch from 152b438 to 7558d39 Compare July 13, 2020 14:43

vercel bot deployed to Preview July 13, 2020 14:43 View deployment

patrickhulce approved these changes Jul 13, 2020

View reviewed changes

patrickhulce removed the waiting4reviewer label Jul 13, 2020

patrickhulce added the waiting4committer label Jul 13, 2020

philipwalton reviewed Jul 13, 2020

View reviewed changes

lighthouse-core/audits/no-unload-listeners.js Outdated Show resolved Hide resolved

lighthouse-core/lib/i18n/locales/en-US.json Outdated Show resolved Hide resolved

paulirish reviewed Jul 13, 2020

View reviewed changes

vercel bot deployed to Preview July 14, 2020 21:29 View deployment

brendankenny added 7 commits July 21, 2020 10:12

new_audit: add no-unload-listeners audit

c29c7d3

strings and smoke test

73738de

switch to source-location details

784c6bd

GlobalListeners

1fcaf1d

fixes

ff2efc3

add no-unload unit tests

4e71cb3

feedback

e5685e1

brendankenny force-pushed the unloadlisteners branch from 0549f57 to e5685e1 Compare July 21, 2020 14:16

vercel bot deployed to Preview July 21, 2020 14:16 View deployment

brendankenny merged commit ca5b892 into master Jul 21, 2020

brendankenny deleted the unloadlisteners branch July 21, 2020 19:37

This was referenced Jul 24, 2020

Unload event listeners should not be used GoogleChrome/web-vitals#67

Closed

Remove the unload listener GoogleChrome/web-vitals#68

Merged

connorjclark added the new_audit label Aug 24, 2020

omBratteng mentioned this pull request Sep 11, 2020

Upgrade web-vitals vercel/next.js#17028

Merged

kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Sep 12, 2020

Upgrade web-vitals (#17028)

5f36f46

ref GoogleChrome/web-vitals#68 won't fail the new [`no-unload-listeners`](GoogleChrome/lighthouse#11085) Lighthouse audit.

HitoriSensei pushed a commit to HitoriSensei/next.js that referenced this pull request Sep 26, 2020

Upgrade web-vitals (vercel#17028)

91a0ac6

ref GoogleChrome/web-vitals#68 won't fail the new [`no-unload-listeners`](GoogleChrome/lighthouse#11085) Lighthouse audit.

beaufortfrancois mentioned this pull request Oct 30, 2020

Update web-vitals package to 0.2.4 GoogleChrome/web.dev#4145

Merged

chrisdavidmills mentioned this pull request Jan 20, 2021

Make the sendBeacon article match reality (including adding a best-practice example at least) mdn/content#1521

Closed

rakina mentioned this pull request Sep 13, 2021

Proposal: Disallow running unload handlers WICG/document-policy#39

Open

brendankenny mentioned this pull request Dec 14, 2021

core: move no-unload-listeners to perf category #13497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new_audit: add no-unload-listeners audit #11085

new_audit: add no-unload-listeners audit #11085

brendankenny commented Jul 10, 2020 •

edited

Loading

brendankenny Jul 10, 2020

brendankenny Jul 10, 2020

brendankenny Jul 10, 2020

brendankenny Jul 10, 2020

patrickhulce left a comment

brendankenny commented Jul 10, 2020

brendankenny commented Jul 10, 2020

patrickhulce left a comment

brendankenny left a comment

connorjclark Jul 10, 2020

brendankenny Jul 10, 2020

patrickhulce left a comment

patrickhulce Jul 13, 2020

brendankenny Jul 14, 2020

patrickhulce Jul 13, 2020

brendankenny Jul 14, 2020

paulirish Jul 13, 2020

paulirish Jul 13, 2020

brendankenny Jul 14, 2020

	description: 'The `unload` event does not fire reliably and listening for it can prevent browser optimizations like the back/forward cache. Consider using the `pagehide` or `visibilitychange` events instead. [Learn More](https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-unload-event)',
	description: 'The `unload` event does not fire reliably and listening for it prevents browser optimizations like the back/forward cache. Consider using the `pagehide` or `visibilitychange` events instead. [Learn More](https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-unload-event)',

new_audit: add no-unload-listeners audit #11085

new_audit: add no-unload-listeners audit #11085

Conversation

brendankenny commented Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny commented Jul 10, 2020

brendankenny commented Jul 10, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jul 10, 2020 •

edited

Loading