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

wardpeet · 2017-11-16T21:11:00Z

So I cooked up a branch that gets fontfaces that are loaded before FCP and do not use font-display swap or optional.

Some issues:
FCP isn't always the correct metric as contenful != meaningful.
The initiator of the font is impossible to find. I can find the stylesheet that holds the font-face but not when it's triggered unless I just take startTime of the networkRecord.

For now I just do end of the network record - navigationstart to calculated wasted time.

I still need to cleanup the code so you shouldn't do a code review
fixes #3107

addyosmani · 2017-11-16T21:20:29Z

Thanks for working on the initial PR for this, @wardpeet!

FCP isn't always the correct metric as contenful != meaningful.

I was worried about FCP not being able to always capture the right moment here. Are there examples of particular sites where this is more of a pronounced issue than others?

wardpeet · 2017-11-16T21:39:49Z

@addyosmani haven't found a site that has FOIT. We should probably look for sites where fcp is a bit lower than fmp like 10% or so.

Haven't seen any sites in the wild. I don't see FOIT that often as I'm on WIFI most of the time

you can trick lighthouse by

<html>
  <meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
  <body>
    <script>
      document.fonts.ready.then(() => {
        window.__FONTSREADY = performance.now();
      });
    </script>
    <style>
      @font-face {
        font-family: 'Roboto';
        font-style: normal;
        font-weight: 400;
        src: url(https://fonts.gstatic.com/s/roboto/v18/oMMgfZMQthOryQo9n22dcuvvDin1pK8aKteLpeZ5c0A.woff2) format('woff2');
        unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2212, U+2215;
      }
      span {
        font-family: "Roboto";
      }
    </style>
    <span>Font font font</span>
    <p>no special font</p>
  </body>
  </html>

patrickhulce

nice ward!! thanks for pushing up the WIP! :)

patrickhulce · 2017-11-17T16:37:31Z

lighthouse-core/audits/webfonts.js

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


we should be fine with fallback too I think, just not pushing for it in our docs

patrickhulce · 2017-11-17T16:38:39Z

lighthouse-core/audits/webfonts.js

+
+    return Promise.all([traceOfTabPromise, networkPromise]).then(([tabTrace, networkRecords]) => {
+      let totalWasted = 0;
+      const fcpInMS = tabTrace.timestamps.firstContentfulPaint / 1000000;


can we compare everything in milliseconds instead?

you'll need to multiply the record endtimes by 1000 but keeps things easier than switching :)

patrickhulce · 2017-11-17T16:39:49Z

lighthouse-core/audits/webfonts.js

+        })
+        // calculate wasted time
+        .map(record => {
+          const wastedTime = (record._endTime * 1000 - tabTrace.timestamps.navigationStart / 1000);


using navstart is a little mean since it's definitely not delaying anything until it's been discovered, how about just the record._startTime?

I think @paulirish disagrees with me on this though

wasn't sure but startTime is the best timing we got as I haven't found a way yet to get the initiator timing (is this a domNode that enters the page later on with JS or is it a stylesheet that gets loaded, ???). What if the font got preloaded do we still want to show this error as the font could be just fetched from cache. Should we only consider fonts that take longer than 100ms? I guess cached fonts don't get hit by the network throttling

patrickhulce · 2017-11-17T16:42:04Z

lighthouse-core/gather/gatherers/fonts.js

+    weight: fontFace.weight,
+  });
+
+  if (document.fonts.status === 'loaded') {


is this if necessary? can we just always use document.fonts.ready

also TIL about document.fonts :)

I think document.fonts.ready didn't resolve anymore if all fonts were already loaded (could be mistaken)

hm playing around with it on github console seemed to be an already resolved promise so might be ok?

smoketests should help us find out :)

patrickhulce · 2017-11-17T17:39:39Z

lighthouse-core/gather/gatherers/fonts.js

+      var rule = document.styleSheets[sheet].cssRules[i];
+
+      if (rule instanceof CSSFontFaceRule) {
+        const keys = Object.keys(rule.style);


feels a bit odd to iterate over the properties in this way, can we just pull the font data directly?

fontFaceRules.push({ display: rule.style.fontDisplay, ... })

sure we can, just wanted this audit to be future proof if any more rules were added to font-face that we could use but maybe just add it if when we need it

ah, I'd be in favor of clearer code until we need actually more font props

not sure about others

I follow your reasoning so 👍

patrickhulce · 2017-11-17T17:41:58Z

lighthouse-core/gather/gatherers/fonts.js

+  }
+
+  _onStyleSheetRemoved({ stylesheetId }) {
+    this.stylesheetIds.splice(this.stylesheetIds.indexOf(stylesheetId), 1);


the stylesheetIds look unused at this point, do we still need them?

yeah I left it here because I wasn't sure if we would go for the javascript approach or devtools protocol approach with devtools I need to get the stylesheet ids so I can parse them in afterpass

if we can do it without devtools and just stick to the JS that seems great to me :)

patrickhulce · 2017-11-17T17:43:30Z

lighthouse-core/audits/webfonts.js

+      const fcpInMS = tabTrace.timestamps.firstContentfulPaint / 1000000;
+      const results = networkRecords.filter(record => {
+        const isFont = record._resourceType === WebInspector.resourceTypes.Font;
+        const isLoadedBeforeFCP = record._endTime < fcpInMS || true;


I thought we settled on flagging all fonts since FCP was too early, is this right @addyosmani @paulirish or am I misremembering?

haha my debug is still here with the || true so basically I don't check fcp 😛 One problem with not using anything like fcp means that we also flag fonts that are using font loader api which in theory do not need the font-display.

patrickhulce · 2017-11-17T17:45:06Z

lighthouse-core/gather/gatherers/fonts.js

+'use strict';
+
+const Gatherer = require('./gatherer');
+const fontUrlRegex = new RegExp('url\\((?:"|\')([^"]+)(?:"|\')\\)');


hm this is just getting the URL as its defined then isn't it? Ideally we could get the resolved URL, but not sure best way.

Perhaps just do this in the page and set the href of a link to get the real value?

I needed this to parse

@font-face { font-family: test; src: local(Test), url('test.woff2'), url("test.woff");

it's probably a good idea to get the full url using the anchor trick

yeah totally get why it's needed just hoping we can report https://example.com/test.woff2 instead of just test.woff2 👍

paulirish · 2017-11-28T21:33:52Z

lighthouse-core/audits/webfonts.js

+      description: 'uses font-display',
+      failureDescription: 'Your fonts are blocking FCP!',
+      helpText: 'You should use font-display!!!!',
+      requiredArtifacts: ['traces', 'Fonts'],


add devtoolsLogs

paulirish · 2017-11-28T21:35:12Z

lighthouse-core/audits/webfonts.js

+    return {
+      name: 'webfonts',
+      description: 'uses font-display',
+      failureDescription: 'Your fonts are blocking FCP!',


Avoid invisible text while webfonts are loading

paulirish · 2017-11-28T21:35:35Z

lighthouse-core/audits/webfonts.js

+  static get meta() {
+    return {
+      name: 'webfonts',
+      description: 'uses font-display',


All text remains visible during webfont loads

paulirish · 2017-11-28T21:37:22Z

lighthouse-core/audits/webfonts.js

+      name: 'webfonts',
+      description: 'uses font-display',
+      failureDescription: 'Your fonts are blocking FCP!',
+      helpText: 'You should use font-display!!!!',


Leverage the font-display CSS feature to ensure text is user-visible while webfonts are loading and avoid a FOIT.

paulirish · 2017-11-28T21:38:03Z

lighthouse-core/audits/webfonts.js

+const UnusedBytes = require('./byte-efficiency/byte-efficiency-audit');
+const allowedFontFaceDisplays = ['optional', 'swap', 'fallback'];
+
+class WebFonts extends Audit {


let's name the audit font-display. change filename, classname, meta.name, etc

paulirish · 2017-11-28T21:41:14Z

lighthouse-core/audits/webfonts.js

+      return {
+        score: UnusedBytes.scoreForWastedMs(totalWasted),
+        rawValue: totalWasted,
+        displayValue: Util.formatMilliseconds(totalWasted, 1),


let's skip a displayValue for this. i don't think summing these times makes too much sense. nor does showing count of the offending requests.

paulirish · 2017-11-28T21:48:10Z

lighthouse-core/audits/webfonts.js

+      const details = Audit.makeTableDetails(headings, results);
+
+      return {
+        score: UnusedBytes.scoreForWastedMs(totalWasted),


how should we determine this is failing/passing?

IMO basically everything should have a font-display on it. if it's an icon-font they might want to explicitly say font-display: block. But they need to make a call either way.

If we follow this policy, then rawValue: results.length === 0

paulirish · 2017-11-28T21:55:59Z

lighthouse-core/audits/webfonts.js

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


similar to our new caching audit, let's accept 'block' as an accepted value.
Slightly better is we include block ones in the results table, but indicate with text that this was allowed. We can defer this later, however.

'auto' i don't think we should accept.

paulirish · 2017-11-28T21:58:04Z

lighthouse-core/gather/gatherers/fonts.js

+    ).then(([loadedFonts, fontFaces]) => {
+      return loadedFonts.map(fontFace => {
+        const fontFaceItem = this._findSameFontFamily(fontFace, fontFaces);
+        fontFace.src = fontFaceItem.src || [];


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' } }

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 */

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

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

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

paulirish · 2017-11-29T01:33:50Z

lighthouse-core/gather/gatherers/fonts.js

+  // get all loaded stylesheets
+  for (let sheet = 0; sheet < document.styleSheets.length; sheet++) {
+    const stylesheet = document.styleSheets[sheet];
+    for (let i = 0; stylesheet.cssRules && i < stylesheet.cssRules.length; i++) {


might as well do a for..of loop for these two.

wardpeet · 2017-12-02T15:42:08Z

I still need to write some tests but I also have a few remarks.

This audit doesn't care about people using font loading api or external services like google fonts where a user has no control of. (do we add this in a message somewhere?)

addyosmani · 2017-12-07T22:04:46Z

Third party font foundries currently own own the @font-face definitions and we need a platform solution for allowing developers to use font-display with third-party served web fonts.

Given this is the case, I think the messaging here should be focused on self-hosted fonts. Ideally, we'd also say something brief for the case where you don't self-host and are still running into issues.

@paulirish what's your take on this?

wardpeet · 2017-12-08T19:04:19Z

If you use a cdn where you self host your font how do we know that it's self-hosted? I believe only way we could is keep a blacklist of domains (googlefonts, typekit, ...)

wardpeet · 2017-12-10T22:06:58Z

Added tests, now just some smoketests are necessary

patrickhulce

looks good after the save-assets-path business and nit! :) thanks for the patience during marathon review @wardpeet 🏃‍♀️ 🏁

patrickhulce · 2018-02-05T18:35:29Z

lighthouse-core/gather/gatherers/fonts.js

+      [
+        driver.evaluateAsync(`(()=>{`
+          + `const args = ${JSON.stringify(fontFaceDescriptors)};`
+          + `return (${getAllLoadedFonts.toString()})(args);})()`),


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()})()`),

patrickhulce · 2018-02-05T18:36:20Z

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -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')


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

patrickhulce · 2018-02-06T18:09:57Z

lighthouse-core/gather/gatherers/fonts.js

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


wardpeet added chillin ❄ new_audit labels Nov 16, 2017

wardpeet requested review from brendankenny, patrickhulce and paulirish as code owners November 16, 2017 21:11

addyosmani added this to the Sprint Quatro: November 13-26 milestone Nov 16, 2017

patrickhulce suggested changes Nov 17, 2017

View reviewed changes

wardpeet removed the chillin ❄ label Nov 27, 2017

paulirish modified the milestones: Sprint Quatro: November 13-26, Sprint Cinco: November 28 - Dec 9 Nov 27, 2017

wardpeet force-pushed the webfonts branch from 5cc778c to de21178 Compare November 27, 2017 23:26

paulirish self-assigned this Nov 28, 2017

paulirish requested changes Nov 29, 2017

View reviewed changes

paulirish added the waiting4committer label Nov 29, 2017

wardpeet force-pushed the webfonts branch from 52e12ce to c2d63ca Compare December 2, 2017 15:29

vinamratasingal-zz modified the milestones: Sprint Cinco: November 28 - Dec 9, Sprint Seis: December 11 - 22 Dec 11, 2017

wardpeet changed the title ~~WIP: webfonts push out FCP - font-display~~ new-audit(font-display) push out FCP - font-display Dec 13, 2017

wardpeet force-pushed the webfonts branch 5 times, most recently from c232e5c to fa39b01 Compare December 15, 2017 22:46

devtools-bot added the waiting4reviewer label Dec 18, 2017

wardpeet and others added 16 commits February 4, 2018 12:22

Review changes

159ab99

fix lint

79e771c

Fix review changes

cdc36f5

Fix eslint

46f771b

Add gatherer test

4da7368

Add audit tests

012be10

Added webfont smoketest

3294b42

Remove debugger statements

090c650

Smokehouse: Convert save-assets-path to an array

ae18d66

fix smokehouse

20d036e

Fix webfonts audit

62b9cea

change eslint-env to browser

7d00459

nuke files

9f32363

Fix regexes for fonts

da7277b

comments and cleanup.

9782386

Review changes

0d2f4a3

wardpeet force-pushed the webfonts branch from 2c4f584 to 0d2f4a3 Compare February 4, 2018 12:02

devtools-bot added waiting4reviewer and removed waiting4committer labels Feb 5, 2018

patrickhulce approved these changes Feb 5, 2018

View reviewed changes

wardpeet and others added 3 commits February 5, 2018 23:38

Remove save-assets-path

d08c5fb

Final nits

600675a

remove unnecessary parens

c1e6405

patrickhulce reviewed Feb 6, 2018

View reviewed changes

patrickhulce added 2 commits February 6, 2018 10:14

add learn more link

8122f7e

remove trailing space

95af1d2

patrickhulce merged commit 7ce4334 into master Feb 7, 2018

patrickhulce deleted the webfonts branch February 7, 2018 21:10

paulirish removed the waiting4reviewer label Mar 6, 2018

tunetheweb mentioned this pull request Jan 21, 2024

The font-display audit doesn't do what it thinks it does #15771

Open

2 tasks

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

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

Conversation

wardpeet commented Nov 16, 2017 • edited

addyosmani commented Nov 16, 2017

wardpeet commented Nov 16, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Nov 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet Nov 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet Nov 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet Nov 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Dec 2, 2017

addyosmani commented Dec 7, 2017

wardpeet commented Dec 8, 2017

wardpeet commented Dec 10, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Nov 16, 2017 •

edited

patrickhulce Nov 21, 2017 •

edited

wardpeet Nov 17, 2017 •

edited

wardpeet Nov 17, 2017 •

edited

wardpeet Nov 17, 2017 •

edited