Repository: GoogleChrome/lighthouse
Pull request #8633 (2019-04-26 01:05:45)

report: augment auditRef, not result, with stackPack ref
Not sure if I can convince everyone we should make this change :) Note that it's contained entirely within the report renderer, so there will be no effect on core, PSI, etc.

In `prepareReportResult` we smoosh a few things to make report rendering easier, like writing a `LH.Audit.Result` to the relevant `LH.Result.AuditRef` and, most recently, writing `LH.ReportResult.StackPackDescription` to the relevant `LH.Audit.Result`.

This PR makes it so *both* `LH.Audit.Result` and `LH.ReportResult.StackPackDescription` are written to the auditRef.

On the con side: it makes a certain amount of sense to match stack packs to auditResults as they have a 1:1 match, while there may be multiple auditRefs to a single audit (and thus a stack pack).

*However*:
- we should be minimizing and isolating the amount of smooshing we do, both for the sake of "purity" and to reduce confusion in the codebase between almost but not quite identical types like the audit results.
- this comes up in a few places in our tests where we formerly could assume that audit results were audit results, but now we have to add special cases to handle these mutant objects. See for example the deleting [needed to make this PR work](https://github.com/GoogleChrome/lighthouse/pull/8536/files#diff-e5c26900a42826b945adef39d538f9ddR157). Ideally we wouldn't have to do that
- the only place we render these we're looping over auditRefs anyway, so we don't save any ease of use either way :)

The solution in this PR keeps modifications contained within `LH.ReportResult.AuditRef` and makes that deletion in #8536 unnecessary.

(@exterkamp if we land this you won't need any of your changes to `util-test.js`)


Patch:
diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js
index e05fd1b68..f6460c28e 100644
--- a/lighthouse-core/audits/font-display.js
+++ b/lighthouse-core/audits/font-display.js
@@ -6,7 +6,7 @@
 'use strict';
 
 const Audit = require('./audit');
-const URL = require('../lib/url-shim').URL;
+const URL = require('../lib/url-shim');
 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');
@@ -82,11 +82,11 @@ class FontDisplay extends Audit {
 
             return s;
           });
-
         // Convert the relative CSS URL to an absolute URL and add it to the passing set
         for (const relativeURL of relativeURLs) {
           try {
-            const relativeRoot = stylesheet.header.sourceURL || artifacts.URL.finalUrl;
+            const relativeRoot = URL.isValid(stylesheet.header.sourceURL) ?
+              stylesheet.header.sourceURL : artifacts.URL.finalUrl;
             const absoluteURL = new URL(relativeURL, relativeRoot);
             passingURLs.add(absoluteURL.href);
           } catch (err) {
diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js
index a828f3abf..7b26f588a 100644
--- a/lighthouse-core/config/config.js
+++ b/lighthouse-core/config/config.js
@@ -496,7 +496,7 @@ class Config {
   }
 
   /**
-   * @param {LH.Config.SettingsJson=} settingsJson
+   * @param {LH.SharedFlagsSettings=} settingsJson
    * @param {LH.Flags=} flags
    * @return {LH.Config.Settings}
    */
diff --git a/lighthouse-core/report/html/renderer/category-renderer.js b/lighthouse-core/report/html/renderer/category-renderer.js
index dcfc9b1fb..adbfcc063 100644
--- a/lighthouse-core/report/html/renderer/category-renderer.js
+++ b/lighthouse-core/report/html/renderer/category-renderer.js
@@ -81,8 +81,8 @@ class CategoryRenderer {
     this.dom.find('.lh-audit__description', auditEl)
         .appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
 
-    if (audit.result.stackPacks) {
-      audit.result.stackPacks.forEach(pack => {
+    if (audit.stackPacks) {
+      audit.stackPacks.forEach(pack => {
         const packElm = this.dom.createElement('div');
         packElm.classList.add('lh-audit__stackpack');
 
diff --git a/lighthouse-core/report/html/renderer/util.js b/lighthouse-core/report/html/renderer/util.js
index 8b1ba2fae..f0dd4e25c 100644
--- a/lighthouse-core/report/html/renderer/util.js
+++ b/lighthouse-core/report/html/renderer/util.js
@@ -72,21 +72,21 @@ class Util {
       }
     }
 
-    // For convenience, smoosh all AuditResults into their auditDfn (which has just weight & group)
+    // For convenience, smoosh all AuditResults into their auditRef (which has just weight & group)
     for (const category of clone.reportCategories) {
-      category.auditRefs.forEach(auditMeta => {
-        const result = clone.audits[auditMeta.id];
-        auditMeta.result = result;
+      category.auditRefs.forEach(auditRef => {
+        const result = clone.audits[auditRef.id];
+        auditRef.result = result;
 
         // attach the stackpacks to the auditRef object
         if (clone.stackPacks) {
           clone.stackPacks.forEach(pack => {
-            if (pack.descriptions[auditMeta.id]) {
-              auditMeta.result.stackPacks = auditMeta.result.stackPacks || [];
-              auditMeta.result.stackPacks.push({
+            if (pack.descriptions[auditRef.id]) {
+              auditRef.stackPacks = auditRef.stackPacks || [];
+              auditRef.stackPacks.push({
                 title: pack.title,
                 iconDataURL: pack.iconDataURL,
-                description: pack.descriptions[auditMeta.id],
+                description: pack.descriptions[auditRef.id],
               });
             }
           });
diff --git a/lighthouse-core/report/html/report-styles.css b/lighthouse-core/report/html/report-styles.css
index 0a5a5d345..f7c25eb11 100644
--- a/lighthouse-core/report/html/report-styles.css
+++ b/lighthouse-core/report/html/report-styles.css
@@ -398,7 +398,7 @@
 /* Expandable Details (Audit Groups, Audits) */
 .lh-audit__header {
   display: flex;
-  padding: var(--lh-audit-vpadding) var(--text-indent);
+  padding: var(--lh-audit-vpadding) 0;
 }
 
 .lh-audit--load-opportunity .lh-audit__header {
@@ -650,6 +650,15 @@
   border-bottom: 1px solid var(--report-secondary-border-color);
 }
 
+/* Apply border-top to just the first audit. */
+.lh-audit {
+  border-top: 1px solid var(--report-secondary-border-color);
+}
+.lh-audit ~ .lh-audit {
+  border-top: none;
+}
+
+
 .lh-audit--error .lh-audit__display-text {
   color: var(--color-fail);
 }
@@ -657,7 +666,7 @@
 /* Audit Group */
 
 .lh-audit-group {
-  margin: var(--lh-audit-group-vpadding) 0;
+  margin-bottom: var(--audits-margin-bottom);
 }
 
 .lh-audit-group__header::before {
@@ -692,11 +701,6 @@
   background-image: var(--pwa-optimized-color-url);
 }
 
-/* Removing too much whitespace */
-.lh-audit-group--metrics {
-  margin-top: calc(var(--gauge-circle-size)/2 * -1);
-  border-bottom: none;
-}
 .lh-audit-group--metrics .lh-audit-group__summary {
   margin-top: 0;
   margin-bottom: 0;
@@ -721,6 +725,8 @@
 .lh-audit-group__header {
   font-size: var(--body-font-size);
   margin: 0 0 var(--lh-audit-group-vpadding);
+  /* When the header takes 100% width, the chevron becomes small. */
+  max-width: calc(100% - var(--chevron-size));
 }
 
 .lh-audit-group__header span.lh-audit-group__title {
@@ -838,6 +844,10 @@
   border: 0;
 }
 
+.lh-gauge__wrapper {
+  padding: var(--score-container-padding);
+}
+
 .lh-scores-header .lh-gauge__wrapper,
 .lh-scores-header .lh-gauge--pwa__wrapper,
 .lh-sticky-header .lh-gauge__wrapper,
diff --git a/lighthouse-core/report/html/templates.html b/lighthouse-core/report/html/templates.html
index dbe7fb8fc..77abd4ac3 100644
--- a/lighthouse-core/report/html/templates.html
+++ b/lighthouse-core/report/html/templates.html
@@ -539,7 +539,12 @@ limitations under the License.
 
     .lh-gauge__svg-wrapper {
       position: relative;
+      height: var(--gauge-circle-size);
+    }
+    .lh-category .lh-gauge__svg-wrapper {
+      --gauge-circle-size: var(--gauge-circle-size-big);
     }
+
     /* The plugin badge overlay */
     .lh-gauge__wrapper--plugin .lh-gauge__svg-wrapper::before {
       width: var(--plugin-badge-size);
@@ -576,15 +581,16 @@ limitations under the License.
       font-size: var(--score-number-font-size);
       line-height: var(--score-number-font-size);
       text-align: center;
-      top: calc(var(--gauge-circle-size) / 3);
+      top: calc(var(--score-container-padding) + var(--gauge-circle-size) / 3);
     }
 
     .lh-category .lh-gauge__percentage {
       --gauge-circle-size: var(--gauge-circle-size-big);
-      --score-number-fontsize: var(--score-number-fontsize-big);
+      --score-number-font-size: var(--score-number-font-size-big);
     }
 
     .lh-gauge__wrapper {
+      position: relative;
       display: flex;
       align-items: center;
       flex-direction: column;
@@ -601,7 +607,7 @@ limitations under the License.
     .lh-gauge__label {
       font-size: var(--score-title-font-size);
       line-height: var(--score-title-line-height);
-      margin-bottom: calc(0.5 * var(--score-title-line-height));
+      margin-top: 10px;
       text-align: center;
       color: black;
     }
@@ -610,7 +616,7 @@ limitations under the License.
     .lh-category .lh-gauge__label {
       --score-title-font-size: var(--score-title-font-size-big);
       --score-title-line-height: var(--score-title-line-height-big);
-      margin: calc(0.5 * var(--score-title-line-height));
+      margin-top: 14px;
     }
 
   </style>
diff --git a/types/config.d.ts b/types/config.d.ts
index 3ba80f0c7..88c4c89a5 100644
--- a/types/config.d.ts
+++ b/types/config.d.ts
@@ -15,7 +15,7 @@ declare global {
        */
       export interface Json {
         extends?: 'lighthouse:default' | 'lighthouse:full' | string | boolean;
-        settings?: SettingsJson;
+        settings?: SharedFlagsSettings;
         passes?: PassJson[] | null;
         audits?: Config.AuditJson[] | null;
         categories?: Record<string, CategoryJson> | null;
@@ -23,10 +23,6 @@ declare global {
         plugins?: Array<string>,
       }
 
-      export interface SettingsJson extends SharedFlagsSettings {
-        extraHeaders?: Crdp.Network.Headers | null;
-      }
-
       export interface PassJson {
         passName: string;
         recordTrace?: boolean;
@@ -80,7 +76,7 @@ declare global {
         group?: string;
       }
 
-      export interface Settings extends Required<SettingsJson> {
+      export interface Settings extends Required<SharedFlagsSettings> {
         throttling: Required<ThrottlingSettings>;
       }
 
diff --git a/types/html-renderer.d.ts b/types/html-renderer.d.ts
index de49d00d1..512329309 100644
--- a/types/html-renderer.d.ts
+++ b/types/html-renderer.d.ts
@@ -59,7 +59,8 @@ declare global {
       }
 
       export interface AuditRef extends Result.AuditRef {
-        result: Audit.Result & { stackPacks?: StackPackDescription[] }
+        result: Audit.Result;
+        stackPacks?: StackPackDescription[];
       }
 
       export interface StackPackDescription {


Test patch:
diff --git a/lighthouse-core/test/audits/font-display-test.js b/lighthouse-core/test/audits/font-display-test.js
index 7d3a22881..36b674050 100644
--- a/lighthouse-core/test/audits/font-display-test.js
+++ b/lighthouse-core/test/audits/font-display-test.js
@@ -261,4 +261,25 @@ describe('Performance: Font Display audit', () => {
     expect(result.details.items).toEqual([]);
     assert.strictEqual(result.score, 1);
   });
+
+  it('handles custom source URLs from sourcemaps', async () => {
+    // Make sure we don't use sourceURL when it's not a valid URL, see https://github.com/GoogleChrome/lighthouse/issues/8534
+    stylesheet.header.sourceURL = 'custom-url-from-source-map';
+    stylesheet.content = `
+      @font-face {
+        src: url(font-0.woff);
+        font-display: swap
+      }
+    `;
+
+    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([]);
+    assert.strictEqual(result.score, 1);
+  });
 });
diff --git a/lighthouse-core/test/report/html/renderer/util-test.js b/lighthouse-core/test/report/html/renderer/util-test.js
index d0b74574e..8d958b6d9 100644
--- a/lighthouse-core/test/report/html/renderer/util-test.js
+++ b/lighthouse-core/test/report/html/renderer/util-test.js
@@ -154,5 +154,31 @@ describe('util helpers', () => {
       const preparedResult = Util.prepareReportResult(clonedSampleResult);
       assert.deepStrictEqual(preparedResult.audits, sampleResult.audits);
     });
+
+    it('appends stack pack descriptions to auditRefs', () => {
+      const clonedSampleResult = JSON.parse(JSON.stringify(sampleResult));
+      const iconDataURL = 'data:image/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg"%3E%3C/svg%3E';
+      clonedSampleResult.stackPacks = [{
+        id: 'snackpack',
+        title: 'SnackPack',
+        iconDataURL,
+        descriptions: {
+          'unused-css-rules': 'Consider using snacks in packs.',
+        },
+      }];
+      const preparedResult = Util.prepareReportResult(clonedSampleResult);
+
+      const perfAuditRefs = preparedResult.categories.performance.auditRefs;
+      const unusedCssRef = perfAuditRefs.find(ref => ref.id === 'unused-css-rules');
+      assert.deepStrictEqual(unusedCssRef.stackPacks, [{
+        title: 'SnackPack',
+        iconDataURL,
+        description: 'Consider using snacks in packs.',
+      }]);
+
+      // No stack pack on audit wth no stack pack.
+      const interactiveRef = perfAuditRefs.find(ref => ref.id === 'interactive');
+      assert.strictEqual(interactiveRef.stackPacks, undefined);
+    });
   });
 });