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.

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

new-audit(font-display): enforce font-display optional #3831

Merged
merged 22 commits into from
Feb 7, 2018
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
34 changes: 34 additions & 0 deletions lighthouse-cli/test/fixtures/perf/fonts.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<html>
<head>
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<style>
@font-face {
font-family: 'Lobster';
font-style: normal;
font-weight: 400;
src: local('Lobster'), url('./lobster-v20-latin-regular.eot?#iefix') format('eot'), url('./lobster-v20-latin-regular.woff2') format('woff2');
}
@font-face {
font-family: 'Lobster Two';
font-style: normal;
font-weight: 700;
font-display: optional;
src: local("Lobster Two"), url("./lobster-two-v10-latin-700.woff2?delay=4000") format('woff2');
}
.webfont {
font-family: Lobster, sans-serif;
}
strong.webfont {
font-family: Lobster Two, sans-serif;
}
.nofont {
font-family: Unknown, sans-serif;
}
</style>
</head>
<body>
<p class="webfont">Let's load some sweet webfonts...</p>
<p><strong class="webfont">Let's load some sweet webfonts...</strong></p>
<p class"nofont">Some lovely text that uses the fallback font</p>
</body>
</html>
Binary file not shown.
Binary file not shown.
15 changes: 15 additions & 0 deletions lighthouse-cli/test/smokehouse/perf/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,19 @@ module.exports = [
},
},
},
{
initialUrl: 'http://localhost:10200/perf/fonts.html',
url: 'http://localhost:10200/perf/fonts.html',
audits: {
'font-display': {
score: false,
rawValue: false,
details: {
items: {
length: 1,
},
},
},
},
},
];
6 changes: 4 additions & 2 deletions lighthouse-cli/test/smokehouse/smokehouse.js
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,10 @@ const cli = yargs
'config-path': 'The path to the config JSON file',
'expectations-path': 'The path to the expected audit results file',
})
.array('save-assets-path')
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this actually used anywhere anymore? I think I reverted the original uses of it

Copy link
Collaborator Author

@wardpeet wardpeet Feb 4, 2018

Choose a reason for hiding this comment

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

ah nice :) should I create a PR to remove save-assets-path everywhere in smokehouse?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this is already done :) I can't find any other uses on master

.default('config-path', DEFAULT_CONFIG_PATH)
.default('expectations-path', DEFAULT_EXPECTATIONS_PATH)
.default('save-assets-path', [])
.argv;

const configPath = resolveLocalOrCwd(cli['config-path']);
Expand All @@ -276,9 +278,9 @@ const expectations = require(resolveLocalOrCwd(cli['expectations-path']));
// reporting result.
let passingCount = 0;
let failingCount = 0;
expectations.forEach(expected => {
expectations.forEach((expected, index) => {
console.log(`Checking '${expected.initialUrl}'...`);
const results = runLighthouse(expected.initialUrl, configPath);
const results = runLighthouse(expected.initialUrl, configPath, cli['save-assets-path'][index]);
const collated = collateResults(results, expected);
const counts = report(collated);
passingCount += counts.passed;
Expand Down
80 changes: 80 additions & 0 deletions lighthouse-core/audits/font-display.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.
* 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 http://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.
*/
'use strict';

const Audit = require('./audit');
const Util = require('../report/v2/renderer/util');
const WebInspector = require('../lib/web-inspector');
const allowedFontFaceDisplays = ['block', 'fallback', 'optional', 'swap'];

class FontDisplay extends Audit {
/**
* @return {!AuditMeta}
*/
static get meta() {
return {
name: 'font-display',
description: 'All text remains visible during webfont loads',
failureDescription: 'Avoid invisible text while webfonts are loading',
helpText: 'Leverage the font-display CSS feature to ensure text is user-visible while ' +
'webfonts are loading.',
requiredArtifacts: ['devtoolsLogs', 'Fonts'],
};
}

/**
* @param {!Artifacts} artifacts
* @return {!AuditResult}
*/
static audit(artifacts) {
const devtoolsLogs = artifacts.devtoolsLogs[this.DEFAULT_PASS];
const fontFaces = artifacts.Fonts;

// Filter font-faces that do not have a display tag with optional or swap
const fontsWithoutProperDisplay = fontFaces.filter(fontFace =>
!fontFace.display || !allowedFontFaceDisplays.includes(fontFace.display)
);

return artifacts.requestNetworkRecords(devtoolsLogs).then((networkRecords) => {
const results = networkRecords.filter(record => {
const isFont = record._resourceType === WebInspector.resourceTypes.Font;

return isFont;
})
.filter(fontRecord => {
// find the fontRecord of a font
return !!fontsWithoutProperDisplay.find(fontFace => {
return fontFace.src.find(src => fontRecord.url === src);
});
})
// calculate wasted time
.map(record => {
// In reality the end time should be calculated with paint time included
// all browsers wait 3000ms to block text so we make sure 3000 is our max wasted time
const wastedTime = Math.min((record._endTime - record._startTime) * 1000, 3000);

return {
url: record.url,
wastedTime: Util.formatMilliseconds(wastedTime, 1),
};
});

const headings = [
{key: 'url', itemType: 'url', text: 'Font URL'},
{key: 'wastedTime', itemType: 'text', text: 'Font download time'},
];
const details = Audit.makeTableDetails(headings, results);

return {
score: results.length === 0,
rawValue: results.length === 0,
details,
};
});
}
}

module.exports = FontDisplay;
3 changes: 3 additions & 0 deletions lighthouse-core/config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ module.exports = {
'seo/hreflang',
'seo/embedded-content',
'seo/canonical',
'fonts',
],
},
{
Expand Down Expand Up @@ -98,6 +99,7 @@ module.exports = {
'deprecations',
'mainthread-work-breakdown',
'bootup-time',
'font-display',
'manual/pwa-cross-browser',
'manual/pwa-page-transitions',
'manual/pwa-each-page-has-url',
Expand Down Expand Up @@ -285,6 +287,7 @@ module.exports = {
{id: 'bootup-time', weight: 0, group: 'perf-info'},
{id: 'screenshot-thumbnails', weight: 0},
{id: 'mainthread-work-breakdown', weight: 0, group: 'perf-info'},
{id: 'font-display', weight: 0, group: 'perf-info'},
],
},
'pwa': {
Expand Down
158 changes: 158 additions & 0 deletions lighthouse-core/gather/gatherers/fonts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.
* 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 http://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.
*/
'use strict';

const Gatherer = require('./gatherer');
const fontFaceDescriptors = [
'display',
'family',
'featureSettings',
'stretch',
'style',
'unicodeRange',
'variant',
'weight',
];

/* eslint-env browser*/
/**
* Collect applied webfont data from `document.fonts`
* @param {string[]}
* @return {{}}
*/
/* istanbul ignore next */
function getAllLoadedFonts(descriptors) {
const getFont = fontFace => {
const fontRule = {};
descriptors.forEach(descriptor => {
fontRule[descriptor] = fontFace[descriptor];
});

return fontRule;
};

return document.fonts.ready.then(() => {
return Array.from(document.fonts).filter(fontFace => fontFace.status === 'loaded')
.map(getFont);
});
}

/**
* Collect authored webfont data from the `CSSFontFaceRule`s present in document.styleSheets
* @return {{}}
*/
/* istanbul ignore next */
function getFontFaceFromStylesheets() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's throw the /* istanbul ignore next */ on these like paul just did in #4396

/**
* Get full data about each CSSFontFaceRule within a styleSheet object
* @param {StyleSheet} stylesheet
* @return {{}}
*/
function getSheetsFontFaces(stylesheet) {
const fontUrlRegex = 'url\\((?:")([^"]+)(?:"|\')\\)';
const fontFaceRules = [];
if (stylesheet.cssRules) {
for (const rule of stylesheet.cssRules) {
if (rule instanceof CSSFontFaceRule) {
const fontsObject = {
display: rule.style.fontDisplay || 'auto',
family: rule.style.fontFamily.replace(/"|'/g, ''),
stretch: rule.style.fontStretch || 'normal',
style: rule.style.fontStyle || 'normal',
weight: rule.style.fontWeight || 'normal',
Copy link
Member

Choose a reason for hiding this comment

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

from https://github.com/GoogleChrome/lighthouse/pull/3831/files/bf0d8e3cc3c9a16b93ff26b59048563193db85be#r159987426 i think you still need to collect unicodeRange fontVariant and fontFeatureSettings from the rule.style as well.

Copy link
Member

Choose a reason for hiding this comment

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

lgtm

variant: rule.style.fontVariant || 'normal',
unicodeRange: rule.style.unicodeRange || 'U+0-10FFFF',
featureSettings: rule.style.featureSettings || 'normal',
src: [],
};

if (rule.style.src) {
const matches = rule.style.src.match(new RegExp(fontUrlRegex, 'g'));
if (matches) {
fontsObject.src = matches.map(match => {
const res = new RegExp(fontUrlRegex).exec(match);
return new URL(res[1], location.href).href;
});
}
}

fontFaceRules.push(fontsObject);
}
}
}

return fontFaceRules;
}

/**
* Provided a <link rel=stylesheet> element, it attempts to reload the asset with CORS headers.
* Without CORS headers, a cross-origin stylesheet will have node.styleSheet.cssRules === null.
* @param {Element} oldNode
* @return {<!Promise>}
*/
function loadStylesheetWithCORS(oldNode) {
const newNode = oldNode.cloneNode(true);

return new Promise(resolve => {
newNode.addEventListener('load', function onload() {
newNode.removeEventListener('load', onload);
resolve(getFontFaceFromStylesheets());
});
newNode.crossOrigin = 'anonymous';
oldNode.parentNode.insertBefore(newNode, oldNode);
oldNode.remove();
});
}

const promises = [];
// Get all loaded stylesheets
for (const stylesheet of document.styleSheets) {
try {
// Cross-origin stylesheets don't expose cssRules by default. We reload them w/ CORS headers.
if (stylesheet.cssRules === null && stylesheet.href && stylesheet.ownerNode &&
!stylesheet.ownerNode.crossOrigin) {
promises.push(loadStylesheetWithCORS(stylesheet.ownerNode));
} else {
promises.push(Promise.resolve(getSheetsFontFaces(stylesheet)));
}
} catch (err) {
promises.push(loadStylesheetWithCORS(stylesheet.ownerNode));
}
}
// Flatten results
return Promise.all(promises).then(fontFaces => [].concat(...fontFaces));
}
/* eslint-env node */

class Fonts extends Gatherer {
_findSameFontFamily(fontFace, fontFacesList) {
return fontFacesList.find(fontItem => {
return !fontFaceDescriptors.find(descriptor => {
return fontFace[descriptor] !== fontItem[descriptor];
});
});
}

afterPass({driver}) {
return Promise.all(
[
driver.evaluateAsync(`(()=>{`
+ `const args = ${JSON.stringify(fontFaceDescriptors)};`
+ `return (${getAllLoadedFonts.toString()})(args);})()`),
Copy link
Collaborator

Choose a reason for hiding this comment

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

could stringify outside the eval just to make the call as similar as possible to the other :)

const args = JSON.stringify(...)
return Promise.all([
  driver.evaluateAsync(`(${getAllLoadedFonts.toString()})(${args})`),
  driver.evaluateAsync(`(${getFontFaceFromStylesheets.toString()})()`),

driver.evaluateAsync(`(${getFontFaceFromStylesheets.toString()})()`),
]
).then(([loadedFonts, fontFaces]) => {
return loadedFonts.map(fontFace => {
const fontFaceItem = this._findSameFontFamily(fontFace, fontFaces);
fontFace.src = fontFaceItem.src || [];
Copy link
Member

Choose a reason for hiding this comment

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

running this on paulirish.com i'm getting an exception here that fontFaceItem is undefined.. https://sentry.io/google-lighthouse/lighthouse/issues/410603773/

to help debug, the call into _findSameFontFamily has these two arguments:

{ fontFacesList: [] }
{ fontFace:
   { display: 'auto',
     family: 'Droid Sans',
     stretch: 'normal',
     style: 'normal',
     weight: '400' } }

Copy link
Member

Choose a reason for hiding this comment

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

figured it out and wrote the patch. :)

diff --git a/lighthouse-core/gather/gatherers/fonts.js b/lighthouse-core/gather/gatherers/fonts.js
index 9b3f82bc..363f8217 100644
--- a/lighthouse-core/gather/gatherers/fonts.js
+++ b/lighthouse-core/gather/gatherers/fonts.js
@@ -23,7 +23,11 @@ function getAllLoadedFonts() {
   });
 }
 
 function getFontFaceFromStylesheets() {
+  let resolve;
+  const promise = new Promise(fulfill => { resolve = fulfill; });
+
   function resolveUrl(url) {
     const link = document.createElement('a');
     link.href = url;
@@ -34,8 +38,24 @@ function getFontFaceFromStylesheets() {
   const fontUrlRegex = new RegExp('url\\((?:"|\')([^"]+)(?:"|\')\\)');
   const fontFaceRules = [];
   // get all loaded stylesheets
   for (let sheet = 0; sheet < document.styleSheets.length; sheet++) {
     const stylesheet = document.styleSheets[sheet];
+
+    // Cross-origin stylesheets don't expose cssRules by default. We reload them with CORS headers.
+    if (stylesheet.cssRules === null && stylesheet.href && stylesheet.ownerNode && !stylesheet.ownerNode.crossOrigin) {
+        const oldNode = stylesheet.ownerNode;
+        const newNode = oldNode.cloneNode(true);
+        newNode.addEventListener('load', function onload(){
+            newNode.removeEventListener('load', onload);
+            resolve(getFontFaceFromStylesheets());
+        });
+        newNode.crossOrigin = 'anonymous';
+        oldNode.parentNode.insertBefore(newNode, oldNode);
+        oldNode.remove();
+        return promise;
+    }
+
     for (let i = 0; stylesheet.cssRules && i < stylesheet.cssRules.length; i++) {
       var rule = stylesheet.cssRules[i];
 
@@ -61,7 +81,7 @@ function getFontFaceFromStylesheets() {
     }
   }
 
-  return fontFaceRules;
+  return Promise.resolve(fontFaceRules);
 }
 /* eslint-enable */
 

Copy link
Member

Choose a reason for hiding this comment

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

separately i think you'll need some error handling for the case that we have a mismatch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice catch, let me cleanup this audit a bit :) it was just a WIP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did a bit differently as I think the way I did it is more readable. I also got a security exception when running stylesheet.cssRules so I used a try catch but also an if just to be sure to cover all the things


return fontFace;
});
});
}
}

module.exports = Fonts;
Loading