Repository: GoogleChrome/lighthouse
Pull request #9301 (2019-06-28 05:55:56)

report: remove unnecessary attribute in svg
Did a quick overview of our SVGs to see if [@tomayac's advice on currentColor](https://web.dev/prefers-color-scheme/) might apply anywhere. I didn't find any, but I did find this small thing. The chevron stroke is handled in the CSS.


Patch:
diff --git a/lighthouse-core/report/html/renderer/crc-details-renderer.js b/lighthouse-core/report/html/renderer/crc-details-renderer.js
index 150064d47..0c53f0865 100644
--- a/lighthouse-core/report/html/renderer/crc-details-renderer.js
+++ b/lighthouse-core/report/html/renderer/crc-details-renderer.js
@@ -84,9 +84,10 @@ class CriticalRequestChainRenderer {
    * @param {DOM} dom
    * @param {DocumentFragment} tmpl
    * @param {CRCSegment} segment
+   * @param {DetailsRenderer} detailsRenderer
    * @return {Node}
    */
-  static createChainNode(dom, tmpl, segment) {
+  static createChainNode(dom, tmpl, segment, detailsRenderer) {
     const chainsEl = dom.cloneTemplate('#tmpl-lh-crc__chains', tmpl);
 
     // Hovering over request shows full URL.
@@ -120,10 +121,10 @@ class CriticalRequestChainRenderer {
     }
 
     // Fill in url, host, and request size information.
-    const {file, hostname} = Util.parseURL(segment.node.request.url);
+    const url = segment.node.request.url;
+    const linkEl = detailsRenderer.renderTextURL(url);
     const treevalEl = dom.find('.crc-node__tree-value', chainsEl);
-    dom.find('.crc-node__tree-file', treevalEl).textContent = `${file}`;
-    dom.find('.crc-node__tree-hostname', treevalEl).textContent = hostname ? `(${hostname})` : '';
+    treevalEl.appendChild(linkEl);
 
     if (!segment.hasChildren) {
       const {startTime, endTime, transferSize} = segment.node.request;
@@ -146,14 +147,15 @@ class CriticalRequestChainRenderer {
    * @param {CRCSegment} segment
    * @param {Element} elem Parent element.
    * @param {LH.Audit.Details.CriticalRequestChain} details
+   * @param {DetailsRenderer} detailsRenderer
    */
-  static buildTree(dom, tmpl, segment, elem, details) {
-    elem.appendChild(CriticalRequestChainRenderer.createChainNode(dom, tmpl, segment));
+  static buildTree(dom, tmpl, segment, elem, details, detailsRenderer) {
+    elem.appendChild(CRCRenderer.createChainNode(dom, tmpl, segment, detailsRenderer));
     if (segment.node.children) {
       for (const key of Object.keys(segment.node.children)) {
-        const childSegment = CriticalRequestChainRenderer.createSegment(segment.node.children, key,
+        const childSegment = CRCRenderer.createSegment(segment.node.children, key,
           segment.startTime, segment.transferSize, segment.treeMarkers, segment.isLastChild);
-        CriticalRequestChainRenderer.buildTree(dom, tmpl, childSegment, elem, details);
+        CRCRenderer.buildTree(dom, tmpl, childSegment, elem, details, detailsRenderer);
       }
     }
   }
@@ -162,9 +164,10 @@ class CriticalRequestChainRenderer {
    * @param {DOM} dom
    * @param {ParentNode} templateContext
    * @param {LH.Audit.Details.CriticalRequestChain} details
+   * @param {DetailsRenderer} detailsRenderer
    * @return {Element}
    */
-  static render(dom, templateContext, details) {
+  static render(dom, templateContext, details, detailsRenderer) {
     const tmpl = dom.cloneTemplate('#tmpl-lh-crc', templateContext);
     const containerEl = dom.find('.lh-crc', tmpl);
 
@@ -176,17 +179,19 @@ class CriticalRequestChainRenderer {
         Util.formatMilliseconds(details.longestChain.duration);
 
     // Construct visual tree.
-    const root = CriticalRequestChainRenderer.initTree(details.chains);
+    const root = CRCRenderer.initTree(details.chains);
     for (const key of Object.keys(root.tree)) {
-      const segment = CriticalRequestChainRenderer.createSegment(root.tree, key,
-          root.startTime, root.transferSize);
-      CriticalRequestChainRenderer.buildTree(dom, tmpl, segment, containerEl, details);
+      const segment = CRCRenderer.createSegment(root.tree, key, root.startTime, root.transferSize);
+      CRCRenderer.buildTree(dom, tmpl, segment, containerEl, details, detailsRenderer);
     }
 
     return dom.find('.lh-crc-container', tmpl);
   }
 }
 
+// Alias b/c the name is really long.
+const CRCRenderer = CriticalRequestChainRenderer;
+
 // Allow Node require()'ing.
 if (typeof module !== 'undefined' && module.exports) {
   module.exports = CriticalRequestChainRenderer;
diff --git a/lighthouse-core/report/html/renderer/details-renderer.js b/lighthouse-core/report/html/renderer/details-renderer.js
index 4e26b2594..84a5d49f6 100644
--- a/lighthouse-core/report/html/renderer/details-renderer.js
+++ b/lighthouse-core/report/html/renderer/details-renderer.js
@@ -53,7 +53,7 @@ class DetailsRenderer {
       case 'table':
         return this._renderTable(details);
       case 'criticalrequestchain':
-        return CriticalRequestChainRenderer.render(this._dom, this._templateContext, details);
+        return CriticalRequestChainRenderer.render(this._dom, this._templateContext, details, this);
       case 'opportunity':
         return this._renderTable(details);
 
@@ -97,7 +97,7 @@ class DetailsRenderer {
    * @param {string} text
    * @return {HTMLElement}
    */
-  _renderTextURL(text) {
+  renderTextURL(text) {
     const url = text;
 
     let displayedPath;
@@ -113,7 +113,7 @@ class DetailsRenderer {
     }
 
     const element = this._dom.createElement('div', 'lh-text__url');
-    element.appendChild(this._renderText(displayedPath));
+    element.appendChild(this._renderLink({text: displayedPath, url}));
 
     if (displayedHost) {
       const hostElem = this._renderText(displayedHost);
@@ -130,7 +130,7 @@ class DetailsRenderer {
   }
 
   /**
-   * @param {LH.Audit.Details.LinkValue} details
+   * @param {{text: string, url: string}} details
    * @return {Element}
    */
   _renderLink(details) {
@@ -215,7 +215,7 @@ class DetailsRenderer {
           return this.renderNode(value);
         }
         case 'url': {
-          return this._renderTextURL(value.value);
+          return this.renderTextURL(value.value);
         }
         default: {
           throw new Error(`Unknown valueType: ${value.type}`);
@@ -260,7 +260,7 @@ class DetailsRenderer {
       case 'url': {
         const strValue = String(value);
         if (URL_PREFIXES.some(prefix => strValue.startsWith(prefix))) {
-          return this._renderTextURL(strValue);
+          return this.renderTextURL(strValue);
         } else {
           // Fall back to <pre> rendering if not actually a URL.
           return this._renderCode(strValue);
diff --git a/lighthouse-core/report/html/report-styles.css b/lighthouse-core/report/html/report-styles.css
index 6d4b7a319..be06994c3 100644
--- a/lighthouse-core/report/html/report-styles.css
+++ b/lighthouse-core/report/html/report-styles.css
@@ -1365,12 +1365,8 @@
   width: 12%;
 }
 
-.lh-text__url:hover {
-  text-decoration: underline dotted #999;
-  text-decoration-skip-ink: auto;
-}
 
-.lh-text__url > .lh-text, .lh-text__url-host {
+.lh-text__url-host {
   display: inline;
 }
 
@@ -1386,6 +1382,15 @@
   object-fit: contain;
 }
 
+.lh-text__url > a {
+  color: inherit;
+  text-decoration: none;
+}
+
+.lh-text__url > a:hover {
+  text-decoration: underline dotted #999;
+}
+
 /* Chevron
    https://codepen.io/paulirish/pen/LmzEmK
  */
diff --git a/lighthouse-core/report/html/templates.html b/lighthouse-core/report/html/templates.html
index 92f633d0a..34f1c9698 100644
--- a/lighthouse-core/report/html/templates.html
+++ b/lighthouse-core/report/html/templates.html
@@ -34,10 +34,10 @@ limitations under the License.
 
 <!-- Toggle arrow chevron -->
 <template id="tmpl-lh-chevron">
-  <svg class="lh-chevron" title="See audits" xmlns="http://www.w3.org/2000/svg"  viewbox="0 0 100 100">
+  <svg class="lh-chevron" title="See audits" xmlns="http://www.w3.org/2000/svg" viewbox="0 0 100 100">
     <g class="lh-chevron__lines">
-      <path class="lh-chevron__line lh-chevron__line-left" d="M10 50h40" stroke="#707173"></path>
-      <path class="lh-chevron__line lh-chevron__line-right" d="M90 50H50" stroke="#707173"></path>
+      <path class="lh-chevron__line lh-chevron__line-left" d="M10 50h40"></path>
+      <path class="lh-chevron__line lh-chevron__line-right" d="M90 50H50"></path>
     </g>
   </svg>
 </template>
@@ -741,12 +741,12 @@ limitations under the License.
       .lh-crc .crc-node__tree-value {
         margin-left: 10px;
       }
+      .lh-crc .crc-node__tree-value div {
+        display: inline;
+      }
       .lh-crc .crc-node__chain-duration {
         font-weight: 700;
       }
-      .lh-crc .crc-node__tree-hostname {
-        color: #595959;
-      }
       .lh-crc .crc-initial-nav {
         color: #595959;
         font-style: italic;
@@ -769,8 +769,6 @@ limitations under the License.
 
           </span>
           <span class="crc-node__tree-value">
-            <span class="crc-node__tree-file"><!-- fill me: node.request.url.file --></span>
-            <span class="crc-node__tree-hostname">(<!-- fill me: node.request.url.host -->)</span>
 
           </span>
         </div>
diff --git a/lighthouse-core/scripts/build-report-for-autodeployment.js b/lighthouse-core/scripts/build-report-for-autodeployment.js
index 5bfab5d56..7fdc9d06a 100644
--- a/lighthouse-core/scripts/build-report-for-autodeployment.js
+++ b/lighthouse-core/scripts/build-report-for-autodeployment.js
@@ -14,32 +14,33 @@ const mkdirp = require('mkdirp').sync;
 const swapLocale = require('../lib/i18n/swap-locale.js');
 
 const ReportGenerator = require('../../lighthouse-core/report/report-generator.js');
+const {defaultSettings} = require('../config/constants.js');
+const lighthouse = require('../index.js');
 const lhr = /** @type {LH.Result} */ (require('../../lighthouse-core/test/results/sample_v2.json'));
 
 const DIST = path.join(__dirname, `../../dist`);
 
-// Add a plugin to demo plugin rendering.
-lhr.categories['lighthouse-plugin-someplugin'] = {
-  id: 'lighthouse-plugin-someplugin',
-  title: 'Plugin',
-  score: 0.5,
-  auditRefs: [],
-};
-
 (async function() {
+  addPluginCategory(lhr);
+  const errorLhr = await generateErrorLHR();
+
   const filenameToLhr = {
     'english': lhr,
     'espanol': swapLocale(lhr, 'es').lhr,
     'arabic': swapLocale(lhr, 'ar').lhr,
+    'xl-accented': swapLocale(lhr, 'en-XL').lhr,
+    'error': errorLhr,
   };
 
-  mkdirp(DIST);
-
   // Generate and write reports
   Object.entries(filenameToLhr).forEach(([filename, lhr]) => {
     let html = ReportGenerator.generateReportHtml(lhr);
+    // TODO: PSI is another variant to consider
     for (const variant of ['', '-devtools']) {
       if (variant === '-devtools') {
+        // TODO: Make the DevTools Audits panel "emulation" more comprehensive
+        // - the parent widget/vbox container with overflow
+        // - a more constrained/realistic default size
         html = html.replace(`"lh-root lh-vars"`, `"lh-root lh-vars lh-devtools"`);
       }
       const filepath = `${DIST}/${filename}${variant}/index.html`;
@@ -49,3 +50,79 @@ lhr.categories['lighthouse-plugin-someplugin'] = {
     }
   });
 })();
+
+/**
+ * Add a plugin to demo plugin rendering.
+ * @param {LH.Result} lhr
+ */
+function addPluginCategory(lhr) {
+  lhr.categories['lighthouse-plugin-someplugin'] = {
+    id: 'lighthouse-plugin-someplugin',
+    title: 'Plugin',
+    score: 0.5,
+    auditRefs: [],
+  };
+}
+
+/**
+ * Generate an LHR with errors for the renderer to display.
+ * We'll write an "empty" artifacts file to disk, only to use it in auditMode.
+ * @return {Promise<LH.Result>}
+ */
+async function generateErrorLHR() {
+  /** @type {LH.BaseArtifacts} */
+  const artifacts = {
+    fetchTime: '2019-06-26T23:56:58.381Z',
+    LighthouseRunWarnings: [
+      `Something went wrong with recording the trace over your page load. Please run Lighthouse again. (NO_FCP)`, // eslint-disable-line max-len
+    ],
+    TestedAsMobileDevice: true,
+    HostUserAgent: 'Mozilla/5.0 ErrorUserAgent Chrome/66',
+    NetworkUserAgent: 'Mozilla/5.0 ErrorUserAgent Chrome/66',
+    BenchmarkIndex: 1000,
+    WebAppManifest: null,
+    Stacks: [],
+    settings: defaultSettings,
+    URL: {
+      requestedUrl: 'http://fakeurl.com',
+      finalUrl: 'http://fakeurl.com',
+    },
+    Timing: [],
+    PageLoadError: null,
+    devtoolsLogs: {},
+    traces: {},
+  };
+
+  // Save artifacts to disk then run `lighthouse -G` with them.
+  const TMP = `${DIST}/.tmp/`;
+  mkdirp(TMP);
+  fs.writeFileSync(`${TMP}/artifacts.json`, JSON.stringify(artifacts), 'utf-8');
+  const errorRunnerResult = await lighthouse(artifacts.URL.requestedUrl, {auditMode: TMP});
+
+  if (!errorRunnerResult) throw new Error('Failed to run lighthouse on empty artifacts');
+  const errorLhr = errorRunnerResult.lhr;
+
+  // Add audit warnings to font-display
+  errorLhr.audits['font-display'].warnings = [
+    'Lighthouse was unable to automatically check the font-display value for the following URL: https://secure-ds.serving-sys.com/resources/PROD/html5/105657/20190307/1074580285/43862346571980472/fonts/IBMPlexSans-Light-Latin1.woff.',
+    'Lighthouse was unable to automatically check the font-display value for the following URL: https://secure-ds.serving-sys.com/resources/PROD/html5/105657/20190307/1074580285/43862346571980472/fonts/IBMPlexSans-Bold-Latin1.woff.',
+  ];
+  // perf/offscreen-images - set as passing but with a warning
+  const offscreenImagesAudit = errorLhr.audits['offscreen-images'];
+  offscreenImagesAudit.warnings = [
+    'Invalid image sizing information: https://cdn.cnn.com/cnn/.e1mo/img/4.0/vr/vr_new_asset.png',
+  ];
+  offscreenImagesAudit.errorMessage = undefined;
+  offscreenImagesAudit.scoreDisplayMode = 'binary';
+  offscreenImagesAudit.score = 1;
+  // pwa-apple-touch-icon - set as passing but with a warning
+  const appleTouchIconAudit = errorLhr.audits['apple-touch-icon'];
+  appleTouchIconAudit.warnings = [
+    '`apple-touch-icon-precomposed` is out of date; `apple-touch-icon` is preferred.',
+  ];
+  appleTouchIconAudit.errorMessage = undefined;
+  appleTouchIconAudit.scoreDisplayMode = 'binary';
+  appleTouchIconAudit.score = 1;
+
+  return errorLhr;
+}


Test patch:
diff --git a/lighthouse-core/test/report/html/renderer/crc-details-renderer-test.js b/lighthouse-core/test/report/html/renderer/crc-details-renderer-test.js
index 79de69ef5..f5d8ca14c 100644
--- a/lighthouse-core/test/report/html/renderer/crc-details-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/crc-details-renderer-test.js
@@ -13,6 +13,7 @@ const jsdom = require('jsdom');
 const URL = require('../../../../lib/url-shim.js');
 const Util = require('../../../../report/html/renderer/util.js');
 const DOM = require('../../../../report/html/renderer/dom.js');
+const DetailsRenderer = require('../../../../report/html/renderer/details-renderer.js');
 const CriticalRequestChainRenderer =
     require('../../../../report/html/renderer/crc-details-renderer.js');
 
@@ -75,12 +76,14 @@ const DETAILS = {
 
 describe('DetailsRenderer', () => {
   let dom;
+  let detailsRenderer;
 
   beforeAll(() => {
     global.URL = URL;
     global.Util = Util;
     const {document} = new jsdom.JSDOM(TEMPLATE_FILE).window;
     dom = new DOM(document);
+    detailsRenderer = new DetailsRenderer(dom);
   });
 
   afterAll(() => {
@@ -89,18 +92,25 @@ describe('DetailsRenderer', () => {
   });
 
   it('renders tree structure', () => {
-    const el = CriticalRequestChainRenderer.render(dom, dom.document(), DETAILS);
+    const el = CriticalRequestChainRenderer.render(dom, dom.document(), DETAILS, detailsRenderer);
     const chains = el.querySelectorAll('.crc-node');
 
     // Main request
     assert.equal(chains.length, 4, 'generates correct number of chain nodes');
-    assert.equal(chains[0].querySelector('.crc-node__tree-hostname').textContent, '(example.com)');
+    assert.ok(!chains[0].querySelector('.lh-text__url-host'), 'should be no origin for root url');
+    assert.equal(chains[0].querySelector('.lh-text__url a').textContent, 'https://example.com');
+    assert.equal(chains[0].querySelector('.lh-text__url a').href, 'https://example.com/');
+    assert.equal(chains[0].querySelector('.lh-text__url a').rel, 'noopener');
+    assert.equal(chains[0].querySelector('.lh-text__url a').target, '_blank');
 
     // Children
     assert.ok(chains[1].querySelector('.crc-node__tree-marker .vert-right'));
     assert.equal(chains[1].querySelectorAll('.crc-node__tree-marker .right').length, 2);
-    assert.equal(chains[1].querySelector('.crc-node__tree-file').textContent, '/b.js');
-    assert.equal(chains[1].querySelector('.crc-node__tree-hostname').textContent, '(example.com)');
+    assert.equal(chains[1].querySelector('.lh-text__url a').textContent, '/b.js');
+    assert.equal(chains[1].querySelector('.lh-text__url a').href, 'https://example.com/b.js');
+    assert.equal(chains[1].querySelector('.lh-text__url a').rel, 'noopener');
+    assert.equal(chains[1].querySelector('.lh-text__url a').target, '_blank');
+    assert.equal(chains[1].querySelector('.lh-text__url-host').textContent, '(example.com)');
     const durationNodes = chains[1].querySelectorAll('.crc-node__chain-duration');
     assert.equal(durationNodes[0].textContent, ' - 5,000\xa0ms, ');
     // Note: actual transferSize is 2000 bytes but formatter formats to KBs.
diff --git a/lighthouse-core/test/report/html/renderer/details-renderer-test.js b/lighthouse-core/test/report/html/renderer/details-renderer-test.js
index 261172a73..ba7cf34c7 100644
--- a/lighthouse-core/test/report/html/renderer/details-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/details-renderer-test.js
@@ -405,7 +405,10 @@ describe('DetailsRenderer', () => {
       assert.equal(urlEl.localName, 'div');
       assert.equal(urlEl.title, urlText);
       assert.equal(urlEl.dataset.url, urlText);
-      assert.ok(urlEl.firstChild.classList.contains('lh-text'));
+      assert.equal(urlEl.firstChild.nodeName, 'A');
+      assert.equal(urlEl.firstChild.href, urlText);
+      assert.equal(urlEl.firstChild.rel, 'noopener');
+      assert.equal(urlEl.firstChild.target, '_blank');
       assert.equal(urlEl.textContent, displayUrlText);
     });
 
@@ -430,7 +433,7 @@ describe('DetailsRenderer', () => {
       assert.equal(urlEl.localName, 'div');
       assert.equal(urlEl.title, urlText);
       assert.equal(urlEl.dataset.url, urlText);
-      assert.ok(urlEl.firstChild.classList.contains('lh-text'));
+      assert.equal(urlEl.firstChild.nodeName, 'A');
       assert.equal(urlEl.textContent, displayUrlText);
     });
 
diff --git a/lighthouse-core/test/report/html/renderer/report-ui-features-test.js b/lighthouse-core/test/report/html/renderer/report-ui-features-test.js
index d706fc4f8..2beb09fec 100644
--- a/lighthouse-core/test/report/html/renderer/report-ui-features-test.js
+++ b/lighthouse-core/test/report/html/renderer/report-ui-features-test.js
@@ -185,7 +185,7 @@ describe('ReportUIFeatures', () => {
 
         function getUrlsInTable() {
           return dom
-            .findAll('#uses-webp-images .lh-details .lh-text__url .lh-text:first-child', container)
+            .findAll('#uses-webp-images .lh-details .lh-text__url a:first-child', container)
             .map(el => el.textContent);
         }