Fix review changes

GoogleChrome · Dec 2, 2017 · 52e12ce · 52e12ce
1 parent a3b1351
commit 52e12ce
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 57 deletions.
diff --git a/lighthouse-core/audits/webfonts.js → lighthouse-core/audits/font-display.js b/lighthouse-core/audits/webfonts.js → lighthouse-core/audits/font-display.js
@@ -8,20 +8,20 @@
 const Audit = require('./audit');
 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'];
+const allowedFontFaceDisplays = ['block', 'fallback', 'optional', 'swap'];
 
-class WebFonts extends Audit {
+class FontDisplay extends Audit {
   /**
    * @return {!AuditMeta}
    */
   static get meta() {
     return {
-      name: 'webfonts',
-      description: 'uses font-display',
-      failureDescription: 'Your fonts are blocking FCP!',
-      helpText: 'You should use font-display!!!!',
-      requiredArtifacts: ['traces', 'Fonts'],
+      name: 'font-display',
+      description: 'All text remains visible during webfont loads',
+      failureDescription: 'Avoid invisible text while webfonts are loading',
+      helpText: 'Leverage the font-display CSS feature to ensure text is user-visible while ' +
+        'webfonts are loading and avoid a FOIT.',
+      requiredArtifacts: ['devtoolsLogs', 'Fonts'],
     };
   }
 
@@ -30,26 +30,19 @@ class WebFonts extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    const trace = artifacts.traces[this.DEFAULT_PASS];
     const devtoolsLogs = artifacts.devtoolsLogs[this.DEFAULT_PASS];
     const fontFaces = artifacts.Fonts;
-    const traceOfTabPromise = artifacts.requestTraceOfTab(trace);
-    const networkPromise = artifacts.requestNetworkRecords(devtoolsLogs);
 
     // 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 Promise.all([traceOfTabPromise, networkPromise]).then(([tabTrace, networkRecords]) => {
-      let totalWasted = 0;
-      // const fcpInMS = tabTrace.timestamps.firstContentfulPaint / 1000;
+    return artifacts.requestNetworkRecords(devtoolsLogs).then((networkRecords) => {
       const results = networkRecords.filter(record => {
         const isFont = record._resourceType === WebInspector.resourceTypes.Font;
-        // const isLoadedBeforeFCP = record._endTime * 1000 < fcpInMS;
 
-        return isFont;// && isLoadedBeforeFCP;
+        return isFont;
       })
         .filter(fontRecord => {
           // find the fontRecord of a font
@@ -59,8 +52,9 @@ class WebFonts extends Audit {
         })
         // calculate wasted time
         .map(record => {
-          const wastedTime = (record._endTime * 1000 - tabTrace.timestamps.navigationStart / 1000);
-          totalWasted += wastedTime;
+          // 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 wastedTime = Math.min((record._endTime - record._startTime) * 1000, 3000);
 
           return {
             url: record.url,
@@ -70,23 +64,17 @@ class WebFonts extends Audit {
 
       const headings = [
         {key: 'url', itemType: 'url', text: 'Font URL'},
-        {key: 'wastedTime', itemType: 'text', text: 'Time it took'},
+        {key: 'wastedTime', itemType: 'text', text: 'Font download time'},
       ];
       const details = Audit.makeTableDetails(headings, results);
 
       return {
-        score: UnusedBytes.scoreForWastedMs(totalWasted),
-        rawValue: totalWasted,
-        displayValue: Util.formatMilliseconds(totalWasted, 1),
-        extendedInfo: {
-          value: {
-            wastedMs: totalWasted,
-          },
-        },
+        score: results.length === 0,
+        rawValue: results.length === 0,
         details,
       };
     });
   }
 }
 
-module.exports = WebFonts;
+module.exports = FontDisplay;
diff --git a/lighthouse-core/config/default.js b/lighthouse-core/config/default.js
@@ -93,7 +93,7 @@ module.exports = {
     'deprecations',
     'mainthread-work-breakdown',
     'bootup-time',
-    'webfonts',
+    'font-display',
     'manual/pwa-cross-browser',
     'manual/pwa-page-transitions',
     'manual/pwa-each-page-has-url',
@@ -273,7 +273,7 @@ module.exports = {
         {id: 'bootup-time', weight: 0, group: 'perf-info'},
         {id: 'screenshot-thumbnails', weight: 0},
         {id: 'mainthread-work-breakdown', weight: 0, group: 'perf-info'},
-        {id: 'webfonts', weight: 0, group: 'perf-info'},
+        {id: 'font-display', weight: 0, group: 'perf-info'},
       ],
     },
     'accessibility': {

diff --git a/lighthouse-core/gather/gatherers/fonts.js b/lighthouse-core/gather/gatherers/fonts.js
@@ -7,7 +7,6 @@
 
 const Gatherer = require('./gatherer');
 
-/* eslint-disable */
 function getAllLoadedFonts() {
   const getFont = fontFace => ({
     display: fontFace.display,
@@ -31,39 +30,61 @@ function getFontFaceFromStylesheets() {
     return link.href;
   }
 
-  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];
-    for (let i = 0; stylesheet.cssRules && i < stylesheet.cssRules.length; i++) {
-      var rule = stylesheet.cssRules[i];
-
-      if (rule instanceof CSSFontFaceRule) {
-        const fontsObject = {
-          display: rule.style.fontDisplay || 'auto',
-          family: rule.style.fontFamily.replace(/"|'/g, ''),
-          stretch: rule.style.fontStretch || 'normal',
-          style: rule.style.fontStyle || 'normal',
-          weight: rule.style.fontWeight || 'normal',
-          src: [],
-        }
+  function getFontFaceRules(stylesheet) {
+    const fontFaceRules = [];
+    if (stylesheet.cssRules) {
+      for (const rule of stylesheet.cssRules) {
+        if (rule instanceof CSSFontFaceRule) {
+          const fontsObject = {
+            display: rule.style.fontDisplay || 'auto',
+            family: rule.style.fontFamily.replace(/"|'/g, ''),
+            stretch: rule.style.fontStretch || 'normal',
+            style: rule.style.fontStyle || 'normal',
+            weight: rule.style.fontWeight || 'normal',
+            src: [],
+          }
 
-        if (rule.style.src) {
-          const matches = rule.style.src.match(fontUrlRegex);
-          if (matches) {
-            fontsObject.src.push(resolveUrl(matches[1]));
+          if (rule.style.src) {
+            const matches = rule.style.src.match(fontUrlRegex);
+            if (matches) {
+              fontsObject.src.push(resolveUrl(matches[1]));
+            }
           }
-        }
 
-        fontFaceRules.push(fontsObject);
+          fontFaceRules.push(fontsObject);
+        }
       }
     }
+
+    return fontFaceRules;
+  }
+
+  const fontUrlRegex = new RegExp('url\\((?:"|\')([^"]+)(?:"|\')\\)');
+  const fontFacePromises = [];
+  // get all loaded stylesheets
+  for(const stylesheet of document.styleSheets) {
+    // Cross-origin stylesheets don't expose cssRules by default. We reload them with CORS headers.
+    try {
+      fontFacePromises.push(Promise.resolve(getFontFaceRules(stylesheet)));
+    } catch (err) {
+      const oldNode = stylesheet.ownerNode;
+      const newNode = oldNode.cloneNode(true);
+
+      fontFacePromises.push(new Promise(resolve => {
+        newNode.addEventListener('load', function onload() {
+          newNode.removeEventListener('load', onload);
+          resolve(getFontFaceFromStylesheets());
+        });
+        newNode.crossOrigin = 'anonymous';
+        oldNode.parentNode.insertBefore(newNode, oldNode);
+        oldNode.remove();
+      }));
+    }
   }
 
-  return fontFaceRules;
+  return Promise.all(fontFacePromises)
+    .then(fontFaces => [].concat(...fontFaces));
 }
-/* eslint-enable */
 
 class Fonts extends Gatherer {
   constructor() {