core(layout-shift-elements): surface CLS contribution per shifted element #10968

Beytoven · 2020-06-16T01:13:57Z

The layout-shift-elements audit surfaces up to the top 5 elements that shifted during page load and contributed the most to the CLS metric. With this change, it'll also show the amount each of these elements contribute to the CLS score.

Addresses #10751

googlebot · 2020-06-16T01:14:03Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Beytoven · 2020-06-16T01:15:39Z

lighthouse-core/gather/gatherers/trace-elements.js

-    const clsPerNodeMap = new Map();
-    /** @type {Set<number>} */
-    const clsNodeIds = new Set();
+  static getLayoutShiftElements(mainThreadEvents) {


Gonna add comments throughout this function as it has grown pretty complicated.

Agreed. A high-level comment here explaining the approach would be useful. Explaining that score is the additional CLS since the last event, and that we want to distribute that incremental score between all the nodes that have shifted, proportional to the impact region of each shift.

Beytoven · 2020-06-16T01:17:03Z

types/artifacts.d.ts

@@ -483,6 +483,7 @@ declare global {
        nodeLabel?: string;
        devtoolsNodePath: string;
        snippet?: string;
+        score?: number;


Open to suggestions on renaming this. Don't know if there's a better naming option here so left it as score for now.

yeah this seems fine to me. score will be specific to the type of element, so metricName becomes a discriminant. future metrics may overload score but it'll be orthogonal so it seems fine to just use score and not cls_score. also seems fine to keep the types simple and not do a real type discriminant (like interface CLSTraceElement extends TraceElement { score: number}.

Beytoven · 2020-06-16T01:17:56Z

lighthouse-cli/test/smokehouse/test-definitions/perf/expectations.js

@@ -176,15 +176,15 @@ module.exports = [
          },
        },
        // TODO: uncomment when Chrome m84 lands


Will delete this TODO once I verify the smoke test can pass in the CI runs.

#10976 will take care of this.

can you rm this change

connorjclark

Any idea why the now deployment doesn't have any values for CLS contribution?

connorjclark · 2020-06-16T01:21:52Z

lighthouse-core/gather/gatherers/trace-elements.js

@@ -16,6 +16,8 @@ const pageFunctions = require('../../lib/page-functions.js');
 const TraceProcessor = require('../../lib/tracehouse/trace-processor.js');
 const RectHelpers = require('../../lib/rect-helpers.js');

+/** @typedef {{nodeId: Number, score?: Number}} TraceElementData */


number. see https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#general-types

connorjclark · 2020-06-16T01:25:14Z

lighthouse-core/gather/gatherers/trace-elements.js

+        totalAreaOfImpact += areaOfImpact;
+      });
+
+      [...pixelsMovedPerNode.entries()].forEach(entry => {


for (const [key, pixelsMoved] of pixelsMovedPerNode.entries()) { ...

for-loops are better than .forEach, and destructuring key-value entry into variables is nice for readability

lighthouse-core/gather/gatherers/trace-elements.js

connorjclark · 2020-06-16T01:26:10Z

lighthouse-core/gather/gatherers/trace-elements.js

-    .sort((a, b) => b[1] - a[1])
-    .slice(0, 5).map(entry => Number(entry[0]));
+    const topFive = [...clsPerNode.entries()]
+    .sort((a, b) => b[1] - a[1]).slice(0, 5)


Suggested change

.sort((a, b) => b[1] - a[1]).slice(0, 5)

.sort((a, b) => b[1] - a[1])

.slice(0, 5)

nit

connorjclark · 2020-06-16T01:26:45Z

lighthouse-core/gather/gatherers/trace-elements.js

+    .map(entry => {
+      return {
+        nodeId: Number(entry[0]),
+        score: Number(entry[1]),


I feel like at least one of these are numbers already

connorjclark · 2020-06-16T01:30:33Z

types/artifacts.d.ts

@@ -483,6 +483,7 @@ declare global {
        nodeLabel?: string;
        devtoolsNodePath: string;
        snippet?: string;
+        score?: number;


yeah this seems fine to me. score will be specific to the type of element, so metricName becomes a discriminant. future metrics may overload score but it'll be orthogonal so it seems fine to just use score and not cls_score. also seems fine to keep the types simple and not do a real type discriminant (like interface CLSTraceElement extends TraceElement { score: number}.

lighthouse-core/gather/gatherers/trace-elements.js

connorjclark · 2020-06-16T01:36:24Z

lighthouse-core/gather/gatherers/trace-elements.js

-    const clsPerNodeMap = new Map();
-    /** @type {Set<number>} */
-    const clsNodeIds = new Set();
+  static getLayoutShiftElements(mainThreadEvents) {


Agreed. A high-level comment here explaining the approach would be useful. Explaining that score is the additional CLS since the last event, and that we want to distribute that incremental score between all the nodes that have shifted, proportional to the impact region of each shift.

lighthouse-core/gather/gatherers/trace-elements.js

connorjclark · 2020-06-19T18:37:16Z

lighthouse-core/gather/gatherers/trace-elements.js

+
+      for (const [nodeId, pixelsMoved] of pixelsMovedPerNode.entries()) {
+        let clsContribution = clsPerNode.get(nodeId) || 0;
+        clsContribution += (pixelsMoved / totalAreaOfImpact) * Number(event.score);


is event.score already a number?

bump on this

lighthouse-core/test/gather/gatherers/trace-elements-test.js

connorjclark

Just one small thing left from me (the usage of Number(event.score)

connorjclark · 2020-06-23T21:23:51Z

As a potential follow up: maybe we should instead show the percentage (or add a third column for it)? Since CLS itself is a fairly nebulous unit, it'd be nice to just weigh these contributions in terms of %.

googlebot · 2020-06-23T22:49:14Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

…ment (GoogleChrome#10968)

Beytoven and others added 3 commits June 8, 2020 13:29

Attempting to get shifterinos per element

bcaf961

log

fcd339c

Surface CLS contribution per element

d8aaba1

Beytoven requested a review from connorjclark June 16, 2020 01:13

Beytoven requested a review from a team as a code owner June 16, 2020 01:13

googlebot added the cla: no label Jun 16, 2020

Beytoven commented Jun 16, 2020

View reviewed changes

devtools-bot assigned connorjclark Jun 16, 2020

devtools-bot added the waiting4reviewer label Jun 16, 2020

Beytoven commented Jun 16, 2020

View reviewed changes

connorjclark requested changes Jun 16, 2020

View reviewed changes

connorjclark changed the title ~~core(layout-shift-elements): capture and surface CLS contribution per top shifted elements~~ core(layout-shift-elements): surface CLS contribution per shifted element Jun 16, 2020

connorjclark added waiting4committer and removed waiting4reviewer labels Jun 16, 2020

Refactoring + comments

360a926

vercel bot deployed to Preview June 16, 2020 23:17 View deployment

Beytoven added 2 commits June 16, 2020 16:21

Merge branch 'master' into cls-element-values

c7d77d2

Fix after merge

f6bb4e5

vercel bot deployed to Preview June 17, 2020 01:11 View deployment

Beytoven added waiting4reviewer and removed waiting4committer labels Jun 17, 2020

connorjclark reviewed Jun 17, 2020

View reviewed changes

lighthouse-core/gather/gatherers/trace-elements.js Show resolved Hide resolved

connorjclark reviewed Jun 19, 2020

View reviewed changes

connorjclark added waiting4committer and removed waiting4reviewer labels Jun 19, 2020

Add unit tests for trace-elements gatherer

b1079ca

vercel bot deployed to Preview June 22, 2020 17:52 View deployment

connorjclark reviewed Jun 22, 2020

View reviewed changes

lighthouse-core/test/gather/gatherers/trace-elements-test.js Show resolved Hide resolved

connorjclark added the 6.1 label Jun 22, 2020

Verify totals in unit tests

c7c776a

vercel bot deployed to Preview June 23, 2020 20:09 View deployment

connorjclark approved these changes Jun 23, 2020

View reviewed changes

Remove number casting

35a7837

vercel bot deployed to Preview June 23, 2020 22:25 View deployment

Re-comment out smoke test

beac3d7

vercel bot deployed to Preview June 23, 2020 22:41 View deployment

connorjclark added cla: yes and removed cla: no labels Jun 23, 2020

connorjclark approved these changes Jun 23, 2020

View reviewed changes

Beytoven merged commit ead5da4 into master Jun 24, 2020

Beytoven deleted the cls-element-values branch June 24, 2020 00:06

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Jul 6, 2020

core(layout-shift-elements): surface CLS contribution per shifted ele…

b6fa705

…ment (GoogleChrome#10968)

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Aug 28, 2020

core(layout-shift-elements): surface CLS contribution per shifted ele…

ffc57b1

…ment (GoogleChrome#10968)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(layout-shift-elements): surface CLS contribution per shifted element #10968

core(layout-shift-elements): surface CLS contribution per shifted element #10968

Beytoven commented Jun 16, 2020

googlebot commented Jun 16, 2020

Beytoven Jun 16, 2020

connorjclark Jun 16, 2020

Beytoven Jun 16, 2020

connorjclark Jun 16, 2020

Beytoven Jun 16, 2020

connorjclark Jun 17, 2020

connorjclark Jun 19, 2020

connorjclark left a comment

connorjclark Jun 16, 2020

connorjclark Jun 16, 2020

connorjclark Jun 16, 2020

connorjclark Jun 16, 2020

connorjclark Jun 16, 2020

connorjclark Jun 16, 2020

connorjclark Jun 19, 2020

connorjclark Jun 23, 2020

connorjclark left a comment

connorjclark commented Jun 23, 2020

googlebot commented Jun 23, 2020

	.sort((a, b) => b[1] - a[1]).slice(0, 5)
	.sort((a, b) => b[1] - a[1])
	.slice(0, 5)

core(layout-shift-elements): surface CLS contribution per shifted element #10968

core(layout-shift-elements): surface CLS contribution per shifted element #10968

Conversation

Beytoven commented Jun 16, 2020

googlebot commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark left a comment

Choose a reason for hiding this comment

connorjclark commented Jun 23, 2020

googlebot commented Jun 23, 2020