Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(render-blocking): handle amp-style stylesheets #4555

Merged
merged 6 commits into from
Feb 26, 2018
Merged

core(render-blocking): handle amp-style stylesheets #4555

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d08e3f
core(render-blocking): handle amp-style stylesheets
patrickhulce Feb 15, 2018
3df2c9b
lint
patrickhulce Feb 15, 2018
672ee72
add comment to dbw tester while loop
patrickhulce Feb 15, 2018
32cd97f
feedback
patrickhulce Feb 22, 2018
6b3789a
feedback 2
patrickhulce Feb 22, 2018
b5092b6
better smoketests
patrickhulce Feb 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@
<link rel="stylesheet" href="./dbw_tester.css?scriptActivated&delay=200"> <!-- PASS: initiator is script -->
</template>

<!-- AMP-style stylesheet script -->
<!-- based on https://github.com/ampproject/amphtml/blob/38f1bdf4cc385f9a25cf2979abf215952880b876/src/font-stylesheet-timeout.js#L87-L103 -->
<script type="text/javascript">
setTimeout(() => {
const stylesheet = document.getElementById('amp-style-link');
stylesheet.media = 'not-matching';
stylesheet.onload = () => stylesheet.media = 'screen';
stylesheet.parentNode.insertBefore(stylesheet, stylesheet.nextSibling);
}, 500);
</script>

<!-- Note: these will only fail when using the static-server.js, which supports the ?delay=true param.
If you're using your own server, the resource will load instantly and the
stylesheets will be ignored for being below the threshold. -->
Expand All @@ -27,9 +38,11 @@
<link rel="import" href="./dbw_partial_a.html?delay=200"> <!-- FAIL -->
<link rel="import" href="./dbw_partial_b.html?delay=200&isasync" async> <!-- PASS -->

<!-- FAIL: render blocking but capped at 500ms -->
<link id="amp-style-link" rel="stylesheet" href="./dbw_tester.css?delay=3000&capped">
<!-- PASS: preload that's activated later does not block rendering. -->
<link rel="preload" href="./dbw_tester.css?delay=2000&async=true" as="style" onload="this.rel = 'stylesheet'">
<!-- PASS: async stylesheet that loads after FCP -->
<!-- PASS: async stylesheet -->
<link rel="stylesheet" href="./dbw_tester.css?delay=3000&async=true" disabled onload="this.disabled = false">

<!-- FAIL: block rendering -->
Expand Down Expand Up @@ -90,6 +103,9 @@
</style>
</template>

<!-- Force FCP to be ~5s out, necessary to make sure our render-blocking audits still work -->
<!-- even when other forces were responsible for delaying the render. -->
<script> while (Date.now() - performance.timing.responseStart < 5000) ; </script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use an add'l stylesheet with delay=5000 instead? Seems more natural.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately, no. since the only way we assert that we didn't flag the amp stylesheet is by asserting the overall render-blocking stylesheets value is <3s. in the future details, but extendedInfo-style world this will be easier :)

</head>
<body>

Expand Down
9 changes: 5 additions & 4 deletions lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ module.exports = [
extendedInfo: {
value: {
results: {
length: 16,
length: 17,
},
},
},
details: {
items: {
length: 16,
length: 17,
},
},
},
Expand Down Expand Up @@ -71,16 +71,17 @@ module.exports = [
},
'link-blocking-first-paint': {
score: 0,
rawValue: '<3000',
extendedInfo: {
value: {
results: {
length: 4,
length: 5,
},
},
},
details: {
items: {
length: 4,
length: 5,
},
},
},
Expand Down
83 changes: 61 additions & 22 deletions lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,39 @@
*/

/**
* @fileoverview
* Identifies stylesheets, HTML Imports, and scripts that potentially block
* the first paint of the page by running several scripts in the page context.
* Candidate blocking tags are collected by querying for all script tags in
* the head of the page and all link tags that are either matching media
* stylesheets or non-async HTML imports. These are then compared to the
* network requests to ensure they were initiated by the parser and not
* injected with script. To avoid false positives from strategies like
* (http://filamentgroup.github.io/loadCSS/test/preload.html), a separate
* script is run to flag all links that at one point were rel=preload.
*/
* @fileoverview
* Identifies stylesheets, HTML Imports, and scripts that potentially block
* the first paint of the page by running several scripts in the page context.
* Candidate blocking tags are collected by querying for all script tags in
* the head of the page and all link tags that are either matching media
* stylesheets or non-async HTML imports. These are then compared to the
* network requests to ensure they were initiated by the parser and not
* injected with script. To avoid false positives from strategies like
* (http://filamentgroup.github.io/loadCSS/test/preload.html), a separate
* script is run to flag all links that at one point were rel=preload.
*/

'use strict';

const Gatherer = require('../gatherer');

/* global document,window */
/* global document,window,HTMLLinkElement */

function installMediaListener() {
window.___linkMediaChanges = [];
Object.defineProperty(HTMLLinkElement.prototype, 'media', {
set: function(val) {
window.___linkMediaChanges.push({
url: this.href,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming bikesheds?

url => href
value => media
time => msSinceHTMLEnd
matches == matches

value: val,
time: Date.now() - window.performance.timing.responseEnd,
matches: window.matchMedia(val).matches,
});

return this.setAttribute('media', val);
},
});
}

/* istanbul ignore next */
function collectTagsThatBlockFirstPaint() {
Expand All @@ -30,17 +46,19 @@ function collectTagsThatBlockFirstPaint() {
const tagList = [...document.querySelectorAll('link, head script[src]')]
.filter(tag => {
if (tag.tagName === 'SCRIPT') {
return !tag.hasAttribute('async') &&
!tag.hasAttribute('defer') &&
!/^data:/.test(tag.src) &&
tag.getAttribute('type') !== 'module';
return (
!tag.hasAttribute('async') &&
!tag.hasAttribute('defer') &&
!/^data:/.test(tag.src) &&
tag.getAttribute('type') !== 'module'
);
}

// Filter stylesheet/HTML imports that block rendering.
// https://www.igvita.com/2012/06/14/debunking-responsive-css-performance-myths/
// https://www.w3.org/TR/html-imports/#dfn-import-async-attribute
const blockingStylesheet = (tag.rel === 'stylesheet' &&
window.matchMedia(tag.media).matches && !tag.disabled);
const blockingStylesheet =
tag.rel === 'stylesheet' && window.matchMedia(tag.media).matches && !tag.disabled;
const blockingImport = tag.rel === 'import' && !tag.hasAttribute('async');
return blockingStylesheet || blockingImport;
})
Expand All @@ -53,6 +71,7 @@ function collectTagsThatBlockFirstPaint() {
rel: tag.rel,
media: tag.media,
disabled: tag.disabled,
mediaChanges: window.___linkMediaChanges.filter(item => item.url === tag.href),
};
});
resolve(tagList);
Expand Down Expand Up @@ -99,17 +118,30 @@ class TagsBlockingFirstPaint extends Gatherer {

static findBlockingTags(driver, networkRecords) {
const scriptSrc = `(${collectTagsThatBlockFirstPaint.toString()}())`;
const firstRequestEndTime = networkRecords.reduce(
(min, record) => Math.min(min, record._endTime),
Infinity
);
return driver.evaluateAsync(scriptSrc).then(tags => {
const requests = filteredAndIndexedByUrl(networkRecords);

return tags.reduce((prev, tag) => {
const request = requests[tag.url];
if (request && !request.isLinkPreload) {
// Even if the request was initially blocking or appeared to be blocking once the
// page was loaded, the media attribute could have been changed during load, capping the
// amount of time it was render blocking. See https://github.com/GoogleChrome/lighthouse/issues/2832.
const nonMatchingMediaChangeTimes = (tag.mediaChanges || [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timesMediaSetToNonBlocking ?

.filter(change => !change.matches)
.map(change => change.time);
const earliestNonBlockingTime = Math.min(...nonMatchingMediaChangeTimes);
const lastTimeResourceWasBlocking = firstRequestEndTime + earliestNonBlockingTime / 1000;

prev.push({
tag,
transferSize: request.transferSize || 0,
startTime: request.startTime,
endTime: request.endTime,
endTime: Math.min(request.endTime, lastTimeResourceWasBlocking),
});

// Prevent duplicates from showing up again
Expand All @@ -122,12 +154,19 @@ class TagsBlockingFirstPaint extends Gatherer {
}

/**
* @param {!Object} options
* @param {!Object} context
*/
beforePass(context) {
return context.driver.evaluteScriptOnNewDocument(`(${installMediaListener.toString()})()`);
}

/**
* @param {!Object} context
* @param {{networkRecords: !Array<!NetworkRecord>}} tracingData
* @return {!Array<{tag: string, transferSize: number, startTime: number, endTime: number}>}
*/
afterPass(options, tracingData) {
return TagsBlockingFirstPaint.findBlockingTags(options.driver, tracingData.networkRecords);
afterPass(context, tracingData) {
return TagsBlockingFirstPaint.findBlockingTags(context.driver, tracingData.networkRecords);
}
}

Expand Down