Repository: GoogleChrome/lighthouse
Pull request #9186 (2019-06-11 19:24:49)

report: position sticky-header highlighter with css grid
alternative fix to #9167

Putting the sticky-header highlighter in the first gauge was a nice way to get it positioned correctly on load and a good transform origin to translate from on scroll, but it ends up a little weird since it's fundamentally part of the sticky header, not a single gauge.

Moving it out of the gauge, though, I couldn't find a way to get an absolutely positioned element to be aligned with the first gauge using flexbox. CSS grid makes this really easy, though, so switched the sticky header to using that.


Patch:
diff --git a/clients/lightrider/lightrider-entry.js b/clients/lightrider/lightrider-entry.js
index 5bfb7ce78..49cfa5561 100644
--- a/clients/lightrider/lightrider-entry.js
+++ b/clients/lightrider/lightrider-entry.js
@@ -24,9 +24,9 @@ const LR_PRESETS = {
  * If configOverride is provided, lrDevice and categoryIDs are ignored.
  * @param {Connection} connection
  * @param {string} url
- * @param {LH.Flags} flags Lighthouse flags, including `output`
+ * @param {LH.Flags} flags Lighthouse flags
  * @param {{lrDevice?: 'desktop'|'mobile', categoryIDs?: Array<string>, logAssets: boolean, configOverride?: LH.Config.Json}} lrOpts Options coming from Lightrider
- * @return {Promise<string|Array<string>|void>}
+ * @return {Promise<string>}
  */
 async function runLighthouseInLR(connection, url, flags, lrOpts) {
   const {lrDevice, categoryIDs, logAssets, configOverride} = lrOpts;
@@ -52,22 +52,17 @@ async function runLighthouseInLR(connection, url, flags, lrOpts) {
 
   try {
     const runnerResult = await lighthouse(url, flags, config, connection);
-    if (!runnerResult) return;
+    if (!runnerResult) throw new Error('Lighthouse finished without a runnerResult');
 
-    // pre process the LHR for proto
-    if (flags.output === 'json' && typeof runnerResult.report === 'string') {
-      // When LR is called with |internal: {keep_raw_response: true, save_lighthouse_assets: true}|,
-      // this code will log artifacts to raw_response.artifacts.
-      if (logAssets) {
-        // @ts-ignore - Regenerate the report, but tack on the artifacts.
-        runnerResult.lhr.artifacts = runnerResult.artifacts;
-        runnerResult.report = JSON.stringify(runnerResult.lhr);
-      }
-
-      return preprocessor.processForProto(runnerResult.report);
+    // When LR is called with |internal: {keep_raw_response: true, save_lighthouse_assets: true}|,
+    // this code will log artifacts to raw_response.artifacts.
+    if (logAssets) {
+      // @ts-ignore - piggyback the artifacts on the LHR.
+      runnerResult.lhr.artifacts = runnerResult.artifacts;
     }
 
-    return runnerResult.report;
+    // pre process the LHR for proto
+    return JSON.stringify(preprocessor.processForProto(runnerResult.lhr));
   } catch (err) {
     // If an error ruined the entire lighthouse run, attempt to return a meaningful error.
     let runtimeError;
diff --git a/lighthouse-core/lib/proto-preprocessor.js b/lighthouse-core/lib/proto-preprocessor.js
index 0db83e1fe..7f5142b95 100644
--- a/lighthouse-core/lib/proto-preprocessor.js
+++ b/lighthouse-core/lib/proto-preprocessor.js
@@ -16,11 +16,13 @@ const fs = require('fs');
  */
 
 /**
-  * @param {string} result
+  * Transform an LHR into a proto-friendly, mostly-compatible LHR.
+  * @param {LH.Result} lhr
+  * @return {LH.Result}
   */
-function processForProto(result) {
+function processForProto(lhr) {
   /** @type {LH.Result} */
-  const reportJson = JSON.parse(result);
+  const reportJson = JSON.parse(JSON.stringify(lhr));
 
   // Clean up the configSettings
   // Note: This is not strictly required for conversion if protobuf parsing is set to
@@ -95,7 +97,7 @@ function processForProto(result) {
 
   removeStrings(reportJson);
 
-  return JSON.stringify(reportJson);
+  return reportJson;
 }
 
 // @ts-ignore claims always false, but this checks if cli or module
@@ -113,9 +115,9 @@ if (require.main === module) {
 
   if (input && output) {
     // process the file
-    const report = processForProto(fs.readFileSync(input, 'utf-8'));
+    const report = processForProto(JSON.parse(fs.readFileSync(input, 'utf-8')));
     // write to output from argv
-    fs.writeFileSync(output, report, 'utf-8');
+    fs.writeFileSync(output, JSON.stringify(report), 'utf-8');
   }
 } else {
   module.exports = {
diff --git a/lighthouse-core/report/html/renderer/pwa-category-renderer.js b/lighthouse-core/report/html/renderer/pwa-category-renderer.js
index 0c96c6015..1b49cff5c 100644
--- a/lighthouse-core/report/html/renderer/pwa-category-renderer.js
+++ b/lighthouse-core/report/html/renderer/pwa-category-renderer.js
@@ -18,6 +18,16 @@
 
 /* globals self, Util, CategoryRenderer */
 
+/**
+ * An always-increasing counter for making unique SVG ID suffixes.
+ */
+const getUniqueSuffix = (() => {
+  let svgSuffix = 0;
+  return function() {
+    return svgSuffix++;
+  };
+})();
+
 class PwaCategoryRenderer extends CategoryRenderer {
   /**
    * @param {LH.ReportResult.Category} category
@@ -62,6 +72,11 @@ class PwaCategoryRenderer extends CategoryRenderer {
       tmpl));
     wrapper.href = `#${category.id}`;
 
+    // Correct IDs in case multiple instances end up in the page.
+    const svgRoot = tmpl.querySelector('svg');
+    if (!svgRoot) throw new Error('no SVG element found in PWA score gauge template');
+    PwaCategoryRenderer._makeSvgReferencesUnique(svgRoot);
+
     const allGroups = this._getGroupIds(category.auditRefs);
     const passingGroupIds = this._getPassingGroupIds(category.auditRefs);
 
@@ -147,6 +162,38 @@ class PwaCategoryRenderer extends CategoryRenderer {
 
     return auditsElem;
   }
+
+  /**
+   * Alters SVG id references so multiple instances of an SVG element can coexist
+   * in a single page. If `svgRoot` has a `<defs>` block, gives all elements defined
+   * in it unique ids, then updates id references (`<use xlink:href="...">`,
+   * `fill="url(#...)"`) to the altered ids in all descendents of `svgRoot`.
+   * @param {SVGElement} svgRoot
+   */
+  static _makeSvgReferencesUnique(svgRoot) {
+    const defsEl = svgRoot.querySelector('defs');
+    if (!defsEl) return;
+
+    const idSuffix = getUniqueSuffix();
+    const elementsToUpdate = defsEl.querySelectorAll('[id]');
+    for (const el of elementsToUpdate) {
+      const oldId = el.id;
+      const newId = `${oldId}-${idSuffix}`;
+      el.id = newId;
+
+      // Update all <use>s.
+      const useEls = svgRoot.querySelectorAll(`use[href="#${oldId}"]`);
+      for (const useEl of useEls) {
+        useEl.setAttribute('href', `#${newId}`);
+      }
+
+      // Update all fill="url(#...)"s.
+      const fillEls = svgRoot.querySelectorAll(`[fill="url(#${oldId})"]`);
+      for (const fillEl of fillEls) {
+        fillEl.setAttribute('fill', `url(#${newId})`);
+      }
+    }
+  }
 }
 
 if (typeof module !== 'undefined' && module.exports) {
diff --git a/lighthouse-core/report/html/renderer/report-ui-features.js b/lighthouse-core/report/html/renderer/report-ui-features.js
index 213199757..df9583dac 100644
--- a/lighthouse-core/report/html/renderer/report-ui-features.js
+++ b/lighthouse-core/report/html/renderer/report-ui-features.js
@@ -314,9 +314,8 @@ class ReportUIFeatures {
     this.scoreScaleEl = this._dom.find('.lh-scorescale', this._document);
     this.stickyHeaderEl = this._dom.find('.lh-sticky-header', this._document);
 
-    // Position highlighter at first gauge; will be transformed on scroll.
-    const firstGauge = this._dom.find('.lh-gauge__wrapper', this.stickyHeaderEl);
-    this.highlightEl = this._dom.createChildOf(firstGauge, 'div', 'lh-highlighter');
+    // Highlighter will be absolutely positioned at first gauge, then transformed on scroll.
+    this.highlightEl = this._dom.createChildOf(this.stickyHeaderEl, 'div', 'lh-highlighter');
   }
 
   /**
diff --git a/lighthouse-core/report/html/report-styles.css b/lighthouse-core/report/html/report-styles.css
index f7690d342..62d1cb231 100644
--- a/lighthouse-core/report/html/report-styles.css
+++ b/lighthouse-core/report/html/report-styles.css
@@ -144,37 +144,39 @@
   --pwa-optimized-color-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><rect fill="%230CCE6B" width="24" height="24" rx="12"/><path d="M5 5h14v14H5z"/><path fill="%23FFF" d="M12 15.07l3.6 2.18-.95-4.1 3.18-2.76-4.2-.36L12 6.17l-1.64 3.86-4.2.36 3.2 2.76-.96 4.1z"/></g></svg>');
 }
 
-.lh-vars.dark {
-  --color-red-700: var(--color-red);
-  --color-green-700: var(--color-green);
-  --color-teal-600: var(--color-cyan-500);
-  --color-orange-700: var(--color-orange);
-
-  --color-black-200: var(--color-black-800);
-  --color-black-400: var(--color-black-600);
-  --color-black-600: var(--color-black-500);
-
-  --topbar-bg: var(--color-black);
-  --plugin-badge-bg: var(--color-black-800);
-  --env-item-bg: var(--color-black);
-  --report-secondary-border-color: var(--color-black-200);
-
-  --body-background-color: var(--color-black-900);
-  --body-text-color: var(--color-black-100);
-  --secondary-text-color: var(--color-black-400);
-
-  --plugin-icon-url: var(--plugin-icon-url-dark);
-
-  --informative-color: var(--color-blue-200);
-
-  --medium-50-gray: #757575;
-  --medium-75-gray: var(--color-white);
-
-  --color-hover: rgba(0, 0, 0, 0.2);
-
-  --pwa-fast-reliable-gray-url: var(--pwa-fast-reliable-gray-url-dark);
-  --pwa-installable-gray-url: var(--pwa-installable-gray-url-dark);
-  --pwa-optimized-gray-url: var(--pwa-optimized-gray-url-dark);
+@media not print {
+  .lh-vars.dark {
+    --color-red-700: var(--color-red);
+    --color-green-700: var(--color-green);
+    --color-teal-600: var(--color-cyan-500);
+    --color-orange-700: var(--color-orange);
+  
+    --color-black-200: var(--color-black-800);
+    --color-black-400: var(--color-black-600);
+    --color-black-600: var(--color-black-500);
+  
+    --topbar-bg: var(--color-black);
+    --plugin-badge-bg: var(--color-black-800);
+    --env-item-bg: var(--color-black);
+    --report-secondary-border-color: var(--color-black-200);
+  
+    --body-background-color: var(--color-black-900);
+    --body-text-color: var(--color-black-100);
+    --secondary-text-color: var(--color-black-400);
+  
+    --plugin-icon-url: var(--plugin-icon-url-dark);
+  
+    --informative-color: var(--color-blue-200);
+  
+    --medium-50-gray: #757575;
+    --medium-75-gray: var(--color-white);
+  
+    --color-hover: rgba(0, 0, 0, 0.2);
+  
+    --pwa-fast-reliable-gray-url: var(--pwa-fast-reliable-gray-url-dark);
+    --pwa-installable-gray-url: var(--pwa-installable-gray-url-dark);
+    --pwa-optimized-gray-url: var(--pwa-optimized-gray-url-dark);
+  }
 }
 
 @media only screen and (max-width: 480px) {
diff --git a/lighthouse-core/report/html/templates.html b/lighthouse-core/report/html/templates.html
index 2e6d9cc32..0323cf54d 100644
--- a/lighthouse-core/report/html/templates.html
+++ b/lighthouse-core/report/html/templates.html
@@ -186,7 +186,8 @@ limitations under the License.
     }
 
     .lh-sticky-header--visible {
-      display: flex;
+      display: grid;
+      grid-auto-flow: column;
       pointer-events: auto;
     }
 
@@ -204,8 +205,10 @@ limitations under the License.
       width: var(--score-container-width);
       height: 1px;
       background: var(--color-highlighter-bg);
+      /* Position at bottom of first gauge in sticky header. */
       position: absolute;
-      bottom: -5px;
+      grid-column: 1;
+      bottom: -1px;
     }
 
     .lh-gauge__wrapper:first-of-type {
@@ -675,24 +678,24 @@ limitations under the License.
         <!-- Just fast and reliable. -->
         <g class="lh-gauge--pwa__component lh-gauge--pwa__fast-reliable-badge" transform="translate(20, 29)">
           <path fill="url(#lh-gauge--pwa__fast-reliable__shadow-gradient)" d="M33.63 19.49A30 30 0 0 1 16.2 30.36L3 17.14 17.14 3l16.49 16.49z"/>
-          <use xlink:href="#lh-gauge--pwa__fast-reliable-badge" />
+          <use href="#lh-gauge--pwa__fast-reliable-badge" />
         </g>
 
         <!-- Just installable. -->
         <g class="lh-gauge--pwa__component lh-gauge--pwa__installable-badge" transform="translate(20, 29)">
           <path fill="url(#lh-gauge--pwa__installable__shadow-gradient)" d="M33.629 19.487c-4.272 5.453-10.391 9.39-17.415 10.869L3 17.142 17.142 3 33.63 19.487z"/>
-          <use xlink:href="#lh-gauge--pwa__installable-badge" />
+          <use href="#lh-gauge--pwa__installable-badge" />
         </g>
 
         <!-- Fast and reliable and installable. -->
         <g class="lh-gauge--pwa__component lh-gauge--pwa__fast-reliable-installable-badges">
           <g transform="translate(8, 29)"> <!-- fast and reliable -->
             <path fill="url(#lh-gauge--pwa__fast-reliable__shadow-gradient)" d="M16.321 30.463L3 17.143 17.142 3l22.365 22.365A29.864 29.864 0 0 1 22 31c-1.942 0-3.84-.184-5.679-.537z"/>
-            <use xlink:href="#lh-gauge--pwa__fast-reliable-badge" />
+            <use href="#lh-gauge--pwa__fast-reliable-badge" />
           </g>
           <g transform="translate(32, 29)"> <!-- installable -->
             <path fill="url(#lh-gauge--pwa__installable__shadow-gradient)" d="M25.982 11.84a30.107 30.107 0 0 1-13.08 15.203L3 17.143 17.142 3l8.84 8.84z"/>
-            <use xlink:href="#lh-gauge--pwa__installable-badge" />
+            <use href="#lh-gauge--pwa__installable-badge" />
           </g>
         </g>
 


Test patch:
diff --git a/lighthouse-core/test/lib/proto-preprocessor-test.js b/lighthouse-core/test/lib/proto-preprocessor-test.js
index ca1761c88..1057a0bbe 100644
--- a/lighthouse-core/test/lib/proto-preprocessor-test.js
+++ b/lighthouse-core/test/lib/proto-preprocessor-test.js
@@ -5,10 +5,17 @@
  */
 'use strict';
 
-const processForProto = require('../../lib/proto-preprocessor.js').processForProto;
+const {processForProto} = require('../../lib/proto-preprocessor.js');
+const sampleJson = require('../results/sample_v2.json');
 
 /* eslint-env jest */
 describe('processing for proto', () => {
+  it('doesn\'t modify the input object', () => {
+    const input = JSON.parse(JSON.stringify(sampleJson));
+    processForProto(input);
+    expect(input).toEqual(sampleJson);
+  });
+
   it('keeps only necessary configSettings', () => {
     const input = {
       'configSettings': {
@@ -44,9 +51,9 @@ describe('processing for proto', () => {
         'onlyCategories': null,
       },
     };
-    const output = processForProto(JSON.stringify(input));
+    const output = processForProto(input);
 
-    expect(JSON.parse(output)).toMatchObject(expectation);
+    expect(output).toMatchObject(expectation);
   });
 
   it('cleans up default runtimeErrors', () => {
@@ -56,9 +63,9 @@ describe('processing for proto', () => {
       },
     };
 
-    const output = processForProto(JSON.stringify(input));
+    const output = processForProto(input);
 
-    expect(JSON.parse(output)).not.toHaveProperty('runtimeError');
+    expect(output).not.toHaveProperty('runtimeError');
   });
 
   it('non-default runtimeErrors are untouched', () => {
@@ -68,9 +75,9 @@ describe('processing for proto', () => {
       },
     };
 
-    const output = processForProto(JSON.stringify(input));
+    const output = processForProto(input);
 
-    expect(JSON.parse(output)).toMatchObject(input);
+    expect(output).toMatchObject(input);
   });
 
   it('cleans up audits', () => {
@@ -91,9 +98,9 @@ describe('processing for proto', () => {
         },
       },
     };
-    const output = processForProto(JSON.stringify(input));
+    const output = processForProto(input);
 
-    expect(JSON.parse(output)).toMatchObject(expectation);
+    expect(output).toMatchObject(expectation);
   });
 
 
@@ -108,9 +115,9 @@ describe('processing for proto', () => {
     const expectation = {
       'i18n': {},
     };
-    const output = processForProto(JSON.stringify(input));
+    const output = processForProto(input);
 
-    expect(JSON.parse(output)).toMatchObject(expectation);
+    expect(output).toMatchObject(expectation);
   });
 
   it('removes empty strings', () => {
@@ -151,8 +158,8 @@ describe('processing for proto', () => {
         ],
       },
     };
-    const output = processForProto(JSON.stringify(input));
+    const output = processForProto(input);
 
-    expect(JSON.parse(output)).toMatchObject(expectation);
+    expect(output).toMatchObject(expectation);
   });
 });
diff --git a/lighthouse-core/test/report/html/renderer/pwa-category-renderer-test.js b/lighthouse-core/test/report/html/renderer/pwa-category-renderer-test.js
index 8bbe83b1d..e71a81f5d 100644
--- a/lighthouse-core/test/report/html/renderer/pwa-category-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/pwa-category-renderer-test.js
@@ -242,7 +242,7 @@ describe('PwaCategoryRenderer', () => {
   describe('#renderScoreGauge', () => {
     it('renders an error score gauge in case of category error', () => {
       category.score = null;
-      const badgeGauge = pwaRenderer.renderScoreGauge(category);
+      const badgeGauge = pwaRenderer.renderScoreGauge(category, sampleResults.categoryGroups);
 
       // Not a PWA gauge.
       assert.strictEqual(badgeGauge.querySelector('.lh-gauge--pwa__wrapper'), null);
@@ -251,5 +251,37 @@ describe('PwaCategoryRenderer', () => {
       assert.strictEqual(percentageElem.textContent, '?');
       assert.strictEqual(percentageElem.title, Util.UIStrings.errorLabel);
     });
+
+    it('renders score gauges with unique ids for items in <defs>', () => {
+      const gauge1 = pwaRenderer.renderScoreGauge(category, sampleResults.categoryGroups);
+      const gauge1Ids = [...gauge1.querySelectorAll('defs [id]')].map(el => el.id);
+      assert.ok(gauge1Ids.length > 3);
+
+      const gauge2 = pwaRenderer.renderScoreGauge(category, sampleResults.categoryGroups);
+      const gauge2Ids = [...gauge2.querySelectorAll('defs [id]')].map(el => el.id);
+      assert.ok(gauge2Ids.length === gauge1Ids.length);
+
+      /** Returns whether id is used by at least one <use> or fill under `rootEl`. */
+      function idInUseElOrFillAttr(rootEl, id) {
+        const isUse = rootEl.querySelector(`use[href="#${id}"]`);
+        const isFill = rootEl.querySelector(`[fill="url(#${id})"]`);
+
+        return !!(isUse || isFill);
+      }
+
+      // Check that each gauge1 ID is actually used in gauge1 and isn't used in gauge2.
+      for (const gauge1Id of gauge1Ids) {
+        assert.equal(idInUseElOrFillAttr(gauge1, gauge1Id), true);
+        assert.ok(!gauge2Ids.includes(gauge1Id));
+        assert.equal(idInUseElOrFillAttr(gauge2, gauge1Id), false);
+      }
+
+      // And that the reverse is true for gauge2 IDs.
+      for (const gauge2Id of gauge2Ids) {
+        assert.equal(idInUseElOrFillAttr(gauge1, gauge2Id), false);
+        assert.equal(idInUseElOrFillAttr(gauge2, gauge2Id), true);
+        assert.ok(!gauge1Ids.includes(gauge2Id));
+      }
+    });
   });
 });
diff --git a/lighthouse-core/test/report/proto-test.js b/lighthouse-core/test/report/proto-test.js
index a7d051c8b..4425924c7 100644
--- a/lighthouse-core/test/report/proto-test.js
+++ b/lighthouse-core/test/report/proto-test.js
@@ -5,60 +5,57 @@
  */
 'use strict';
 
-const path = require('path');
-const fs = require('fs');
-
-const sample = fs.readFileSync(path.resolve(__dirname, '../results/sample_v2.json'));
+const sampleJson = require('../results/sample_v2.json');
 const roundTripJson = require('../../../proto/sample_v2_round_trip.json');
 const preprocessor = require('../../lib/proto-preprocessor.js');
 
 /* eslint-env jest */
 
 describe('round trip JSON comparison subsets', () => {
-  let sampleJson;
+  let processedLHR;
 
   beforeEach(() => {
-    sampleJson = JSON.parse(preprocessor.processForProto(sample));
+    processedLHR = preprocessor.processForProto(sampleJson);
   });
 
   it('has the same audit results and details (if applicable)', () => {
-    for (const auditId of Object.keys(sampleJson.audits)) {
-      expect(roundTripJson.audits[auditId]).toEqual(sampleJson.audits[auditId]);
+    for (const auditId of Object.keys(processedLHR.audits)) {
+      expect(roundTripJson.audits[auditId]).toEqual(processedLHR.audits[auditId]);
     }
   });
 
   it('has the same i18n rendererFormattedStrings', () => {
-    expect(roundTripJson.i18n).toMatchObject(sampleJson.i18n);
+    expect(roundTripJson.i18n).toMatchObject(processedLHR.i18n);
   });
 
   it('has the same top level values', () => {
     // Don't test all top level properties that are objects.
-    Object.keys(sampleJson).forEach(audit => {
-      if (typeof sampleJson[audit] === 'object' && !Array.isArray(sampleJson[audit])) {
-        delete sampleJson[audit];
+    Object.keys(processedLHR).forEach(audit => {
+      if (typeof processedLHR[audit] === 'object' && !Array.isArray(processedLHR[audit])) {
+        delete processedLHR[audit];
       }
     });
 
     // Properties set to their type's default value will be omitted in the roundTripJson.
     // For an explicit list of properties, remove sampleJson values if set to a default.
-    if (Array.isArray(sampleJson.stackPacks) && sampleJson.stackPacks.length === 0) {
-      delete sampleJson.stackPacks;
+    if (Array.isArray(processedLHR.stackPacks) && processedLHR.stackPacks.length === 0) {
+      delete processedLHR.stackPacks;
     }
 
-    expect(roundTripJson).toMatchObject(sampleJson);
+    expect(roundTripJson).toMatchObject(processedLHR);
   });
 
   it('has the same config values', () => {
     // Config settings from proto round trip should be a subset of the actual settings.
-    expect(sampleJson.configSettings).toMatchObject(roundTripJson.configSettings);
+    expect(processedLHR.configSettings).toMatchObject(roundTripJson.configSettings);
   });
 });
 
 describe('round trip JSON comparison to everything', () => {
-  let sampleJson;
+  let processedLHR;
 
   beforeEach(() => {
-    sampleJson = JSON.parse(preprocessor.processForProto(sample));
+    processedLHR = preprocessor.processForProto(sampleJson);
 
     // Proto conversion turns empty summaries into null. This is OK,
     // and is handled in the PSI roundtrip just fine, but messes up the easy
@@ -71,6 +68,6 @@ describe('round trip JSON comparison to everything', () => {
   });
 
   it('has the same JSON overall', () => {
-    expect(sampleJson).toMatchObject(roundTripJson);
+    expect(processedLHR).toMatchObject(roundTripJson);
   });
 });