From 3eca575a6eeabfccfbc500b4a1e668e4fc5f670d Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 3 Oct 2018 10:35:03 -0500 Subject: [PATCH 1/3] core: remove fonts gatherer --- .../test/cli/__snapshots__/index-test.js.snap | 3 - lighthouse-core/audits/font-display.js | 131 ++++++---- lighthouse-core/config/default-config.js | 1 - lighthouse-core/gather/gatherers/fonts.js | 237 ------------------ .../test/audits/font-display-test.js | 137 +++++----- .../test/gather/gatherers/fonts-test.js | 118 --------- 6 files changed, 161 insertions(+), 466 deletions(-) delete mode 100644 lighthouse-core/gather/gatherers/fonts.js delete mode 100644 lighthouse-core/test/gather/gatherers/fonts-test.js diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap index 2c3f4e57fa85..0806483569f2 100644 --- a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap +++ b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap @@ -1104,9 +1104,6 @@ Object { Object { "path": "seo/robots-txt", }, - Object { - "path": "fonts", - }, ], "networkQuietThresholdMs": 1000, "passName": "defaultPass", diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js index a8115628d44d..facbe4b789c4 100644 --- a/lighthouse-core/audits/font-display.js +++ b/lighthouse-core/audits/font-display.js @@ -6,8 +6,10 @@ 'use strict'; const Audit = require('./audit'); -const NetworkRequest = require('../lib/network-request'); -const allowedFontFaceDisplays = ['block', 'fallback', 'optional', 'swap']; +const URL = require('../lib/url-shim').URL; +const PASSING_FONT_DISPLAY_REGEX = /block|fallback|optional|swap/; +const CSS_URL_REGEX = /url\((.*?)\)/; +const CSS_URL_GLOBAL_REGEX = new RegExp(CSS_URL_REGEX, 'g'); const i18n = require('../lib/i18n/i18n.js'); const UIStrings = { @@ -16,7 +18,8 @@ const UIStrings = { /** Title of a diagnostic audit that provides detail on the load of the page's webfonts. Often the text is invisible for seconds before the webfont resource is loaded. This imperative title is shown to users when there is a significant amount of execution time that could be reduced. */ failureTitle: 'Ensure text remains visible during webfont load', /** Description of a Lighthouse audit that tells the user *why* they should use the font-display CSS feature. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ - description: 'Leverage the font-display CSS feature to ensure text is user-visible while ' + + description: + 'Leverage the font-display CSS feature to ensure text is user-visible while ' + 'webfonts are loading. ' + '[Learn more](https://developers.google.com/web/updates/2016/02/font-display).', }; @@ -33,59 +36,91 @@ class FontDisplay extends Audit { title: str_(UIStrings.title), failureTitle: str_(UIStrings.failureTitle), description: str_(UIStrings.description), - requiredArtifacts: ['devtoolsLogs', 'Fonts'], + requiredArtifacts: ['devtoolsLogs', 'CSSUsage', 'URL'], }; } + /** + * + * @param {LH.Artifacts} artifacts + */ + static findPassingFontDisplayDeclarations(artifacts) { + /** @type {Set} */ + const passingURLs = new Set(); + + for (const stylesheet of artifacts.CSSUsage.stylesheets) { + const newlinesStripped = stylesheet.content.replace(/\n/g, ' '); + const fontFaceDeclarations = newlinesStripped.match(/@font-face\s*{(.*?)}/g) || []; + for (const declaration of fontFaceDeclarations) { + const rawFontDisplay = declaration.match(/font-display:(.*?);/); + if (!rawFontDisplay) continue; + const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay[0]); + if (!hasPassingFontDisplay) continue; + + const rawFontURLs = declaration.match(CSS_URL_GLOBAL_REGEX); + if (!rawFontURLs) continue; + + const relativeURLs = rawFontURLs + // @ts-ignore - guaranteed to match from previous regex, pull URL group out + .map(s => s.match(CSS_URL_REGEX)[1].trim()) + // remove any optional quotes before/after + .map(s => { + const firstChar = s.charAt(0); + if (firstChar === s.charAt(s.length - 1) && (firstChar === '"' || firstChar === '\'')) { + return s.substr(1, s.length - 2); + } + + return s; + }); + + const absoluteURLs = relativeURLs.map(url => new URL(url, artifacts.URL.finalUrl)); + + for (const url of absoluteURLs) { + passingURLs.add(url.href); + } + } + } + + return passingURLs; + } + /** * @param {LH.Artifacts} artifacts * @return {Promise} */ - static audit(artifacts) { + static async 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 === NetworkRequest.TYPES.Font; - - return isFont; - }) - .filter(fontRecord => { - // find the fontRecord of a font - return !!fontsWithoutProperDisplay.find(fontFace => { - return !!fontFace.src && !!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 wastedMs = Math.min((record.endTime - record.startTime) * 1000, 3000); - - return { - url: record.url, - wastedMs, - }; - }); - - const headings = [ - {key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)}, - {key: 'wastedMs', itemType: 'ms', text: str_(i18n.UIStrings.columnWastedMs)}, - ]; - const details = Audit.makeTableDetails(headings, results); - - return { - score: Number(results.length === 0), - rawValue: results.length === 0, - details, - }; - }); + const networkRecords = await artifacts.requestNetworkRecords(devtoolsLogs); + const passingFontURLs = FontDisplay.findPassingFontDisplayDeclarations(artifacts); + + const results = networkRecords + // Find all fonts... + .filter(record => record.resourceType === 'Font') + // ...that don't have a passing font-display value + .filter(record => !passingFontURLs.has(record.url)) + .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 wastedMs = Math.min((record.endTime - record.startTime) * 1000, 3000); + + return { + url: record.url, + wastedMs, + }; + }); + + const headings = [ + {key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)}, + {key: 'wastedMs', itemType: 'ms', text: str_(i18n.UIStrings.columnWastedMs)}, + ]; + + const details = Audit.makeTableDetails(headings, results); + + return { + score: Number(results.length === 0), + rawValue: results.length === 0, + details, + }; } } diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js index 2087e6f5f5f7..a6f1a0b5ad02 100644 --- a/lighthouse-core/config/default-config.js +++ b/lighthouse-core/config/default-config.js @@ -74,7 +74,6 @@ const defaultConfig = { 'seo/embedded-content', 'seo/canonical', 'seo/robots-txt', - 'fonts', ], }, { diff --git a/lighthouse-core/gather/gatherers/fonts.js b/lighthouse-core/gather/gatherers/fonts.js deleted file mode 100644 index dc0ae4cb7327..000000000000 --- a/lighthouse-core/gather/gatherers/fonts.js +++ /dev/null @@ -1,237 +0,0 @@ -/** - * @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. - */ -// eslint-disable-next-line spaced-comment -/// -'use strict'; - -const Gatherer = require('./gatherer'); -const Sentry = require('../../lib/sentry'); - -// All the property keys of FontFace where the value is a string and are worth -// using for finding font matches (see _findSameFontFamily). -/** @typedef {'family'|'style'|'weight'|'stretch'|'unicodeRange'|'variant'|'featureSettings'|'display'} FontFaceStringKeys */ -/** @typedef {{err: {message: string, stack?: string}}} FontGatherError */ - -/** @type {Array} */ -const fontFaceDescriptors = [ - 'display', - 'family', - 'featureSettings', - 'stretch', - 'style', - 'unicodeRange', - 'variant', - 'weight', -]; - -/* eslint-env browser*/ -/** - * Collect applied webfont data from `document.fonts` - * @param {Array} descriptors - * @return {Promise>} - */ -/* istanbul ignore next */ -function getAllLoadedFonts(descriptors) { - /** @param {FontFace} fontFace */ - const getFont = fontFace => { - /** @type {Partial} */ - const fontRule = { - src: [], - }; - descriptors.forEach(descriptor => { - fontRule[descriptor] = fontFace[descriptor]; - }); - - return /** @type {LH.Artifacts.Font} */ (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 {Promise>} - */ -/* istanbul ignore next */ -function getFontFaceFromStylesheets() { - /** - * Get full data about each CSSFontFaceRule within a styleSheet object - * @param {CSSStyleSheet} stylesheet - * @return {Array} - */ - function getSheetsFontFaces(stylesheet) { - const fontUrlRegex = 'url\\((?:")([^"]+)(?:"|\')\\)'; - const fontFaceRules = []; - - if (stylesheet.cssRules) { - for (const rule of Array.from(stylesheet.cssRules)) { - if (rule instanceof CSSFontFaceRule) { - const fontsObject = { - // @ts-ignore (currently) non-standard Chrome extension to CSSStyleDeclaration - // See disussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1296373#c4 - display: rule.style.fontDisplay || 'auto', - family: rule.style.fontFamily ? rule.style.fontFamily.replace(/"|'/g, '') : '', - stretch: rule.style.fontStretch || 'normal', - style: rule.style.fontStyle || 'normal', - weight: rule.style.fontWeight || 'normal', - variant: rule.style.fontVariant || 'normal', - // @ts-ignore (currently) non-standard Chrome extension to CSSStyleDeclaration - unicodeRange: rule.style.unicodeRange || 'U+0-10FFFF', - // @ts-ignore (currently) non-standard Chrome extension to CSSStyleDeclaration - featureSettings: rule.style.featureSettings || 'normal', - /** @type {Array} */ - src: [], - }; - - /** @type {string|undefined} */ - // @ts-ignore (currently) non-standard Chrome extension to CSSStyleDeclaration - const src = rule.style.src; - if (src) { - const matches = src.match(new RegExp(fontUrlRegex, 'g')); - if (matches) { - matches.forEach(match => { - const res = new RegExp(fontUrlRegex).exec(match); - if (res) { - fontsObject.src.push(new URL(res[1], location.href).href); - } - }); - } - } - - fontFaceRules.push(fontsObject); - } - } - } - - return fontFaceRules; - } - - /** - * Provided a element, it attempts to reload the asset with CORS headers. - * Without CORS headers, a cross-origin stylesheet will have node.styleSheet.cssRules === null. - * @param {HTMLLinkElement} oldNode - * @return {Promise>} - */ - function loadStylesheetWithCORS(oldNode) { - const newNode = /** @type {HTMLLinkElement} */ (oldNode.cloneNode(true)); - - return new Promise(resolve => { - newNode.addEventListener('load', function onload() { - newNode.removeEventListener('load', onload); - try { - const stylesheet = Array.from(document.styleSheets).find(s => s.ownerNode === newNode); - if (stylesheet) { - const cssStylesheet = /** @type {CSSStyleSheet} */ (stylesheet); - resolve(getSheetsFontFaces(cssStylesheet)); - } else { - resolve([{err: {message: 'Could not load stylesheet with CORS'}}]); - } - } catch (err) { - resolve([{err: {message: err.message, stack: err.stack}}]); - } - }); - newNode.crossOrigin = 'anonymous'; - oldNode.parentNode && oldNode.parentNode.insertBefore(newNode, oldNode); - oldNode.remove(); - - // Give each stylesheet 5s to load before giving up - setTimeout(() => resolve([{err: {message: 'Could not load stylesheet (timeout)'}}]), 5000); - }); - } - - /** @type {Array} */ - const data = []; - /** @type {Array>>} */ - const corsDataPromises = []; - // Get all loaded stylesheets - for (const stylesheet of Array.from(document.styleSheets)) { - const cssStylesheet = /** @type {CSSStyleSheet} */ (stylesheet); - - try { - // cssRules can be null or this access can throw when CORS isn't enabled; throw a matching error message. - if (!cssStylesheet.cssRules) { - throw new Error('Failed to read cssRules'); - } - - data.push(...getSheetsFontFaces(cssStylesheet)); - } catch (err) { - const failedToReadRules = /Failed to read.*cssRules/.test(err.message); - // @ts-ignore - crossOrigin exists if ownerNode is an HTMLLinkElement - const alreadyCORS = !cssStylesheet.ownerNode || !!cssStylesheet.ownerNode.crossOrigin; - - if (failedToReadRules && !alreadyCORS && cssStylesheet.href) { - // Cross-origin stylesheets don't expose cssRules by default. We reload them w/ CORS headers. - const ownerLinkEl = /** @type {HTMLLinkElement} */ (cssStylesheet.ownerNode); - corsDataPromises.push(loadStylesheetWithCORS(ownerLinkEl)); - } else { - // Otherwise this is a legit error we should report back to the gatherer. - data.push({err: {message: err.message, stack: err.stack}}); - } - } - } - // Flatten results - return Promise.all(corsDataPromises).then(corsFontFaces => data.concat(...corsFontFaces)); -} -/* eslint-env node */ - -class Fonts extends Gatherer { - /** - * @param {LH.Artifacts.Font} fontFace - * @param {Array} fontFacesList - * @return {LH.Artifacts.Font|undefined} - */ - _findSameFontFamily(fontFace, fontFacesList) { - return fontFacesList.find(fontItem => { - return !fontFaceDescriptors.find(descriptor => { - return fontFace[descriptor] !== fontItem[descriptor]; - }); - }); - } - - /** - * @param {LH.Gatherer.PassContext} passContext - * @return {Promise} - */ - afterPass(passContext) { - const driver = passContext.driver; - const args = JSON.stringify(fontFaceDescriptors); - /** @type {Promise<[Array, Array]>} */ - const fontData = Promise.all( - [ - driver.evaluateAsync(`(${getAllLoadedFonts.toString()})(${args})`), - driver.evaluateAsync(`(${getFontFaceFromStylesheets.toString()})()`), - ] - ); - return fontData.then(([loadedFonts, fontsAndErrors]) => { - // Filter out errors from retrieving data on font faces. - const fontFaces = /** @type {Array} */ (fontsAndErrors.filter( - fontOrError => !('err' in fontOrError))); - - const firstFontError = fontsAndErrors.find(fontOrError => 'err' in fontOrError); - if (firstFontError) { - // Abuse the type system a bit since `err` property isn't common between types. - const dataError = /** @type {FontGatherError} */ (firstFontError); - if (dataError.err) { - const err = new Error(dataError.err.message); - err.stack = dataError.err.stack || err.stack; - Sentry.captureException(err, {tags: {gatherer: 'Fonts'}, level: 'warning'}); - } - } - - return loadedFonts.map(loadedFont => { - const fontFaceItem = this._findSameFontFamily(loadedFont, fontFaces); - loadedFont.src = (fontFaceItem && fontFaceItem.src) || []; - - return loadedFont; - }); - }); - } -} - -module.exports = Fonts; diff --git a/lighthouse-core/test/audits/font-display-test.js b/lighthouse-core/test/audits/font-display-test.js index 1a836ba4c2a2..7284b3ed2cf7 100644 --- a/lighthouse-core/test/audits/font-display-test.js +++ b/lighthouse-core/test/audits/font-display-test.js @@ -5,88 +5,107 @@ */ 'use strict'; -const NetworkRequest = require('../../lib/network-request'); const Audit = require('../../audits/font-display.js'); const assert = require('assert'); /* eslint-env jest */ -const openSansFont = { - display: 'auto', - family: 'open Sans', - stretch: 'normal', - style: 'normal', - weight: '400', - src: [ - 'https://fonts.gstatic.com/s/opensans/v15/u-WUoqrET9fUeobQW7jkRYX0hVgzZQUfRDuZrPvH3D8.ttf', - 'https://fonts.gstatic.com/s/opensans/v15/u-WUoqrET9fUeobQW7jkRYX0hVgzZQUfRDuZrPvH3D8.woff2', - ], -}; -const openSansFontBold = { - display: 'auto', - family: 'open Sans', - stretch: 'normal', - style: 'normal', - weight: '600', - src: [ - 'https://fonts.gstatic.com/s/opensans/v15/k3k702ZOKiLJc3WVjuplzA7aC6SjiAOpAWOKfJDfVRY.woff2', - ], -}; describe('Performance: Font Display audit', () => { - function getArtifacts(networkRecords, fonts) { - return { + let artifacts; + let networkRecords; + let stylesheet; + + beforeEach(() => { + stylesheet = {content: ''}; + artifacts = { devtoolsLogs: {[Audit.DEFAULT_PASS]: []}, requestNetworkRecords: () => Promise.resolve(networkRecords), - Fonts: fonts, + URL: {finalUrl: 'https://example.com/foo/bar/page'}, + CSSUsage: {stylesheets: [stylesheet]}, }; - } + }); - it('fails when not all fonts have a correct font-display rule', () => { - const webFonts = [ - Object.assign({}, openSansFont, {display: 'block'}), - openSansFontBold, - ]; + it('fails when not all fonts have a correct font-display rule', async () => { + stylesheet.content = ` + @font-face { + src: url("./font-a.woff"); + } + + @font-face { + src: url('../font-b.woff'); + } + + @font-face { + src: url(font.woff); + } + `; - return Audit.audit(getArtifacts([ + networkRecords = [ { - url: openSansFont.src[0], + url: 'https://example.com/foo/bar/font-a.woff', endTime: 3, startTime: 1, - resourceType: NetworkRequest.TYPES.Font, + resourceType: 'Font', }, { - url: openSansFontBold.src[0], - endTime: 3, startTime: 1, - resourceType: NetworkRequest.TYPES.Font, + url: 'https://example.com/foo/font-b.woff', + endTime: 5, startTime: 1, + resourceType: 'Font', }, - ], webFonts)).then(result => { - const items = [{ - url: openSansFontBold.src[0], - wastedMs: 2000, - }]; - assert.strictEqual(result.rawValue, false); - assert.deepEqual(result.details.items, items); - }); - }); + { + url: 'https://example.com/foo/bar/font.woff', + endTime: 2, startTime: 1, + resourceType: 'Font', + }, + ]; - it('passes when all fonts have a correct font-display rule', () => { - const webFonts = [ - Object.assign({}, openSansFont, {display: 'block'}), - Object.assign({}, openSansFontBold, {display: 'fallback'}), + const result = await Audit.audit(artifacts); + const items = [ + {url: networkRecords[0].url, wastedMs: 2000}, + {url: networkRecords[1].url, wastedMs: 3000}, + {url: networkRecords[2].url, wastedMs: 1000}, ]; + assert.strictEqual(result.rawValue, false); + assert.deepEqual(result.details.items, items); + }); + + it('passes when all fonts have a correct font-display rule', async () => { + stylesheet.content = ` + @font-face { + font-display: 'block'; + src: url("./font-a.woff"); + } + + @font-face { + font-display: 'fallback'; + src: url('../font-b.woff'); + } - return Audit.audit(getArtifacts([ + @font-face { + font-display: 'optional'; + src: url(font.woff); + } + `; + + networkRecords = [ { - url: openSansFont.src[0], + url: 'https://example.com/foo/bar/font-a.woff', endTime: 3, startTime: 1, - resourceType: NetworkRequest.TYPES.Font, + resourceType: 'Font', }, { - url: openSansFontBold.src[0], - endTime: 3, startTime: 1, - resourceType: NetworkRequest.TYPES.Font, + url: 'https://example.com/foo/font-b.woff', + endTime: 5, startTime: 1, + resourceType: 'Font', }, - ], webFonts)).then(result => { - assert.strictEqual(result.rawValue, true); - }); + { + url: 'https://example.com/foo/bar/font.woff', + endTime: 2, startTime: 1, + resourceType: 'Font', + }, + ]; + + const result = await Audit.audit(artifacts); + assert.strictEqual(result.rawValue, true); + assert.deepEqual(result.details.items, []); }); }); diff --git a/lighthouse-core/test/gather/gatherers/fonts-test.js b/lighthouse-core/test/gather/gatherers/fonts-test.js deleted file mode 100644 index dda26fff4f22..000000000000 --- a/lighthouse-core/test/gather/gatherers/fonts-test.js +++ /dev/null @@ -1,118 +0,0 @@ -/** - * @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'; - -/* eslint-env jest */ - -const FontsGatherer = require('../../../gather/gatherers/fonts'); -const assert = require('assert'); -let fontsGatherer; - -const openSansFont = { - display: 'auto', - family: 'open Sans', - stretch: 'normal', - style: 'normal', - weight: '400', - variant: 'normal', - unicodeRange: 'U+0-10FFFF', - featureSettings: 'normal', -}; -const openSansFontBold = { - display: 'auto', - family: 'open Sans', - stretch: 'normal', - style: 'normal', - weight: '600', - variant: 'normal', - unicodeRange: 'U+0-10FFFF', - featureSettings: 'normal', -}; - -const openSansFontFaces = []; -openSansFontFaces.push(Object.assign({}, - openSansFont, - { - src: [ - 'https://fonts.gstatic.com/s/opensans/v15/u-WUoqrET9fUeobQW7jkRYX0hVgzZQUfRDuZrPvH3D8.woff2', - ], - } -)); -openSansFontFaces.push(Object.assign({}, - openSansFontBold, - { - src: [ - 'https://fonts.gstatic.com/s/opensans/v15/k3k702ZOKiLJc3WVjuplzA7aC6SjiAOpAWOKfJDfVRY.woff2', - ], - } -)); - -describe('Fonts gatherer', () => { - // Reset the Gatherer before each test. - beforeEach(() => { - fontsGatherer = new FontsGatherer(); - }); - - it('returns an artifact', () => { - return fontsGatherer.afterPass({ - driver: { - evaluateAsync: (code) => { - if (code.includes('getAllLoadedFonts')) { - return Promise.resolve([ - openSansFont, - ]); - } else { - return Promise.resolve(openSansFontFaces); - } - }, - }, - }).then(artifact => { - const expectedArtifact = Object.assign({}, - openSansFont, - { - src: ['https://fonts.gstatic.com/s/opensans/v15/u-WUoqrET9fUeobQW7jkRYX0hVgzZQUfRDuZrPvH3D8.woff2'], - } - ); - - assert.equal(artifact.length, 1); - assert.deepEqual(artifact[0], expectedArtifact); - }); - }); - - it('shouldn\'t break when no fonts are used', function() { - return fontsGatherer.afterPass({ - driver: { - evaluateAsync: (code) => { - if (code.includes('getAllLoadedFonts')) { - return Promise.resolve([]); - } else { - return Promise.resolve(openSansFontFaces); - } - }, - }, - }).then(artifact => { - assert.ok(artifact); - }); - }); - - // some stylesheets are loaded by import rules. document.stylesheets do not capture these. - // this means we can't find the src of a webfont. - it('shouldn\'t break when no font-face rules are found', function() { - return fontsGatherer.afterPass({ - driver: { - evaluateAsync: (code) => { - if (code.includes('getAllLoadedFonts')) { - return Promise.resolve(openSansFontFaces); - } else { - return Promise.resolve([]); - } - }, - }, - }).then(artifact => { - assert.ok(artifact); - }); - }); -}); From 6076cc2487dbe3eee3ef5326a3963de6d621a0a7 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 11 Oct 2018 17:22:08 -0500 Subject: [PATCH 2/3] feedback --- lighthouse-core/audits/font-display.js | 27 +++++++--- .../test/audits/font-display-test.js | 52 +++++++++++++++++++ 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js index facbe4b789c4..975ccc040ef7 100644 --- a/lighthouse-core/audits/font-display.js +++ b/lighthouse-core/audits/font-display.js @@ -11,6 +11,7 @@ const PASSING_FONT_DISPLAY_REGEX = /block|fallback|optional|swap/; const CSS_URL_REGEX = /url\((.*?)\)/; const CSS_URL_GLOBAL_REGEX = new RegExp(CSS_URL_REGEX, 'g'); const i18n = require('../lib/i18n/i18n.js'); +const Sentry = require('../lib/sentry.js'); const UIStrings = { /** Title of a diagnostic audit that provides detail on if all the text on a webpage was visible while the page was loading its webfonts. This descriptive title is shown to users when the amount is acceptable and no user action is required. */ @@ -41,42 +42,52 @@ class FontDisplay extends Audit { } /** - * * @param {LH.Artifacts} artifacts */ static findPassingFontDisplayDeclarations(artifacts) { /** @type {Set} */ const passingURLs = new Set(); + // Go through all the stylesheets to find all @font-face declarations for (const stylesheet of artifacts.CSSUsage.stylesheets) { + // Eliminate newlines so we can more easily scan through with a regex const newlinesStripped = stylesheet.content.replace(/\n/g, ' '); + // Find the @font-faces const fontFaceDeclarations = newlinesStripped.match(/@font-face\s*{(.*?)}/g) || []; + // Go through all the @font-face declarations to find a declared `font-display: ` property for (const declaration of fontFaceDeclarations) { const rawFontDisplay = declaration.match(/font-display:(.*?);/); + // If they didn't have a font-display property, it's the default, and it's failing; bail if (!rawFontDisplay) continue; + // If they don't have one of the passing font-display values, it's failing; bail const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay[0]); if (!hasPassingFontDisplay) continue; + // If it's passing, we'll try to find the URL it's referencing. const rawFontURLs = declaration.match(CSS_URL_GLOBAL_REGEX); + // If no URLs, we can't really do anything; bail if (!rawFontURLs) continue; const relativeURLs = rawFontURLs // @ts-ignore - guaranteed to match from previous regex, pull URL group out .map(s => s.match(CSS_URL_REGEX)[1].trim()) - // remove any optional quotes before/after .map(s => { - const firstChar = s.charAt(0); - if (firstChar === s.charAt(s.length - 1) && (firstChar === '"' || firstChar === '\'')) { + // remove any quotes surrounding the URL + if (/^('|").*\1$/.test(s)) { return s.substr(1, s.length - 2); } return s; }); - const absoluteURLs = relativeURLs.map(url => new URL(url, artifacts.URL.finalUrl)); - - for (const url of absoluteURLs) { - passingURLs.add(url.href); + // Convert the relative CSS URL to an absolute URL and add it to the passing set + for (const relativeURL of relativeURLs) { + try { + const absoluteURL = new URL(relativeURL, artifacts.URL.finalUrl) + passingURLs.add(absoluteURL.href); + } catch (err) { + Sentry.captureException(err, {tags: {audit: this.meta.id}}); + } } } } diff --git a/lighthouse-core/test/audits/font-display-test.js b/lighthouse-core/test/audits/font-display-test.js index 832e109164b1..58a94c8245c3 100644 --- a/lighthouse-core/test/audits/font-display-test.js +++ b/lighthouse-core/test/audits/font-display-test.js @@ -31,14 +31,17 @@ describe('Performance: Font Display audit', () => { it('fails when not all fonts have a correct font-display rule', async () => { stylesheet.content = ` @font-face { + /* try with " */ src: url("./font-a.woff"); } @font-face { + /* try up a directory with ' */ src: url('../font-b.woff'); } @font-face { + /* try no path with no quotes ' */ src: url(font.woff); } `; @@ -75,16 +78,19 @@ describe('Performance: Font Display audit', () => { stylesheet.content = ` @font-face { font-display: 'block'; + /* try with " */ src: url("./font-a.woff"); } @font-face { font-display: 'fallback'; + /* try up a directory with ' */ src: url('../font-b.woff'); } @font-face { font-display: 'optional'; + /* try no path with no quotes ' */ src: url(font.woff); } `; @@ -111,4 +117,50 @@ describe('Performance: Font Display audit', () => { assert.strictEqual(result.rawValue, true); assert.deepEqual(result.details.items, []); }); + + it('should handle real-world font-face declarations', async () => { + /* eslint-disable max-len */ + stylesheet.content = ` + @font-face{font-family:CNN Clock;src:url(//edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.eot) format("embedded-opentype"),url(//edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.woff2) format("woff2"),url(//edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.woff) format("woff"),url(//edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.ttf) format("truetype");font-weight:900;font-style:normal} + @font-face{font-family:FAVE-CNN;src:url( + "//registry.api.cnn.io/assets/fave/fonts/2.0.15/cnnsans-bold.eot") + ;src: url("//registry.api.cnn.io/assets/fave/fonts/2.0.15/cnnsans-bold.eot?#iefix") format("embedded-opentype"),url("//registry.api.cnn.io/assets/fave/fonts/2.0.15/cnnsans-bold.woff") format("woff"),url("//registry.api.cnn.io/assets/fave/fonts/2.0.15/cnnsans-bold.ttf") format("truetype"),url("//registry.api.cnn.io/assets/fave/fonts/2.0.15/cnnsans-bold.svg?#cnn-icons") format("svg");font-weight:700;font-style:normal} + @font-face{font-family:\'FontAwesome\';src:url(\'../fonts/fontawesome-webfont.eot?v=4.6.1\');src:url(\'../fonts/fontawesome-webfont.eot?#iefix&v=4.6.1\') format(\'embedded-opentype\'),url(\'../fonts/fontawesome-webfont.woff2?v=4.6.1\') format(\'woff2\'),url(\'../fonts/fontawesome-webfont.woff?v=4.6.1\') format(\'woff\'),url(\'../fonts/fontawesome-webfont.ttf?v=4.6.1\') format(\'truetype\'),url(\'../fonts/fontawesome-webfont.svg?v=4.6.1#fontawesomeregular\') format(\'svg\');font-weight:normal;font-style:normal;font-display:swap;} + @font-face { font-family: \'Lato\'; font-style: normal; font-weight: 900; src: local(\'Lato Black\'), local(\'Lato-Black\'), url(https://fonts.gstatic.com/s/lato/v14/S6u9w4BMUTPHh50XSwiPGQ3q5d0.woff2) format(\'woff2\'); unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02BB-02BC, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2122, U+2191, U+2193, U+2212, U+2215, U+FEFF, U+FFFD; } + `; + /* eslint-enable max-len */ + + networkRecords = [ + { + url: 'https://edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.woff2', + startTime: 1, endTime: 5, + resourceType: 'Font', + }, + { + url: 'https://registry.api.cnn.io/assets/fave/fonts/2.0.15/cnnsans-bold.woff', + startTime: 1, endTime: 5, + resourceType: 'Font', + }, + { + url: 'https://example.com/foo/fonts/fontawesome-webfont.woff2?v=4.6.1', + startTime: 1, endTime: 5, + resourceType: 'Font', + }, + { + url: 'https://fonts.gstatic.com/s/lato/v14/S6u9w4BMUTPHh50XSwiPGQ3q5d0.woff2', + startTime: 1, endTime: 5, + resourceType: 'Font', + }, + ]; + + const result = await Audit.audit(getArtifacts()); + assert.strictEqual(result.rawValue, false); + assert.deepEqual(result.details.items.map(item => item.url), [ + 'https://edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.woff2', + 'https://registry.api.cnn.io/assets/fave/fonts/2.0.15/cnnsans-bold.woff', + // FontAwesome should pass + // 'https://example.com/foo/fonts/fontawesome-webfont.woff2?v=4.6.1', + 'https://fonts.gstatic.com/s/lato/v14/S6u9w4BMUTPHh50XSwiPGQ3q5d0.woff2', + ]); + }); }); From 0552133df7ef34444e3d6610cdad2a5d9b506df3 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 11 Oct 2018 17:24:36 -0500 Subject: [PATCH 3/3] lint --- lighthouse-core/audits/font-display.js | 2 +- lighthouse-core/test/audits/font-display-test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js index 975ccc040ef7..2b2c228cd650 100644 --- a/lighthouse-core/audits/font-display.js +++ b/lighthouse-core/audits/font-display.js @@ -83,7 +83,7 @@ class FontDisplay extends Audit { // Convert the relative CSS URL to an absolute URL and add it to the passing set for (const relativeURL of relativeURLs) { try { - const absoluteURL = new URL(relativeURL, artifacts.URL.finalUrl) + const absoluteURL = new URL(relativeURL, artifacts.URL.finalUrl); passingURLs.add(absoluteURL.href); } catch (err) { Sentry.captureException(err, {tags: {audit: this.meta.id}}); diff --git a/lighthouse-core/test/audits/font-display-test.js b/lighthouse-core/test/audits/font-display-test.js index 58a94c8245c3..61bfb7aa6ede 100644 --- a/lighthouse-core/test/audits/font-display-test.js +++ b/lighthouse-core/test/audits/font-display-test.js @@ -119,7 +119,7 @@ describe('Performance: Font Display audit', () => { }); it('should handle real-world font-face declarations', async () => { - /* eslint-disable max-len */ + /* eslint-disable max-len, no-useless-escape */ stylesheet.content = ` @font-face{font-family:CNN Clock;src:url(//edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.eot) format("embedded-opentype"),url(//edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.woff2) format("woff2"),url(//edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.woff) format("woff"),url(//edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.ttf) format("truetype");font-weight:900;font-style:normal} @font-face{font-family:FAVE-CNN;src:url( @@ -128,7 +128,7 @@ describe('Performance: Font Display audit', () => { @font-face{font-family:\'FontAwesome\';src:url(\'../fonts/fontawesome-webfont.eot?v=4.6.1\');src:url(\'../fonts/fontawesome-webfont.eot?#iefix&v=4.6.1\') format(\'embedded-opentype\'),url(\'../fonts/fontawesome-webfont.woff2?v=4.6.1\') format(\'woff2\'),url(\'../fonts/fontawesome-webfont.woff?v=4.6.1\') format(\'woff\'),url(\'../fonts/fontawesome-webfont.ttf?v=4.6.1\') format(\'truetype\'),url(\'../fonts/fontawesome-webfont.svg?v=4.6.1#fontawesomeregular\') format(\'svg\');font-weight:normal;font-style:normal;font-display:swap;} @font-face { font-family: \'Lato\'; font-style: normal; font-weight: 900; src: local(\'Lato Black\'), local(\'Lato-Black\'), url(https://fonts.gstatic.com/s/lato/v14/S6u9w4BMUTPHh50XSwiPGQ3q5d0.woff2) format(\'woff2\'); unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02BB-02BC, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2122, U+2191, U+2193, U+2212, U+2215, U+FEFF, U+FFFD; } `; - /* eslint-enable max-len */ + /* eslint-enable max-len, no-useless-escape */ networkRecords = [ {