Repository: GoogleChrome/lighthouse
Pull request #9202 (2019-06-14 11:53:28)

misc: use `CHROME_PATH` variable in CI and scripts

r0mflip: @patrickhulce Setting `$CHROME_PATH` alone just makes the script to exit. Even though I use the suggested variable name I would have to set `$LIGHTHOUSE_CHROMIUM_PATH` anyways. This PR changes that behavior.
patrickhulce: > Even though I use the suggested variable name I would have to set $LIGHTHOUSE_CHROMIUM_PATH anyways

I understand; it's a good update to make us get off the deprecated chrome-launcher variable :) I was just also noting for other readers that this variable name does still work in `chrome-launcher` even though we change it for our download script here in Lighthouse.

Patch:
diff --git a/.appveyor.yml b/.appveyor.yml
index 71fa74955..f6e6cf6b9 100644
--- a/.appveyor.yml
+++ b/.appveyor.yml
@@ -21,7 +21,7 @@ install:
 
 before_test:
   - set "PATH=C:\MinGW\msys\1.0\bin;%PATH%"
-  - set "LIGHTHOUSE_CHROMIUM_PATH=%CD%\chrome-win32\chrome.exe"
+  - set "CHROME_PATH=%CD%\chrome-win32\chrome.exe"
   - bash ./lighthouse-core/scripts/download-chrome.sh
   - yarn build-all
 
diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js
index 52f0c1c35..9f273a0a2 100644
--- a/lighthouse-core/audits/font-display.js
+++ b/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,13 +86,14 @@ 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}});
           }
@@ -96,7 +101,7 @@ class FontDisplay extends Audit {
       }
     }
 
-    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})),
     };
   }
 }
diff --git a/lighthouse-core/lib/i18n/en-US.json b/lighthouse-core/lib/i18n/en-US.json
index 224f11629..91c6886af 100644
--- a/lighthouse-core/lib/i18n/en-US.json
+++ b/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."
diff --git a/lighthouse-core/report/html/renderer/performance-category-renderer.js b/lighthouse-core/report/html/renderer/performance-category-renderer.js
index 58083b5ee..1193bb3a8 100644
--- a/lighthouse-core/report/html/renderer/performance-category-renderer.js
+++ b/lighthouse-core/report/html/renderer/performance-category-renderer.js
@@ -123,14 +123,14 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
     }
 
     // Metrics.
-    const metricAudits = category.auditRefs.filter(audit => audit.group === 'metrics');
     const metricAuditsEl = this.renderAuditGroup(groups.metrics);
 
     // Metric descriptions toggle.
     const toggleTmpl = this.dom.cloneTemplate('#tmpl-lh-metrics-toggle', this.templateContext);
     const toggleEl = this.dom.find('.lh-metrics-toggle', toggleTmpl);
-    metricAuditsEl.prepend(...toggleEl.childNodes);
+    metricAuditsEl.append(...toggleEl.childNodes);
 
+    const metricAudits = category.auditRefs.filter(audit => audit.group === 'metrics');
     const keyMetrics = metricAudits.filter(a => a.weight >= 3);
     const otherMetrics = metricAudits.filter(a => a.weight < 3);
 
diff --git a/lighthouse-core/report/html/templates.html b/lighthouse-core/report/html/templates.html
index 01498480b..11d1d78e0 100644
--- a/lighthouse-core/report/html/templates.html
+++ b/lighthouse-core/report/html/templates.html
@@ -71,7 +71,7 @@ limitations under the License.
 <!-- Lighthouse metrics toggle -->
 <template id="tmpl-lh-metrics-toggle">
   <div class="lh-metrics-toggle">
-    <input class="lh-metrics-toggle__input" type="checkbox" name="toggle-metric-descriptions">
+    <input class="lh-metrics-toggle__input" type="checkbox" id="toggle-metric-descriptions" aria-label="Toggle the display of metric descriptions">
     <label class="lh-metrics-toggle__label" for="toggle-metric-descriptions">
       <div class="lh-metrics-toggle__icon lh-metrics-toggle__icon--less" aria-hidden="true">
         <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" viewBox="0 0 24 24">
@@ -366,7 +366,7 @@ limitations under the License.
     <a href="" class="lh-topbar__url" target="_blank" rel="noopener"></a>
 
     <div class="lh-tools">
-      <button class="report-icon report-icon--share lh-tools__button" title="Export report">
+      <button class="report-icon report-icon--share lh-tools__button" title="Tools menu" aria-label="Toggle report tools menu">
         <svg width="100%" height="100%" viewBox="0 0 24 24">
             <path d="M0 0h24v24H0z" fill="none"/>
             <path d="M12 8c1.1 0 2-.9 2-2s-.9-2-2-2-2 .9-2 2 .9 2 2 2zm0 2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2zm0 6c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z"/>
diff --git a/lighthouse-core/scripts/download-chrome.sh b/lighthouse-core/scripts/download-chrome.sh
index 5c073cd7d..aeeae3e90 100755
--- a/lighthouse-core/scripts/download-chrome.sh
+++ b/lighthouse-core/scripts/download-chrome.sh
@@ -14,12 +14,12 @@ else
   url="https://download-chromium.appspot.com/dl/Linux_x64?type=snapshots"
 fi
 
-if [ x"$LIGHTHOUSE_CHROMIUM_PATH" == x ]; then
-  echo "Error: Environment variable LIGHTHOUSE_CHROMIUM_PATH not set"
+if [ x"$CHROME_PATH" == x ]; then
+  echo "Error: Environment variable CHROME_PATH not set"
   exit 1
 fi
 
-if [ -e "$LIGHTHOUSE_CHROMIUM_PATH" ]; then
+if [ -e "$CHROME_PATH" ]; then
   echo "cached chrome found"
 else
   wget "$url" --no-check-certificate -q -O chrome.zip && unzip -q chrome.zip
diff --git a/proto/sample_v2_round_trip.json b/proto/sample_v2_round_trip.json
index 5e0e3279f..54f58ebd4 100644
--- a/proto/sample_v2_round_trip.json
+++ b/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).",


Test patch:
diff --git a/lighthouse-core/test/audits/font-display-test.js b/lighthouse-core/test/audits/font-display-test.js
index 36b674050..f3dfad9d5 100644
--- a/lighthouse-core/test/audits/font-display-test.js
+++ b/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,7 +222,7 @@ 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',
@@ -228,6 +230,7 @@ describe('Performance: Font Display audit', () => {
       // '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/);
   });
 });
diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
index 088cbea10..d9a6deefb 100644
--- a/lighthouse-core/test/results/sample_v2.json
+++ b/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": [],