Fix: Limit false positives for async stylesheets #1389
Conversation
Attempts to reduce false positive rate for tags blocking first paint. Whitelists tags that were added via script or whose rel/async/disabled attributes indicated asynchronous behavior at any point in their lifecycle. Addresses #1317
|
cc @gauntface |
| } | ||
|
|
||
| window.__asyncLinks = window.__asyncLinks || {}; | ||
| setInterval(checkForLinks, 100); |
There was a problem hiding this comment.
couldn't use documentUpdated because the granularity was too coarse
There was a problem hiding this comment.
also has the unfortunate wrinkle that if a blocking script occurs in the head before the stylesheet and the script takes longer than the stylesheet to load and execute the querySelector will return nothing and we will have missed the window to identify it as a preloaded stylesheet
|
I'm not 100% sure what this is doing. Is it looking at the initial page, noting the stylesheets following the loadCSS usage of If so, how would this behave with developers that use loadCSS like so: My personal site waits for page load before adding the link tag with javascript, which may or may not be the same as loadCSS(). |
|
@gauntface this first step attacks the problem from two angles.
|
|
LGTM from me - Thanks for this @patrickhulce. Seems like a good solution to me. |
| @@ -21,6 +21,21 @@ const Gatherer = require('../gatherer'); | |||
| /* global document,window */ | |||
|
|
|||
There was a problem hiding this comment.
can you add a big @fileoverview comment block at the top that explains how this gatherer works?
| function saveAsyncLinks() { | ||
| function checkForLinks() { | ||
| document.querySelectorAll('link').forEach(link => { | ||
| if (link.rel === 'preload' || link.disabled || link.async) { |
There was a problem hiding this comment.
link.hasAttribute('async'). Link doesn't have a .async property.
There was a problem hiding this comment.
Though I'm not sure we need to check for this one. async only applies to HTMLImports and I can think of a use case where someone would set async on an import, only to remove it later.
There was a problem hiding this comment.
whoops, good catch. Yeah I'll remove.
| @@ -62,7 +78,11 @@ function collectTagsThatBlockFirstPaint() { | |||
| function filteredAndIndexedByUrl(networkRecords) { | |||
| return networkRecords.reduce((prev, record) => { | |||
| // Filter stylesheet, javascript, and html import mimetypes. | |||
| if (/(css|html|script)/.test(record._mimeType)) { | |||
| const isHtml = record._mimeType.indexOf('html') > -1; | |||
| // See https://html.spec.whatwg.org/multipage/semantics.html#interactions-of-styling-and-scripting | |||
There was a problem hiding this comment.
Can you add a supplemental comment to the supporting link?
Attempts to reduce false positive rate for tags blocking first paint. Whitelists tags that were added via script or whose rel/async/disabled attributes indicated asynchronous behavior at any point in their lifecycle. Addresses GoogleChrome#1317
Attempts to reduce false positive rate for tags blocking first paint. Whitelists tags that were added via script or whose rel/async/disabled attributes indicated asynchronous behavior at any point in their lifecycle.
Addresses #1317