Repository: GoogleChrome/lighthouse
Pull request #8629 (2019-04-25 20:16:05)

report(redesign): three dots for export button
#8185

@yuinchien the dots in your design use the material fonts icon font. I believe adding a font for just that is not ideal (@paulirish can confirm). I was hoping the unicode character ⋮ would suffice, but the spacing between dots is off.

yuinchien: @Hoten i understand your concern, but it's important to have a consistent icon style throughout the report. The source file (svg and png) can be access here: https://icons.googleplex.com/#icon=ic_more_vert&search=more 

Can we do inline svg instead of loading the whole font file? 
googlebot: We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors.  If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA ([login here](https://cla.developers.google.com/) to double check)?  If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2FGoogleChrome%2Flighthouse%2Fpull%2F8629) for more info**.

<!-- need_author_cla -->
brendankenny: proposal: what if we kept the share button on desktop and only used the three dots on a mobile breakpoint?

The share icon communicates more about what will contained in the drop down and it looks less like we took a mobile UI and blew it up to desktop size (and in devtools we won't suffer from a double three-dot menu icon)
Hoten: > proposal: what if we kept the share button on desktop and only used the three dots on a mobile breakpoint?
> 
> The share icon communicates more about what will contained in the drop down and it looks less like we took a mobile UI and blew it up to desktop size (and in devtools we won't suffer from a double three-dot menu icon)

@yuinchien 
yuinchien: @Hoten thanks for the suggestion. I realized the double menu icon situation after reading it, they are not aligned vertically or having similar sizes that you'd confuse the chrome system icon versus the menu icon in the report. It's also better keep the icon consistent than switching it for diff form factors.

I did start with the share icon in my prototype, but visually they are huge and don't look pretty. Our design update strives for simplicity, I think the menu icon serves its purpose, people are used to access it for more actions. Can we keep the current design and track user's behaviour? If click rate drop significantly we can revisit the design :)
paulirish: i'm not following the thread here too well... where are we on this?
Hoten: @brendankenny suggested keeping the share icon for desktop. @yuinchien gave reason not to.

Patch:
diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js
index 42672983c..d309072f7 100644
--- a/lighthouse-core/config/default-config.js
+++ b/lighthouse-core/config/default-config.js
@@ -389,7 +389,7 @@ const defaultConfig = {
         {id: 'bootup-time', weight: 0, group: 'diagnostics'},
         {id: 'mainthread-work-breakdown', weight: 0, group: 'diagnostics'},
         {id: 'font-display', weight: 0, group: 'diagnostics'},
-        {id: 'performance-budget', weight: 0},
+        {id: 'performance-budget', weight: 0, group: 'budgets'},
         {id: 'resource-summary', weight: 0, group: 'diagnostics'},
         // Audits past this point don't belong to a group and will not be shown automatically
         {id: 'network-requests', weight: 0},
diff --git a/lighthouse-core/report/html/renderer/performance-category-renderer.js b/lighthouse-core/report/html/renderer/performance-category-renderer.js
index 943f80d69..8ca8c4539 100644
--- a/lighthouse-core/report/html/renderer/performance-category-renderer.js
+++ b/lighthouse-core/report/html/renderer/performance-category-renderer.js
@@ -164,6 +164,20 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
       filmstripEl && timelineEl.appendChild(filmstripEl);
     }
 
+    // Budgets
+    const budgetAudit = category.auditRefs.find(audit => audit.id === 'performance-budget');
+    if (budgetAudit && budgetAudit.result.details) {
+      const table = this.detailsRenderer.render(budgetAudit.result.details);
+      if (table) {
+        table.id = budgetAudit.id;
+        table.classList.add('lh-audit');
+        const budgetsGroupEl = this.renderAuditGroup(groups.budgets);
+        budgetsGroupEl.appendChild(table);
+        budgetsGroupEl.classList.add('lh-audit-group--budgets');
+        element.appendChild(budgetsGroupEl);
+      }
+    }
+
     // Opportunities
     const opportunityAudits = category.auditRefs
         .filter(audit => audit.group === 'load-opportunities' && !Util.showAsPassed(audit.result))
diff --git a/lighthouse-core/report/html/renderer/report-renderer.js b/lighthouse-core/report/html/renderer/report-renderer.js
index 813ae2024..928dc4742 100644
--- a/lighthouse-core/report/html/renderer/report-renderer.js
+++ b/lighthouse-core/report/html/renderer/report-renderer.js
@@ -242,17 +242,6 @@ class ReportRenderer {
       wrapper.appendChild(renderer.render(category, report.categoryGroups));
     }
 
-    // Fireworks
-    const scoresAll100 = report.reportCategories.every(cat => cat.score === 1);
-    if (!this._dom.isDevTools() && scoresAll100) {
-      const scoresContainer = this._dom.find('.lh-scores-container', headerContainer);
-      scoresContainer.classList.add('score100');
-      this._dom._document.body.classList.add('dark');
-      scoresContainer.addEventListener('click', _ => {
-        scoresContainer.classList.toggle('fireworks-paused');
-      });
-    }
-
     if (scoreHeader) {
       const scoreGauges =
         this._renderScoreGauges(report, categoryRenderer, specificCategoryRenderers);
diff --git a/lighthouse-core/report/html/renderer/report-ui-features.js b/lighthouse-core/report/html/renderer/report-ui-features.js
index e0733f3b5..2458fa5e6 100644
--- a/lighthouse-core/report/html/renderer/report-ui-features.js
+++ b/lighthouse-core/report/html/renderer/report-ui-features.js
@@ -89,7 +89,27 @@ class ReportUIFeatures {
     this._document.addEventListener('keyup', this.onKeyUp);
     this._document.addEventListener('copy', this.onCopy);
     const topbarLogo = this._dom.find('.lh-topbar__logo', this._document);
-    topbarLogo.addEventListener('click', this._toggleDarkTheme);
+    topbarLogo.addEventListener('click', () => this._toggleDarkTheme());
+
+    let turnOffTheLights = false;
+    if (window.matchMedia('(prefers-color-scheme: dark)').matches) {
+      turnOffTheLights = true;
+    }
+
+    // Fireworks.
+    const scoresAll100 = Object.values(report.categories).every(cat => cat.score === 1);
+    if (!this._dom.isDevTools() && scoresAll100) {
+      turnOffTheLights = true;
+      const scoresContainer = this._dom.find('.lh-scores-container', this._document);
+      scoresContainer.classList.add('score100');
+      scoresContainer.addEventListener('click', _ => {
+        scoresContainer.classList.toggle('fireworks-paused');
+      });
+    }
+
+    if (turnOffTheLights) {
+      this._toggleDarkTheme(true);
+    }
 
     // There is only a sticky header when at least 2 categories are present.
     if (Object.keys(this.json.categories).length >= 2) {
@@ -295,8 +315,7 @@ class ReportUIFeatures {
    */
   onExportButtonClick(e) {
     e.preventDefault();
-    const el = /** @type {Element} */ (e.target);
-    el.classList.toggle('active');
+    this.exportButton.classList.toggle('active');
     this._document.addEventListener('keydown', this.onKeyDown);
   }
 
@@ -513,9 +532,10 @@ class ReportUIFeatures {
 
   /**
    * @private
+   * @param {boolean} [force]
    */
-  _toggleDarkTheme() {
-    this._document.body.classList.toggle('dark');
+  _toggleDarkTheme(force) {
+    this._document.body.classList.toggle('dark', force);
   }
 
   _updateStickyHeaderOnScroll() {
diff --git a/lighthouse-core/report/html/report-styles.css b/lighthouse-core/report/html/report-styles.css
index 39f353415..0ba4ca75d 100644
--- a/lighthouse-core/report/html/report-styles.css
+++ b/lighthouse-core/report/html/report-styles.css
@@ -63,6 +63,8 @@
   --lh-score-highlight-bg: hsla(0, 0%, 90%, 0.2);
   --lh-score-icon-background-size: 24px;
   --lh-group-icon-background-size: var(--lh-score-icon-background-size);
+  --lh-export-icon-size: var(--lh-score-icon-background-size);
+  --lh-export-icon-color: var(--medium-75-gray);
   --lh-score-margin: 12px;
   --lh-table-header-bg: #f8f9fa;
   --lh-table-higlight-bg: hsla(0, 0%, 75%, 0.1);
@@ -369,9 +371,6 @@
   pointer-events: none;
 }
 
-.report-icon--share {
-  background-image: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path fill="none" d="M0 0h24v24H0z"/><path d="M18 16.08c-.76 0-1.44.3-1.96.77L8.91 12.7c.05-.23.09-.46.09-.7s-.04-.47-.09-.7l7.05-4.11c.54.5 1.25.81 2.04.81 1.66 0 3-1.34 3-3s-1.34-3-3-3-3 1.34-3 3c0 .24.04.47.09.7L8.04 9.81C7.5 9.31 6.79 9 6 9c-1.66 0-3 1.34-3 3s1.34 3 3 3c.79 0 1.5-.31 2.04-.81l7.12 4.16c-.05.21-.08.43-.08.65 0 1.61 1.31 2.92 2.92 2.92 1.61 0 2.92-1.31 2.92-2.92s-1.31-2.92-2.92-2.92z"/></svg>');
-}
 .report-icon--print {
   background-image: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M19 8H5c-1.66 0-3 1.34-3 3v6h4v4h12v-4h4v-6c0-1.66-1.34-3-3-3zm-3 11H8v-5h8v5zm3-7c-.55 0-1-.45-1-1s.45-1 1-1 1 .45 1 1-.45 1-1 1zm-1-9H6v4h12V3z"/><path fill="none" d="M0 0h24v24H0z"/></svg>');
 }
@@ -566,7 +565,7 @@
   align-items: center;
   flex-wrap: wrap;
   justify-content: space-between;
-  padding: 10px var(--text-indent);
+  padding: 10px 0;
 }
 
 .lh-metric__details {
@@ -581,7 +580,6 @@
 .lh-metrics__disclaimer {
   color: var(--medium-75-gray);
   margin: var(--lh-section-vpadding) 0;
-  padding: 0 var(--text-indent);
 }
 
 .lh-metric__description {
@@ -839,6 +837,21 @@
   vertical-align: middle;
 }
 
+/* Style the "over budget" columns red. */
+.lh-audit-group--budgets .lh-table tbody tr td:nth-child(4),
+.lh-audit-group--budgets .lh-table tbody tr td:nth-child(5){
+  color: var(--color-red-700);
+}
+
+/* Align the "over budget request count" text to be close to the "over budget bytes" column. */
+.lh-audit-group--budgets .lh-table tbody tr td:nth-child(4){
+  text-align: right;
+}
+
+.lh-audit-group--budgets .lh-table {
+  width: 100%;
+}
+
 .lh-audit-group--pwa-fast-reliable .lh-audit-group__header::before {
   content: '';
   background-image: var(--pwa-fast-reliable-gray-url);
diff --git a/lighthouse-core/report/html/templates.html b/lighthouse-core/report/html/templates.html
index cc96aee34..e46c5a977 100644
--- a/lighthouse-core/report/html/templates.html
+++ b/lighthouse-core/report/html/templates.html
@@ -143,7 +143,6 @@ limitations under the License.
   </div>
 </template>
 
-
 <!-- Lighthouse perf opportunity header -->
 <template id="tmpl-lh-opportunity-header">
   <div class="lh-load-opportunity__header lh-load-opportunity__cols">
@@ -152,7 +151,6 @@ limitations under the License.
   </div>
 </template>
 
-
 <!-- Lighthouse score container -->
 <template id="tmpl-lh-scores-wrapper">
   <style>
@@ -180,7 +178,9 @@ limitations under the License.
       justify-content: center;
       background-color: var(--color-sticky-header-bg);
       border-bottom: 1px solid var(--color-black-200);
-      z-index: 1000;
+      padding-top: var(--score-container-padding);
+      padding-bottom: 4px;
+      z-index: 1;
       pointer-events: none;
       opacity: 0;
     }
@@ -249,20 +249,20 @@ limitations under the License.
       will-change: transform;
     }
     .lh-export__button {
-      border: 1px solid #dadada;
-      border-radius: 2px;
+      width: var(--lh-export-icon-size);
+      height: var(--lh-export-icon-size);
       cursor: pointer;
-      outline: none;
-      height: 23px;
-      width: 30px;
-      background-repeat: no-repeat;
-      background-size: 18px;
-      background-position: 50% 50%;
-      will-change: transform;
+      margin-right: 5px;
+    }
+    .lh-export__button svg {
+      fill: var(--lh-export-icon-color);
+    }
+    .dark .lh-export__button svg {
+      filter: invert(1);
     }
     .lh-export__button:focus,
     .lh-export__button.active {
-      box-shadow: 1px 1px 3px #ccc;
+      outline: none;
     }
     .lh-export__button.active + .lh-export__dropdown {
       opacity: 1;
@@ -348,7 +348,12 @@ limitations under the License.
     <a href="" class="lh-topbar__url" target="_blank" rel="noopener"></a>
 
     <div class="lh-export">
-      <button class="report-icon report-icon--share lh-export__button" title="Export report"></button>
+      <div class="report-icon report-icon--share lh-export__button" title="Export report">
+        <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"/>
+        </svg>
+      </div>
       <div class="lh-export__dropdown">
         <!-- TODO(i18n): localize export dropdown -->
         <a href="#" class="report-icon report-icon--print" data-action="print-summary">Print Summary</a>
diff --git a/proto/sample_v2_round_trip.json b/proto/sample_v2_round_trip.json
index c0c4c3b91..401a3ff9f 100644
--- a/proto/sample_v2_round_trip.json
+++ b/proto/sample_v2_round_trip.json
@@ -3584,6 +3584,7 @@
                     "weight": 0.0
                 },
                 {
+                    "group": "budgets",
                     "id": "performance-budget",
                     "weight": 0.0
                 },


Test patch:
diff --git a/clients/test/extension/extension-test.js b/clients/test/extension/extension-test.js
index 1517e5c00..63e86e88c 100644
--- a/clients/test/extension/extension-test.js
+++ b/clients/test/extension/extension-test.js
@@ -135,8 +135,10 @@ describe('Lighthouse chrome extension', function() {
     for (const category of lighthouseCategories) {
       let expected = getAuditsOfCategory(category);
       if (category === 'performance') {
-        expected = getAuditsOfCategory(category).filter(a => !!a.group);
+        expected = getAuditsOfCategory(category)
+          .filter(a => !!a.group && a.id !== 'performance-budget');
       }
+      // Performance budget audit is not included in the Chrome extension of Lighthouse
       expected = expected.map(audit => audit.id);
       const elementIds = await getAuditElementsIds({category, selector: selectors.audits});
 
diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
index 13268b7bc..d2aa1fbba 100644
--- a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
+++ b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
@@ -821,6 +821,7 @@ Object {
           "weight": 0,
         },
         Object {
+          "group": "budgets",
           "id": "performance-budget",
           "weight": 0,
         },
diff --git a/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js b/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
index 776763a7c..42c115c40 100644
--- a/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
@@ -68,7 +68,7 @@ describe('PerfCategoryRenderer', () => {
   it('renders the sections', () => {
     const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
     const sections = categoryDOM.querySelectorAll('.lh-category > .lh-audit-group');
-    assert.equal(sections.length, 4);
+    assert.equal(sections.length, 5);
   });
 
   it('renders the metrics', () => {
@@ -151,7 +151,8 @@ describe('PerfCategoryRenderer', () => {
     const passedSection = categoryDOM.querySelector('.lh-category > .lh-clump--passed');
 
     const passedAudits = category.auditRefs.filter(audit =>
-        audit.group && audit.group !== 'metrics' && Util.showAsPassed(audit.result));
+      audit.group && audit.group !== 'metrics' && audit.id !== 'performance-budget'
+        && Util.showAsPassed(audit.result));
     const passedElements = passedSection.querySelectorAll('.lh-audit');
     assert.equal(passedElements.length, passedAudits.length);
   });
@@ -175,6 +176,36 @@ describe('PerfCategoryRenderer', () => {
     });
   });
 
+  describe('budgets', () => {
+    it('renders a performance budget', () => {
+      const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
+
+      const budgetsGroup = categoryDOM.querySelector('.lh-audit-group.lh-audit-group--budgets');
+      assert.ok(budgetsGroup);
+
+      const header = budgetsGroup.querySelector('.lh-audit-group__header');
+      assert.ok(header);
+
+      const budgetTable = budgetsGroup.querySelector('#performance-budget.lh-table');
+      assert.ok(budgetTable);
+
+      const lhrBudgetEntries = sampleResults.audits['performance-budget'].details.items;
+      const tableRows = budgetTable.querySelectorAll('tbody > tr');
+      assert.strictEqual(tableRows.length, lhrBudgetEntries.length);
+    });
+
+    it('does not render a budget table when performance-budget audit is notApplicable', () => {
+      const budgetlessCategory = JSON.parse(JSON.stringify(category));
+      const budgetRef = budgetlessCategory.auditRefs.find(a => a.id === 'performance-budget');
+      budgetRef.result.scoreDisplayMode = 'notApplicable';
+      delete budgetRef.result.details;
+
+      const categoryDOM = renderer.render(budgetlessCategory, sampleResults.categoryGroups);
+      const budgetsGroup = categoryDOM.querySelector('.lh-audit-group.lh-audit-group--budgets');
+      assert.strictEqual(budgetsGroup, null);
+    });
+  });
+
   // This is done all in CSS, but tested here.
   describe('metric description toggles', () => {
     let container;
diff --git a/lighthouse-core/test/report/html/renderer/report-renderer-test.js b/lighthouse-core/test/report/html/renderer/report-renderer-test.js
index d6d8e6a98..9e4836459 100644
--- a/lighthouse-core/test/report/html/renderer/report-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/report-renderer-test.js
@@ -82,8 +82,6 @@ describe('ReportRenderer', () => {
       // 3 sets of gauges - one in sticky header, one in scores header, and one in each section.
       assert.equal(output.querySelectorAll('.lh-gauge__wrapper, .lh-gauge--pwa__wrapper').length,
           sampleResults.reportCategories.length * 3, 'renders category gauges');
-      // no fireworks
-      assert.ok(output.querySelector('.score100') === null, 'has no fireworks treatment');
     });
 
     it('renders additional reports by replacing the existing one', () => {
@@ -214,23 +212,6 @@ describe('ReportRenderer', () => {
     assert.equal(renderer._templateContext, otherDocument);
   });
 
-  it('should render an all 100 report with fireworks', () => {
-    const container = renderer._dom._document.body;
-
-    sampleResults.reportCategories.forEach(element => {
-      element.score = 1;
-    });
-
-    const output = renderer.renderReport(sampleResults, container);
-    // standard checks
-    assert.ok(output.querySelector('.lh-header-sticky'), 'has a header');
-    assert.ok(output.querySelector('.lh-report'), 'has report body');
-    assert.equal(output.querySelectorAll('.lh-gauge__wrapper, .lh-gauge--pwa__wrapper').length,
-        sampleResults.reportCategories.length * 3, 'renders category gauges');
-    // fireworks!
-    assert.ok(output.querySelector('.score100'), 'has fireworks treatment');
-  });
-
   it('should add LHR channel to doc link parameters', () => {
     const lhrChannel = sampleResults.configSettings.channel;
     // Make sure we have a channel in the LHR.
@@ -255,7 +236,8 @@ describe('ReportRenderer', () => {
 
     let notApplicableCount = 0;
     Object.values(clonedSampleResult.audits).forEach(audit => {
-      if (audit.scoreDisplayMode === 'notApplicable') {
+      // The performance-budget audit is omitted from the DOM when it is not applicable
+      if (audit.scoreDisplayMode === 'notApplicable' && audit.id !== 'performance-budget') {
         notApplicableCount++;
         audit.scoreDisplayMode = 'not_applicable';
       }
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 4e83cd0cc..54a3d7852 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
@@ -171,4 +171,22 @@ describe('ReportUIFeatures', () => {
       });
     });
   });
+
+  describe('fireworks', () => {
+    it('should render an non-all 100 report without fireworks', () => {
+      const lhr = JSON.parse(JSON.stringify(sampleResults));
+      lhr.categories.performance.score = 0.5;
+      const container = render(lhr);
+      assert.ok(container.querySelector('.score100') === null, 'has no fireworks treatment');
+    });
+
+    it('should render an all 100 report with fireworks', () => {
+      const lhr = JSON.parse(JSON.stringify(sampleResults));
+      Object.values(lhr.categories).forEach(element => {
+        element.score = 1;
+      });
+      const container = render(lhr);
+      assert.ok(container.querySelector('.score100'), 'has fireworks treatment');
+    });
+  });
 });
diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
index 01cae44b9..7fe39c6e7 100644
--- a/lighthouse-core/test/results/sample_v2.json
+++ b/lighthouse-core/test/results/sample_v2.json
@@ -3456,7 +3456,8 @@
         },
         {
           "id": "performance-budget",
-          "weight": 0
+          "weight": 0,
+          "group": "budgets"
         },
         {
           "id": "resource-summary",