Skip to content

Commit

Permalink
core(trace-elements): remove element score field (#15677)
Browse files Browse the repository at this point in the history
  • Loading branch information
@adamraine
adamraine authored Dec 8, 2023
1 parent 5b395a3 commit a60c73d
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 75 deletions.
38 changes: 33 additions & 5 deletions cli/test/smokehouse/test-definitions/perf-trace-elements.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ const expectations = {
height: 37,
},
},
score: '0.035 +/- 0.01',
},
{
traceEventType: 'layout-shift',
Expand All @@ -111,7 +110,6 @@ const expectations = {
height: 18,
},
},
score: '0.017 +/- 0.01',
},
{
traceEventType: 'animation',
Expand Down Expand Up @@ -176,9 +174,39 @@ const expectations = {
score: null,
displayValue: '2 elements found',
details: {
items: {
length: 2,
},
items: [
{
node: {
selector: 'body > h1',
nodeLabel: 'Please don\'t move me',
snippet: '<h1>',
boundingRect: {
top: 465,
bottom: 502,
left: 8,
right: 404,
width: 396,
height: 37,
},
},
score: '0.035 +/- 0.01',
},
{
node: {
nodeLabel: 'Sorry!',
snippet: '<div style="height: 18px;">',
boundingRect: {
top: 426,
bottom: 444,
left: 8,
right: 404,
width: 396,
height: 18,
},
},
score: '0.017 +/- 0.01',
},
],
},
},
'long-tasks': {
Expand Down
4 changes: 2 additions & 2 deletions core/audits/layout-shift-elements.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ class LayoutShiftElements extends Audit {
const {cumulativeLayoutShift: clsSavings, impactByNodeId} =
await CumulativeLayoutShiftComputed.request(artifacts.traces[Audit.DEFAULT_PASS], context);

/** @type {Array<{node: LH.Audit.Details.ItemValue, score?: number}>} */
/** @type {Array<{node: LH.Audit.Details.ItemValue, score: number}>} */
const clsElementData = artifacts.TraceElements
.filter(element => element.traceEventType === 'layout-shift')
.map(element => ({
node: Audit.makeNodeItem(element.node),
score: element.score,
score: impactByNodeId.get(element.nodeId) || 0,
}));

if (clsElementData.length < impactByNodeId.size) {
Expand Down
10 changes: 2 additions & 8 deletions core/gather/gatherers/trace-elements.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {Responsiveness} from '../../computed/metrics/responsiveness.js';
import {CumulativeLayoutShift} from '../../computed/metrics/cumulative-layout-shift.js';
import {ExecutionContext} from '../driver/execution-context.js';

/** @typedef {{nodeId: number, score?: number, animations?: {name?: string, failureReasonsMask?: number, unsupportedProperties?: string[]}[], type?: string}} TraceElementData */
/** @typedef {{nodeId: number, animations?: {name?: string, failureReasonsMask?: number, unsupportedProperties?: string[]}[], type?: string}} TraceElementData */

const MAX_LAYOUT_SHIFT_ELEMENTS = 15;

Expand Down Expand Up @@ -76,12 +76,7 @@ class TraceElements extends BaseGatherer {
return [...impactByNodeId.entries()]
.sort((a, b) => b[1] - a[1])
.slice(0, MAX_LAYOUT_SHIFT_ELEMENTS)
.map(([nodeId, clsContribution]) => {
return {
nodeId: nodeId,
score: clsContribution,
};
});
.map(([nodeId]) => ({nodeId}));
}

/**
Expand Down Expand Up @@ -261,7 +256,6 @@ class TraceElements extends BaseGatherer {
traceElements.push({
traceEventType,
...response.result.value,
score: backendNodeData[i].score,
animations: backendNodeData[i].animations,
nodeId: backendNodeId,
type: backendNodeData[i].type,
Expand Down
62 changes: 35 additions & 27 deletions core/test/audits/layout-shift-elements-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ describe('Performance: layout-shift-elements audit', () => {
traces: {defaultPass: trace},
TraceElements: [{
traceEventType: 'layout-shift',
nodeId: 1,
node: {
devtoolsNodePath: '1,HTML,3,BODY,5,DIV,0,HEADER',
selector: 'div.l-header > div.chorus-emc__content',
nodeLabel: 'My Test Label',
snippet: '<h1 class="test-class">',
},
score: 0.3,
}],
};

Expand All @@ -64,21 +64,13 @@ describe('Performance: layout-shift-elements audit', () => {
expect(auditResult.details.items).toHaveLength(1);
expect(auditResult.details.items[0]).toHaveProperty('node');
expect(auditResult.details.items[0].node).toHaveProperty('type', 'node');
expect(auditResult.details.items[0].score).toEqual(0.3);
expect(auditResult.details.items[0].score).toEqual(0.4);
});

it('correctly surfaces multiple CLS elements', async () => {
const clsElement = {
traceEventType: 'layout-shift',
node: {
devtoolsNodePath: '1,HTML,3,BODY,5,DIV,0,HEADER',
selector: 'div.l-header > div.chorus-emc__content',
nodeLabel: 'My Test Label',
snippet: '<h1 class="test-class">',
},
score: 0.3,
};
const trace = createTestTrace({});
const traceElements = [];

for (let i = 1; i <= 4; ++i) {
trace.traceEvents.push({
args: {
Expand All @@ -97,10 +89,22 @@ describe('Performance: layout-shift-elements audit', () => {
name: 'LayoutShift',
cat: 'loading',
});

traceElements.push({
traceEventType: 'layout-shift',
nodeId: i,
node: {
devtoolsNodePath: '1,HTML,3,BODY,5,DIV,0,HEADER',
selector: 'div.l-header > div.chorus-emc__content',
nodeLabel: 'My Test Label',
snippet: '<h1 class="test-class">',
},
});
}

const artifacts = {
traces: {defaultPass: trace},
TraceElements: Array(4).fill(clsElement),
TraceElements: traceElements,
};

const auditResult = await LayoutShiftElementsAudit.audit(artifacts, {computedCache: new Map()});
Expand All @@ -111,19 +115,8 @@ describe('Performance: layout-shift-elements audit', () => {
});

it('aggregates elements missing from TraceElements', async () => {
const clsElement = {
traceEventType: 'layout-shift',
node: {
devtoolsNodePath: '1,HTML,3,BODY,5,DIV,0,HEADER',
selector: 'div.l-header > div.chorus-emc__content',
nodeLabel: 'My Test Label',
snippet: '<h1 class="test-class">',
},
score: 0.1,
};
const trace = createTestTrace({});

// Create 20 shift events on 20 unique elements
const trace = createTestTrace({});
for (let i = 1; i <= 20; ++i) {
trace.traceEvents.push({
args: {
Expand All @@ -143,10 +136,25 @@ describe('Performance: layout-shift-elements audit', () => {
cat: 'loading',
});
}

// Only create element data for the first 15 elements
const traceElements = [];
for (let i = 1; i <= 15; ++i) {
traceElements.push({
traceEventType: 'layout-shift',
nodeId: i,
node: {
devtoolsNodePath: '1,HTML,3,BODY,5,DIV,0,HEADER',
selector: 'div.l-header > div.chorus-emc__content',
nodeLabel: 'My Test Label',
snippet: '<h1 class="test-class">',
},
});
}

const artifacts = {
traces: {defaultPass: trace},
// Only create element data for the first 15 elements
TraceElements: Array(15).fill(clsElement),
TraceElements: traceElements,
};

const auditResult = await LayoutShiftElementsAudit.audit(artifacts, {computedCache: new Map()});
Expand Down
48 changes: 17 additions & 31 deletions core/test/gather/gatherers/trace-elements-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,6 @@ function makeLCPTraceEvent(nodeId) {
}

describe('Trace Elements gatherer - GetTopLayoutShiftElements', () => {
/**
* @param {Array<{nodeId: number, score: number}>} shiftScores
*/
function sumScores(shiftScores) {
let sum = 0;
shiftScores.forEach(shift => sum += shift.score);
return sum;
}

it('returns layout shift data sorted by impact area', async () => {
const trace = createTestTrace({});
trace.traceEvents.push(
Expand All @@ -102,11 +93,9 @@ describe('Trace Elements gatherer - GetTopLayoutShiftElements', () => {
const result =
await TraceElementsGatherer.getTopLayoutShiftElements(trace, {computedCache: new Map()});
expect(result).toEqual([
{nodeId: 25, score: 0.6},
{nodeId: 60, score: 0.4},
{nodeId: 25}, // score: 0.6
{nodeId: 60}, // score: 0.4
]);
const total = sumScores(result);
expect(total).toBeCloseTo(1.0);
});

it('returns only the top 15 values', async () => {
Expand Down Expand Up @@ -167,24 +156,22 @@ describe('Trace Elements gatherer - GetTopLayoutShiftElements', () => {
const result =
await TraceElementsGatherer.getTopLayoutShiftElements(trace, {computedCache: new Map()});
expect(result).toEqual([
{nodeId: 3, score: 1.0},
{nodeId: 1, score: 0.5},
{nodeId: 2, score: 0.5},
{nodeId: 6, score: 0.25},
{nodeId: 7, score: 0.25},
{nodeId: 4, score: 0.125},
{nodeId: 5, score: 0.125},
{nodeId: 8, score: 0.1},
{nodeId: 9, score: 0.1},
{nodeId: 10, score: 0.1},
{nodeId: 11, score: 0.1},
{nodeId: 12, score: 0.1},
{nodeId: 13, score: 0.1},
{nodeId: 14, score: 0.1},
{nodeId: 15, score: 0.1},
{nodeId: 3}, // score: 1.0
{nodeId: 1}, // score: 0.5
{nodeId: 2}, // score: 0.5
{nodeId: 6}, // score: 0.25
{nodeId: 7}, // score: 0.25
{nodeId: 4}, // score: 0.125
{nodeId: 5}, // score: 0.125
{nodeId: 8}, // score: 0.1
{nodeId: 9}, // score: 0.1
{nodeId: 10}, // score: 0.1
{nodeId: 11}, // score: 0.1
{nodeId: 12}, // score: 0.1
{nodeId: 13}, // score: 0.1
{nodeId: 14}, // score: 0.1
{nodeId: 15}, // score: 0.1
]);
const total = sumScores(result);
expect(total).toBeCloseTo(3.55);
});
});

Expand Down Expand Up @@ -379,7 +366,6 @@ describe('Trace Elements gatherer - Animated Elements', () => {
},
{
...layoutShiftNodeData,
score: 0.5, // the other CLS node contributed an additional 0.5, but it was 'no node found'
nodeId: 4,
},
{
Expand Down
3 changes: 1 addition & 2 deletions types/artifacts.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -560,9 +560,8 @@ declare module Artifacts {

interface TraceElement {
traceEventType: 'largest-contentful-paint'|'layout-shift'|'animation'|'responsiveness';
score?: number;
node: NodeDetails;
nodeId?: number;
nodeId: number;
animations?: {name?: string, failureReasonsMask?: number, unsupportedProperties?: string[]}[];
type?: string;
}
Expand Down

0 comments on commit a60c73d

Please sign in to comment.