Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new_audit: layout-shifts with estimated root causes #15703

Merged
merged 45 commits into from
Jan 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
b9e5501
core(layout_shifts): new_audit
connorjclark Dec 19, 2023
c030cf2
strings
connorjclark Dec 19, 2023
e0ab6a1
rm comment
connorjclark Dec 19, 2023
de3af7f
fix test-docs
connorjclark Dec 19, 2023
a5c3a8f
make TraceEngineResult separate artifact
connorjclark Dec 19, 2023
bfd0edb
start on smoke test
connorjclark Dec 20, 2023
7c1e444
better comment
connorjclark Dec 20, 2023
ad7ea6a
better comment
connorjclark Dec 20, 2023
4e63154
json
connorjclark Dec 20, 2023
49c35fb
run just what we need
connorjclark Dec 20, 2023
8ed3283
add font change example to smoke test
connorjclark Dec 20, 2023
315f956
make root causes a artifact and trace engine result computed
connorjclark Dec 20, 2023
0c05330
move domain enable/disable
connorjclark Dec 20, 2023
f175483
restructure sub item schema
connorjclark Dec 20, 2023
1fe9c4e
top 15
connorjclark Dec 20, 2023
b7165ec
strings
connorjclark Dec 20, 2023
4d30fff
kayout
connorjclark Dec 20, 2023
e8b5b34
fix failing tests
connorjclark Dec 21, 2023
84c2382
cause being first sucked
connorjclark Dec 21, 2023
0a98d92
comment
connorjclark Dec 21, 2023
4a2b384
update
connorjclark Dec 21, 2023
cb9e425
use opinionated order of causes
connorjclark Dec 21, 2023
243fb31
undo some changes
connorjclark Dec 21, 2023
44562d4
fix api-test-pptr
connorjclark Dec 21, 2023
70fb9b6
Update types/trace-engine.d.ts
connorjclark Dec 21, 2023
e7fe8bd
test no dom disable
connorjclark Dec 21, 2023
002b1ea
fix trace element smoke
connorjclark Dec 21, 2023
f30243d
add unsized media to shift attr smoke
connorjclark Dec 21, 2023
f1accfd
try fix font cause in ci
connorjclark Dec 21, 2023
cfa9e05
order
connorjclark Dec 21, 2023
23997ca
make order not matter
connorjclark Dec 21, 2023
e0639f8
fix cdt e2e tests
connorjclark Dec 21, 2023
9a35241
fix unexpected promise
connorjclark Dec 22, 2023
b77fbdd
handle had recent input bug
connorjclark Jan 2, 2024
2c8a991
pr
connorjclark Jan 2, 2024
b166a93
how did THAT Break things
connorjclark Jan 2, 2024
b74d3a8
i filed a bug
connorjclark Jan 2, 2024
93cb90b
smol
connorjclark Jan 3, 2024
a7512c4
build report
connorjclark Jan 3, 2024
3c93946
pr
connorjclark Jan 3, 2024
271759d
rm
connorjclark Jan 3, 2024
7cc0bdb
text
connorjclark Jan 3, 2024
749fb22
Merge remote-tracking branch 'origin/main' into layout-shifts-new-audits
connorjclark Jan 4, 2024
d84048c
run update after merge
connorjclark Jan 4, 2024
338b8e1
add test
connorjclark Jan 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added cli/test/fixtures/RubikBrokenFax-Regular.ttf
Binary file not shown.
49 changes: 49 additions & 0 deletions cli/test/fixtures/shift-attribution.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<style>
div#blue {
background-color: blue;
height: 300px;
color: white;
width: 100%;
}
div#red {
background-color: red;
height: 10px;
width: 100%;
}

@font-face {
font-family: 'Rubik Broken Fax';
font-style: normal;
font-weight: 400;
font-display: swap;
src: url(RubikBrokenFax-Regular.ttf?delay=2000);
unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02BB-02BC, U+02C6, U+02DA, U+02DC, U+0304, U+0308, U+0329, U+2000-206F, U+2074, U+20AC, U+2122, U+2191, U+2193, U+2212, U+2215, U+FEFF, U+FFFD;
}
h1 {
font-family: 'Rubik Broken Fax', sans-serif;
}
</style>
</head>
<body>
<!-- Do many to stack up the shifts from the font change. -->
<h1>Shift attribution</h1>
<h1>Shift attribution</h1>
<h1>Shift attribution</h1>
<h1>Shift attribution</h1>
<h1>Shift attribution</h1>
<h1>Shift attribution</h1>

<div class="iframe-slot"></div>

<img src="launcher-icon-100x100.png?delay=3000">
<div id="red"></div>
<div id="blue">app goes here</div>
<script src="shift-attribution.js"></script>
</body>
</html>
14 changes: 14 additions & 0 deletions cli/test/fixtures/shift-attribution.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
let i = 0;
const handle = setInterval(function() {
if (i === 0) {
document.querySelector('#red').style.height = 500;
} else if (i === 1) {
const iframeEl = document.createElement('iframe');
iframeEl.src = 'simple-page.html';
document.querySelector('.iframe-slot').append(iframeEl);
} else {
clearInterval(handle);
}

i++;
}, 500);
2 changes: 2 additions & 0 deletions cli/test/smokehouse/core-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import seoPassing from './test-definitions/seo-passing.js';
import seoStatus403 from './test-definitions/seo-status-403.js';
import seoTapTargets from './test-definitions/seo-tap-targets.js';
import serviceWorkerReloaded from './test-definitions/service-worker-reloaded.js';
import shiftAttribution from './test-definitions/shift-attribution.js';
import sourceMaps from './test-definitions/source-maps.js';
import timing from './test-definitions/timing.js';

Expand Down Expand Up @@ -127,6 +128,7 @@ const smokeTests = [
seoStatus403,
seoTapTargets,
serviceWorkerReloaded,
shiftAttribution,
sourceMaps,
timing,
];
Expand Down
22 changes: 20 additions & 2 deletions cli/test/smokehouse/test-definitions/perf-trace-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ const expectations = {
type: 'image',
},
{
traceEventType: 'layout-shift',
traceEventType: 'layout-shift-element',
node: {
selector: 'body > h1',
nodeLabel: 'Please don\'t move me',
Expand All @@ -97,7 +97,7 @@ const expectations = {
},
},
{
traceEventType: 'layout-shift',
traceEventType: 'layout-shift-element',
node: {
nodeLabel: 'Sorry!',
snippet: '<div style="height: 18px;">',
Expand All @@ -111,6 +111,24 @@ const expectations = {
},
},
},
{
traceEventType: 'layout-shift',
node: {
nodeLabel: `Please don't move me`,
},
},
{
traceEventType: 'layout-shift',
node: {
nodeLabel: `Please don't move me`,
},
},
{
traceEventType: 'layout-shift',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
node: {
nodeLabel: 'section > img',
},
},
{
traceEventType: 'animation',
node: {
Expand Down
44 changes: 44 additions & 0 deletions cli/test/smokehouse/test-definitions/shift-attribution.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* @license
* Copyright 2023 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

/**
* @type {Smokehouse.ExpectedRunnerResult}
*/
const expectations = {
lhr: {
requestedUrl: 'http://localhost:10200/shift-attribution.html',
finalDisplayedUrl: 'http://localhost:10200/shift-attribution.html',
audits: {
'layout-shifts': {
details: {
items: {
// Order (aka shift size) may vary due to environment.
_includes: [
{
node: {selector: 'body > div#blue'},
subItems: {items: [{cause: /font/, extra: {value: /Regular\.ttf/}}]},
},
{
node: {selector: 'body > div#blue'},
// TODO: We can't get nodes from non-main frames yet. See runRootCauseAnalysis.
subItems: {items: [{cause: /iframe/, extra: undefined}]},
},
{
node: {selector: 'body > div#blue'},
subItems: {items: [{cause: /Media/, extra: {selector: 'body > img'}}]},
},
],
},
},
},
},
},
};

export default {
id: 'shift-attribution',
expectations,
};
2 changes: 1 addition & 1 deletion core/audits/layout-shift-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class LayoutShiftElements extends Audit {

/** @type {Array<{node: LH.Audit.Details.ItemValue, score: number}>} */
const clsElementData = artifacts.TraceElements
.filter(element => element.traceEventType === 'layout-shift')
.filter(element => element.traceEventType === 'layout-shift-element')
.map(element => ({
node: Audit.makeNodeItem(element.node),
score: impactByNodeId.get(element.nodeId) || 0,
Expand Down
158 changes: 158 additions & 0 deletions core/audits/layout-shifts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/**
* @license Copyright 2023 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import {Audit} from './audit.js';
import * as i18n from '../lib/i18n/i18n.js';
import {CumulativeLayoutShift as CumulativeLayoutShiftComputed} from '../computed/metrics/cumulative-layout-shift.js';
import CumulativeLayoutShift from './metrics/cumulative-layout-shift.js';
import TraceElements from '../gather/gatherers/trace-elements.js';
import {TraceEngineResult} from '../computed/trace-engine-result.js';

const MAX_LAYOUT_SHIFTS = 15;

/** @typedef {LH.Audit.Details.TableItem & {node?: LH.Audit.Details.NodeValue, score: number, subItems?: {type: 'subitems', items: SubItem[]}}} Item */
/** @typedef {{extra?: LH.Audit.Details.NodeValue | LH.Audit.Details.UrlValue, cause: LH.IcuMessage}} SubItem */

/* eslint-disable max-len */
const UIStrings = {
/** Descriptive title of a diagnostic audit that provides the top elements affected by layout shifts. */
title: 'Avoid large layout shifts',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same title as layout-shift-elements. I understand why since the call to action is the same, but this makes me wonder if we should somehow combine the audits (e.g. multiple tables in the same audit)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the linked doc, we left the fate of this other audit unresolved: We should make it hidden or remove it or make it informative….

wdyt of marking it informative?

can also change title to Avoid large layout shifts (impacted elements)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm leaning towards the hide/remove option at this point. This new audit seems generally better than the old one and less of an information dump. That being said, if we still see value in the element summary my preferences go as follows:

  1. Hide/remove the old audit
  2. Add a second table to the new layout-shifts audit that takes the elements from the old audit
  3. Make the old audit informative

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to ditching the old one if possible (hidden for now to not break anyone makes sense). Is it possible to make sure all the information in the old audit is in the new audit (either improved or at least as-is)? e.g. for shifts without a known root cause

Copy link
Collaborator Author

@connorjclark connorjclark Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's hide it.

Is it possible to make sure all the information in the old audit is in the new audit (either improved or at least as-is)?

This audit won't hide a layout shift if it has no determined root cause. root cause is just an enhancement.

For the top 15 layout shifts, we'll show the the largest impacted element that moves in each shift, whereas the previous audit would show the top impacted elements despite being from the same shift or not. So it isn't 1:1, but we'll be showing all the biggest impacted nodes still. Should be higher useful info : noise ratio

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry, I commented without reading through the code or trying it out :) Sounds great to me!

/** Description of a diagnostic audit that provides the top elements affected by layout shifts. "windowing" means the metric value is calculated using the subset of events in a small window of time during the run. "normalization" is a good substitute for "windowing". The last sentence starting with 'Learn' becomes link text to additional documentation. */
description: 'These are the largest layout shifts observed on the page. Each table item represents a single layout shift, and shows the element that shifted the most. Below each item are possible root causes that led to the layout shift. Some of these layout shifts may not be included in the CLS metric value due to [windowing](https://web.dev/articles/cls#what_is_cls). [Learn how to improve CLS](https://web.dev/articles/optimize-cls)',
/** Label for a column in a data table; entries in this column will be a number representing how large the layout shift was. */
columnScore: 'Layout shift score',
/** A possible reason why that the layout shift occured. */
rootCauseUnsizedMedia: 'Media element lacking an explicit size',
/** A possible reason why that the layout shift occured. */
rootCauseFontChanges: 'Web font loaded',
/** A possible reason why that the layout shift occured. */
rootCauseInjectedIframe: 'Injected iframe',
/** A possible reason why that the layout shift occured. */
rootCauseRenderBlockingRequest: 'A late network request adjusted the page layout',
/** Label shown per-audit to show how many layout shifts are present. The `{# shifts found}` placeholder will be replaced with the number of layout shifts. */
displayValueShiftsFound: `{shiftCount, plural, =1 {1 layout shift found} other {# layout shifts found}}`,
};
/* eslint-enable max-len */

const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings);

class LayoutShifts extends Audit {
/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'layout-shifts',
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: Audit.SCORING_MODES.METRIC_SAVINGS,
guidanceLevel: 2,
requiredArtifacts: ['traces', 'RootCauses', 'TraceElements'],
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
const trace = artifacts.traces[Audit.DEFAULT_PASS];
const traceEngineResult = await TraceEngineResult.request({trace}, context);
const clusters = traceEngineResult.LayoutShifts.clusters ?? [];
const {cumulativeLayoutShift: clsSavings, impactByNodeId} =
await CumulativeLayoutShiftComputed.request(trace, context);
const traceElements = artifacts.TraceElements
.filter(element => element.traceEventType === 'layout-shift');

/** @type {Item[]} */
const items = [];
const layoutShiftEvents = clusters.flatMap(c => c.events);
const topLayoutShiftEvents = layoutShiftEvents
.sort((a, b) => b.args.data.weighted_score_delta - a.args.data.weighted_score_delta)
.slice(0, MAX_LAYOUT_SHIFTS);
for (const event of topLayoutShiftEvents) {
const biggestImpactNodeId = TraceElements.getBiggestImpactNodeForShiftEvent(
event.args.data.impacted_nodes, impactByNodeId);
const biggestImpactElement = traceElements.find(t => t.nodeId === biggestImpactNodeId);

// Turn root causes into sub-items.
const index = layoutShiftEvents.indexOf(event);
const rootCauses = artifacts.RootCauses.layoutShifts[index];
/** @type {SubItem[]} */
const subItems = [];
if (rootCauses) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
for (const cause of rootCauses.unsizedMedia) {
const element = artifacts.TraceElements.find(
t => t.traceEventType === 'layout-shift' && t.nodeId === cause.node.backendNodeId);
subItems.push({
extra: element ? Audit.makeNodeItem(element.node) : undefined,
cause: str_(UIStrings.rootCauseUnsizedMedia),
});
}
for (const cause of rootCauses.fontChanges) {
const url = cause.request.args.data.url;
subItems.push({
extra: {type: 'url', value: url},
cause: str_(UIStrings.rootCauseFontChanges),
});
}
for (const cause of rootCauses.iframes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

andres commented that "I remember this one getting noisy at some point on apps with many ads". Something for us to keep in mind. Based on the impl, I don't think 0x0 iframes will be counted, but 1x1 are.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a behavioral difference between 1x1 and 0x0 iframes, or is that purely a developer mistake resulting in (the most minor) shift?

const element = artifacts.TraceElements.find(
t => t.traceEventType === 'layout-shift' && t.nodeId === cause.iframe.backendNodeId);
subItems.push({
extra: element ? Audit.makeNodeItem(element.node) : undefined,
cause: str_(UIStrings.rootCauseInjectedIframe),
});
}
for (const cause of rootCauses.renderBlockingRequests) {
const url = cause.request.args.data.url;
subItems.push({
extra: {type: 'url', value: url},
cause: str_(UIStrings.rootCauseRenderBlockingRequest),
});
}
}

items.push({
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
node: biggestImpactElement ? Audit.makeNodeItem(biggestImpactElement.node) : undefined,
score: event.args.data.weighted_score_delta,
subItems: subItems.length ? {type: 'subitems', items: subItems} : undefined,
});
}

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
/* eslint-disable max-len */
{key: 'node', valueType: 'node', subItemsHeading: {key: 'extra'}, label: str_(i18n.UIStrings.columnElement)},
{key: 'score', valueType: 'numeric', subItemsHeading: {key: 'cause', valueType: 'text'}, granularity: 0.001, label: str_(UIStrings.columnScore)},
/* eslint-enable max-len */
];

const details = Audit.makeTableDetails(headings, items);

let displayValue;
if (items.length > 0) {
displayValue = str_(UIStrings.displayValueShiftsFound,
{shiftCount: items.length});
}

const passed = clsSavings <= CumulativeLayoutShift.defaultOptions.p10;

return {
score: passed ? 1 : 0,
scoreDisplayMode: passed ? Audit.SCORING_MODES.INFORMATIVE : undefined,
metricSavings: {
CLS: clsSavings,
},
notApplicable: details.items.length === 0,
displayValue,
details,
};
}
}

export default LayoutShifts;
export {UIStrings};
Loading
Loading