Skip to content

Commit ccefdac

Browse files
Adam RaineDevtools-frontend LUCI CQ
Adam Raine
authored and
Devtools-frontend LUCI CQ
committed
[RPP] Add passed insights section
https://screenshot.googleplex.com/6AtHW85oLmmiikh - Rephrased viewport audit title and description to make sense when shown in the passed insights section. - Several insights were not hiding empty sections because of limitations with the `:empty` CSS selector. These sections are now hidden via JS. Bug: 369766999 Change-Id: I7c922e8337bf9eb1d2eec12fa2d6ddc25f626b34 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6052253 Commit-Queue: Adam Raine <[email protected]> Reviewed-by: Connor Clark <[email protected]>
1 parent d0ba98e commit ccefdac

File tree

7 files changed

+144
-40
lines changed

7 files changed

+144
-40
lines changed

‎front_end/models/trace/insights/Viewport.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,12 @@ import {
1616

1717
const UIStrings = {
1818
/** Title of an insight that provides details about if the page's viewport is optimized for mobile viewing. */
19-
title: 'Viewport not optimized for mobile',
19+
title: 'Optimize viewport for mobile',
2020
/**
2121
* @description Text to tell the user how a viewport meta element can improve performance. \xa0 is a non-breaking space
2222
*/
2323
description:
24-
'The page\'s viewport is not mobile-optimized, so tap interactions may be [delayed by up to 300\xA0ms](https://developer.chrome.com/blog/300ms-tap-delay-gone-away/).',
24+
'Tap interactions may be [delayed by up to 300\xA0ms](https://developer.chrome.com/blog/300ms-tap-delay-gone-away/) if the viewport is not optimized for mobile.',
2525
};
2626

2727
const str_ = i18n.i18n.registerUIStrings('models/trace/insights/Viewport.ts', UIStrings);

‎front_end/panels/timeline/components/SidebarSingleInsightSet.test.ts‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,13 @@ function getUserVisibleInsights(component: Components.SidebarSingleInsightSet.Si
2323
return [...component.shadowRoot.querySelectorAll<BaseInsightComponent>('[data-insight-name]')];
2424
}
2525

26+
function getPassedInsights(component: Components.SidebarSingleInsightSet.SidebarSingleInsightSet):
27+
BaseInsightComponent[] {
28+
assert.isOk(component.shadowRoot);
29+
return [...component.shadowRoot.querySelectorAll<BaseInsightComponent>(
30+
'.passed-insights-section [data-insight-name]')];
31+
}
32+
2633
describeWithEnvironment('SidebarSingleInsightSet', () => {
2734
it('renders a list of insights', async function() {
2835
const {parsedTrace, insights} = await TraceLoader.traceEngine(this, 'web-dev-with-commit.json.gz');
@@ -54,6 +61,18 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
5461
'Render blocking requests',
5562
'Document request latency',
5663
'Third parties',
64+
'Improve image delivery',
65+
'Optimize viewport for mobile',
66+
'CSS Selector costs',
67+
]);
68+
69+
const passedInsightTitles = getPassedInsights(component).flatMap(component => {
70+
return getCleanTextContentFromElements(component.shadowRoot!, '.insight-title');
71+
});
72+
assert.deepEqual(passedInsightTitles, [
73+
'Improve image delivery',
74+
'Optimize viewport for mobile',
75+
'CSS Selector costs',
5776
]);
5877
});
5978

@@ -81,6 +100,19 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
81100
'Layout shift culprits',
82101
'Improve image delivery',
83102
'Third parties',
103+
'Document request latency',
104+
'Optimize viewport for mobile',
105+
'CSS Selector costs',
106+
]);
107+
108+
const passedInsightTitles = getPassedInsights(component).flatMap(component => {
109+
return getCleanTextContentFromElements(component.shadowRoot!, '.insight-title');
110+
});
111+
// Does not include "font display", which is experimental.
112+
assert.deepEqual(passedInsightTitles, [
113+
'Document request latency',
114+
'Optimize viewport for mobile',
115+
'CSS Selector costs',
84116
]);
85117
});
86118

@@ -104,14 +136,26 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
104136
const userVisibleTitles = getUserVisibleInsights(component).flatMap(component => {
105137
return getCleanTextContentFromElements(component.shadowRoot!, '.insight-title');
106138
});
107-
// Does not include "font display", which is experimental.
139+
// Includes "font display", which is experimental.
108140
assert.deepEqual(userVisibleTitles, [
109141
'LCP by phase',
110142
'LCP request discovery',
111143
'Layout shift culprits',
112144
'Improve image delivery',
113145
'Font display',
114146
'Third parties',
147+
'Document request latency',
148+
'Optimize viewport for mobile',
149+
'CSS Selector costs',
150+
]);
151+
152+
const passedInsightTitles = getPassedInsights(component).flatMap(component => {
153+
return getCleanTextContentFromElements(component.shadowRoot!, '.insight-title');
154+
});
155+
assert.deepEqual(passedInsightTitles, [
156+
'Document request latency',
157+
'Optimize viewport for mobile',
158+
'CSS Selector costs',
115159
]);
116160
});
117161

‎front_end/panels/timeline/components/SidebarSingleInsightSet.ts‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ const UIStrings = {
2424
*@example {poor} PH3
2525
*/
2626
metricScore: '{PH1}: {PH2} {PH3} score',
27+
/**
28+
* @description Summary text for an expandable dropdown that contains all insights in a passing state.
29+
* @example {4} PH1
30+
*/
31+
passedInsights: 'Passed insights ({PH1})',
2732
};
2833
const str_ = i18n.i18n.registerUIStrings('panels/timeline/components/SidebarSingleInsightSet.ts', UIStrings);
2934
const i18nString = i18n.i18n.getLocalizedString.bind(undefined, str_);
@@ -209,14 +214,15 @@ export class SidebarSingleInsightSet extends HTMLElement {
209214
return LitHtml.nothing;
210215
}
211216

212-
const components: LitHtml.TemplateResult[] = [];
217+
const shownInsights: LitHtml.TemplateResult[] = [];
218+
const passedInsights: LitHtml.TemplateResult[] = [];
213219
for (const [name, componentClass] of Object.entries(INSIGHT_NAME_TO_COMPONENT)) {
214220
if (!includeExperimental && EXPERIMENTAL_INSIGHTS.has(name)) {
215221
continue;
216222
}
217223

218224
const model = models[name as keyof typeof models];
219-
if (!model || !model.shouldShow ||
225+
if (!model ||
220226
!shouldRenderForCategory({activeCategory: this.#data.activeCategory, insightCategory: model.category})) {
221227
continue;
222228
}
@@ -232,10 +238,26 @@ export class SidebarSingleInsightSet extends HTMLElement {
232238
</div>`;
233239
// clang-format on
234240

235-
components.push(component);
241+
if (model.shouldShow) {
242+
shownInsights.push(component);
243+
} else {
244+
passedInsights.push(component);
245+
}
236246
}
237247

238-
return html`${components}`;
248+
// clang-format off
249+
return html`
250+
${shownInsights}
251+
${passedInsights.length ? html`
252+
<details class="passed-insights-section">
253+
<summary>${i18nString(UIStrings.passedInsights, {
254+
PH1: passedInsights.length,
255+
})}</summary>
256+
${passedInsights}
257+
</details>
258+
` : LitHtml.nothing}
259+
`;
260+
// clang-format on
239261
}
240262

241263
#render(): void {

‎front_end/panels/timeline/components/insights/RenderBlocking.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,14 @@ export class RenderBlocking extends BaseInsightComponent<RenderBlockingInsightMo
6262
const MAX_REQUESTS = 3;
6363
const topRequests = this.model.renderBlockingRequests.slice(0, MAX_REQUESTS);
6464

65+
if (!topRequests.length) {
66+
return LitHtml.nothing;
67+
}
68+
6569
// clang-format off
6670
return html`
6771
<div class="insight-section">
68-
${html`<devtools-performance-table
72+
<devtools-performance-table
6973
.data=${{
7074
insight: this,
7175
headers: [i18nString(UIStrings.renderBlockingRequest), i18nString(UIStrings.duration)],
@@ -77,8 +81,9 @@ export class RenderBlocking extends BaseInsightComponent<RenderBlockingInsightMo
7781
overlays: [this.#createOverlayForRequest(request)],
7882
})),
7983
}}>
80-
</devtools-performance-table>`}
81-
</div>`;
84+
</devtools-performance-table>
85+
</div>
86+
`;
8287
// clang-format on
8388
}
8489

‎front_end/panels/timeline/components/insights/SlowCSSSelector.ts‎

Lines changed: 36 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -127,23 +127,28 @@ export class SlowCSSSelector extends BaseInsightComponent<SlowCSSSelectorInsight
127127
i18n.TimeUtilities.millisToString(Platform.Timing.microSecondsToMilliSeconds(us));
128128

129129
// clang-format off
130-
return html`
131-
<div>
132-
<div class="insight-section">
133-
${html`<devtools-performance-table
134-
.data=${{
135-
insight: this,
136-
headers: [i18nString(UIStrings.total), ''],
137-
rows: [
138-
{values: [i18nString(UIStrings.elapsed), i18n.TimeUtilities.millisToString(this.model.totalElapsedMs)]},
139-
{values: [i18nString(UIStrings.matchAttempts), this.model.totalMatchAttempts]},
140-
{values: [i18nString(UIStrings.matchCount), this.model.totalMatchCount]},
141-
],
142-
} as TableData}>
143-
</devtools-performance-table>`}
144-
</div>
130+
const sections = [html`
131+
<div class="insight-section">
132+
<devtools-performance-table
133+
.data=${{
134+
insight: this,
135+
headers: [i18nString(UIStrings.total), ''],
136+
rows: [
137+
{values: [i18nString(UIStrings.elapsed), i18n.TimeUtilities.millisToString(this.model.totalElapsedMs)]},
138+
{values: [i18nString(UIStrings.matchAttempts), this.model.totalMatchAttempts]},
139+
{values: [i18nString(UIStrings.matchCount), this.model.totalMatchCount]},
140+
],
141+
} as TableData}>
142+
</devtools-performance-table>
143+
</div>
144+
`];
145+
// clang-format on
146+
147+
if (this.model.topElapsedMs.length) {
148+
// clang-format off
149+
sections.push(html`
145150
<div class="insight-section">
146-
${html`<devtools-performance-table
151+
<devtools-performance-table
147152
.data=${{
148153
insight: this,
149154
headers: [i18nString(UIStrings.topSelectors), i18nString(UIStrings.elapsed)],
@@ -155,10 +160,17 @@ export class SlowCSSSelector extends BaseInsightComponent<SlowCSSSelectorInsight
155160
};
156161
}),
157162
} as TableData}>
158-
</devtools-performance-table>`}
163+
</devtools-performance-table>
159164
</div>
165+
`);
166+
// clang-format on
167+
}
168+
169+
if (this.model.topMatchAttempts.length) {
170+
// clang-format off
171+
sections.push(html`
160172
<div class="insight-section">
161-
${html`<devtools-performance-table
173+
<devtools-performance-table
162174
.data=${{
163175
insight: this,
164176
headers: [i18nString(UIStrings.topSelectors), i18nString(UIStrings.matchAttempts)],
@@ -170,10 +182,13 @@ export class SlowCSSSelector extends BaseInsightComponent<SlowCSSSelectorInsight
170182
};
171183
}),
172184
} as TableData}>
173-
</devtools-performance-table>`}
185+
</devtools-performance-table>
174186
</div>
175-
</div>`;
176-
// clang-format on
187+
`);
188+
// clang-format on
189+
}
190+
191+
return html`${sections}`;
177192
}
178193

179194
override render(): void {

‎front_end/panels/timeline/components/insights/ThirdParties.ts‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -76,11 +76,12 @@ export class ThirdParties extends BaseInsightComponent<ThirdPartiesInsightModel>
7676
const topTransferSizeEntries = entries.sort((a, b) => b[1].transferSize - a[1].transferSize).slice(0, 6);
7777
const topMainThreadTimeEntries = entries.sort((a, b) => b[1].mainThreadTime - a[1].mainThreadTime).slice(0, 6);
7878

79-
// clang-format off
80-
return html`
81-
<div>
79+
const sections = [];
80+
if (topTransferSizeEntries.length) {
81+
// clang-format off
82+
sections.push(html`
8283
<div class="insight-section">
83-
${html`<devtools-performance-table
84+
<devtools-performance-table
8485
.data=${{
8586
insight: this,
8687
headers: [i18nString(UIStrings.columnThirdParty), i18nString(UIStrings.columnTransferSize)],
@@ -92,11 +93,17 @@ export class ThirdParties extends BaseInsightComponent<ThirdPartiesInsightModel>
9293
overlays: this.#overlaysForEntity.get(entity),
9394
})),
9495
}}>
95-
</devtools-performance-table>`}
96+
</devtools-performance-table>
9697
</div>
98+
`);
99+
// clang-format on
100+
}
97101

102+
if (topMainThreadTimeEntries.length) {
103+
// clang-format off
104+
sections.push(html`
98105
<div class="insight-section">
99-
${html`<devtools-performance-table
106+
<devtools-performance-table
100107
.data=${{
101108
insight: this,
102109
headers: [i18nString(UIStrings.columnThirdParty), i18nString(UIStrings.columnBlockingTime)],
@@ -108,10 +115,13 @@ export class ThirdParties extends BaseInsightComponent<ThirdPartiesInsightModel>
108115
overlays: this.#overlaysForEntity.get(entity),
109116
})),
110117
}}>
111-
</devtools-performance-table>`}
118+
</devtools-performance-table>
112119
</div>
113-
</div>`;
114-
// clang-format on
120+
`);
121+
// clang-format on
122+
}
123+
124+
return html`${sections}`;
115125
}
116126

117127
override render(): void {

‎front_end/panels/timeline/components/sidebarSingleInsightSet.css‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,11 @@
5959
padding: 0 1px;
6060
}
6161
}
62+
63+
.passed-insights-section {
64+
margin-top: var(--sys-size-5);
65+
66+
summary {
67+
font-weight: var(--ref-typeface-weight-medium);
68+
}
69+
}

0 commit comments

Comments
 (0)