-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core: remove fonts gatherer #6166
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<string>} */ | ||
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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how do we want to deal with exceptions here? e.g. invalid font URL probably shouldn't throw the whole audit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah good point, I'll add a try/catch with beacon |
||
|
||
for (const url of absoluteURLs) { | ||
passingURLs.add(url.href); | ||
} | ||
} | ||
} | ||
|
||
return passingURLs; | ||
} | ||
|
||
/** | ||
* @param {LH.Artifacts} artifacts | ||
* @return {Promise<LH.Audit.Product>} | ||
*/ | ||
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, | ||
}; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,7 +74,6 @@ const defaultConfig = { | |
'seo/embedded-content', | ||
'seo/canonical', | ||
'seo/robots-txt', | ||
'fonts', | ||
], | ||
}, | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better like this than in the regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this might be more readable than the backreference regex, but I guess not :) switched!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, it might be! I was just asking