core(font-display): limit false positives (#9148)

lighthouse-core/audits/font-display.js

@@ -24,6 +24,9 @@ const UIStrings = {
     '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).',
+  /** A warning message that is shown when Lighthouse couldn't automatically check some of the page's fonts and that the user will need to manually check it. */
+  undeclaredFontURLWarning: 'Lighthouse was unable to automatically check the font-display value ' +
+    'for the following URL: {fontURL}.',
 };
 
 const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
@@ -44,10 +47,13 @@ class FontDisplay extends Audit {
 
   /**
    * @param {LH.Artifacts} artifacts
+   * @return {{passingURLs: Set<string>, failingURLs: Set<string>}}
    */
-  static findPassingFontDisplayDeclarations(artifacts) {
+  static findFontDisplayDeclarations(artifacts) {
     /** @type {Set<string>} */
     const passingURLs = new Set();
+    /** @type {Set<string>} */
+    const failingURLs = new Set();
 
     // Go through all the stylesheets to find all @font-face declarations
     for (const stylesheet of artifacts.CSSUsage.stylesheets) {
@@ -57,20 +63,18 @@ class FontDisplay extends Audit {
       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) {
-        // Find the font-display value by matching a single token, optionally surrounded by whitespace,
-        // followed either by a semicolon or the end of a block.
-        const rawFontDisplay = declaration.match(/font-display\s*:\s*(\w+)\s*(;|\})/);
-        // 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[1]);
-        if (!hasPassingFontDisplay) continue;
-
-        // If it's passing, we'll try to find the URL it's referencing.
+        // 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;
+        // Find the font-display value by matching a single token, optionally surrounded by whitespace,
+        // followed either by a semicolon or the end of a block.
+        const fontDisplayMatch = declaration.match(/font-display\s*:\s*(\w+)\s*(;|\})/);
+        const rawFontDisplay = (fontDisplayMatch && fontDisplayMatch[1]) || '';
+        const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay);
+        const targetURLSet = hasPassingFontDisplay ? passingURLs : failingURLs;
 
+        // Finally convert the raw font URLs to the absolute URLs and add them to the set.
         const relativeURLs = rawFontURLs
           // @ts-ignore - guaranteed to match from previous regex, pull URL group out
           .map(s => s.match(CSS_URL_REGEX)[1].trim())
@@ -82,21 +86,22 @@ class FontDisplay extends Audit {
 
             return s;
           });
-        // Convert the relative CSS URL to an absolute URL and add it to the passing set
+
+        // Convert the relative CSS URL to an absolute URL and add it to the target set.
         for (const relativeURL of relativeURLs) {
           try {
             const relativeRoot = URL.isValid(stylesheet.header.sourceURL) ?
               stylesheet.header.sourceURL : artifacts.URL.finalUrl;
             const absoluteURL = new URL(relativeURL, relativeRoot);
-            passingURLs.add(absoluteURL.href);
+            targetURLSet.add(absoluteURL.href);
           } catch (err) {
             Sentry.captureException(err, {tags: {audit: this.meta.id}});
           }
         }
       }
     }
 
-    return passingURLs;
+    return {passingURLs, failingURLs};
   }
 
   /**
@@ -107,16 +112,24 @@ class FontDisplay extends Audit {
   static async audit(artifacts, context) {
     const devtoolsLogs = artifacts.devtoolsLogs[this.DEFAULT_PASS];
     const networkRecords = await NetworkRecords.request(devtoolsLogs, context);
-    const passingFontURLs = FontDisplay.findPassingFontDisplayDeclarations(artifacts);
+    const {passingURLs, failingURLs} = FontDisplay.findFontDisplayDeclarations(artifacts);
+    /** @type {Array<string>} */
+    const warningURLs = [];
 
     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))
       // ...and that aren't data URLs, the blocking concern doesn't really apply
       .filter(record => !/^data:/.test(record.url))
       .filter(record => !/^blob:/.test(record.url))
+      // ...that have a failing font-display value
+      .filter(record => {
+        // Failing URLs should be considered.
+        if (failingURLs.has(record.url)) return true;
+        // Everything else shouldn't be, but we should warn if we don't recognize the URL at all.
+        if (!passingURLs.has(record.url)) warningURLs.push(record.url);
+        return false;
+      })
       .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
@@ -139,6 +152,7 @@ class FontDisplay extends Audit {
     return {
       score: Number(results.length === 0),
       details,
+      warnings: warningURLs.map(fontURL => str_(UIStrings.undeclaredFontURLWarning, {fontURL})),
     };
   }
 }

lighthouse-core/lib/i18n/en-US.json

@@ -651,6 +651,10 @@
     "message": "All text remains visible during webfont loads",
     "description": "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."
   },
+  "lighthouse-core/audits/font-display.js | undeclaredFontURLWarning": {
+    "message": "Lighthouse was unable to automatically check the font-display value for the following URL: {fontURL}.",
+    "description": "A warning message that is shown when Lighthouse couldn't automatically check some of the page's fonts and that the user will need to manually check it."
+  },
   "lighthouse-core/audits/load-fast-enough-for-pwa.js | description": {
     "message": "A fast page load over a cellular network ensures a good mobile user experience. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/fast-3g).",
     "description": "Description of a Lighthouse audit that tells the user *why* they need to load fast enough on mobile networks. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation."

lighthouse-core/test/audits/font-display-test.js

@@ -6,7 +6,6 @@
 'use strict';
 
 const FontDisplayAudit = require('../../audits/font-display.js');
-const assert = require('assert');
 const networkRecordsToDevtoolsLog = require('../network-records-to-devtools-log.js');
 
 /* eslint-env jest */
@@ -73,8 +72,9 @@ describe('Performance: Font Display audit', () => {
       {url: networkRecords[1].url, wastedMs: 3000},
       {url: networkRecords[2].url, wastedMs: 1000},
     ];
-    assert.strictEqual(result.score, 0);
+    expect(result.score).toEqual(0);
     expect(result.details.items).toEqual(items);
+    expect(result.warnings).toEqual([]);
   });
 
   it('resolves URLs relative to stylesheet URL when available', async () => {
@@ -129,8 +129,9 @@ describe('Performance: Font Display audit', () => {
     ];
 
     const result = await FontDisplayAudit.audit(getArtifacts(), context);
-    assert.strictEqual(result.score, 1);
+    expect(result.score).toEqual(1);
     expect(result.details.items).toEqual([]);
+    expect(result.warnings).toEqual([]);
   });
 
   it('passes when all fonts have a correct font-display rule', async () => {
@@ -180,8 +181,9 @@ describe('Performance: Font Display audit', () => {
     ];
 
     const result = await FontDisplayAudit.audit(getArtifacts(), context);
-    assert.strictEqual(result.score, 1);
+    expect(result.score).toEqual(1);
     expect(result.details.items).toEqual([]);
+    expect(result.warnings).toEqual([]);
   });
 
   it('should handle real-world font-face declarations', async () => {
@@ -220,14 +222,15 @@ describe('Performance: Font Display audit', () => {
     ];
 
     const result = await FontDisplayAudit.audit(getArtifacts(), context);
-    assert.strictEqual(result.score, 0);
+    expect(result.score).toEqual(0);
     expect(result.details.items.map(item => item.url)).toEqual([
       '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',
     ]);
+    expect(result.warnings).toEqual([]);
   });
 
   it('handles varied font-display declarations', async () => {
@@ -259,7 +262,8 @@ describe('Performance: Font Display audit', () => {
 
     const result = await FontDisplayAudit.audit(getArtifacts(), context);
     expect(result.details.items).toEqual([]);
-    assert.strictEqual(result.score, 1);
+    expect(result.score).toEqual(1);
+    expect(result.warnings).toEqual([]);
   });
 
   it('handles custom source URLs from sourcemaps', async () => {
@@ -280,6 +284,57 @@ describe('Performance: Font Display audit', () => {
 
     const result = await FontDisplayAudit.audit(getArtifacts(), context);
     expect(result.details.items).toEqual([]);
-    assert.strictEqual(result.score, 1);
+    expect(result.score).toEqual(1);
+  });
+
+  it('should not flag a URL for which there is not @font-face at all', async () => {
+    // Sometimes the content does not come through, see https://github.com/GoogleChrome/lighthouse/issues/8493
+    stylesheet.content = ``;
+
+    networkRecords = [{
+      url: `https://example.com/foo/bar/font-0.woff`,
+      endTime: 2, startTime: 1,
+      resourceType: 'Font',
+    }];
+
+    const result = await FontDisplayAudit.audit(getArtifacts(), context);
+    expect(result.details.items).toEqual([]);
+    expect(result.score).toEqual(1);
+    expect(result.warnings).toHaveLength(1);
+    expect(result.warnings[0]).toBeDisplayString(/font-0.woff/);
+  });
+
+  it('should handle mixed content', async () => {
+    networkRecords = [{
+      url: `https://example.com/foo/bar/font-0.woff`,
+      endTime: 2, startTime: 1,
+      resourceType: 'Font',
+    }, {
+      url: `https://example.com/foo/bar/font-1.woff`,
+      endTime: 2, startTime: 1,
+      resourceType: 'Font',
+    }];
+
+    const artifacts = getArtifacts();
+    artifacts.CSSUsage.stylesheets = [
+      {content: '', header: {}},
+      {
+        content: `
+          @font-face {
+            /* try with " */
+            src: url("./font-0.woff");
+          }
+        `,
+        header: {},
+      },
+    ];
+    const result = await FontDisplayAudit.audit(artifacts, context);
+    expect(result.details.items).toEqual([{
+      url: `https://example.com/foo/bar/font-0.woff`,
+      wastedMs: 1000,
+    }]);
+    expect(result.score).toEqual(0);
+    expect(result.warnings).toHaveLength(1);
+    expect(result.warnings[0]).toBeDisplayString(/font-1.woff/);
   });
 });

lighthouse-core/test/results/sample_v2.json

@@ -796,6 +796,7 @@
       "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).",
       "score": 1,
       "scoreDisplayMode": "binary",
+      "warnings": [],
       "details": {
         "type": "table",
         "headings": [],

proto/sample_v2_round_trip.json

@@ -759,7 +759,8 @@
             "id": "font-display",
             "score": 1.0,
             "scoreDisplayMode": "binary",
-            "title": "All text remains visible during webfont loads"
+            "title": "All text remains visible during webfont loads",
+            "warnings": []
         },
         "font-size": {
             "description": "Font sizes less than 12px are too small to be legible and require mobile visitors to \u201cpinch to zoom\u201d in order to read. Strive to have >60% of page text \u226512px. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/font-sizes).",