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

core(metrics): support CLS for all frames #11713

Merged
merged 16 commits into from
Nov 26, 2020
Merged

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Nov 25, 2020

Part 1a (step 2?) of the Framehole implementation plan for CLS.

The cumulative_score arg in LayoutShift trace events is computed per-frame. We could find the latest LayoutShift event for each frame but it seemed simpler to compute our own cumulative score from every layout shift event.

Leaving this as a draft for now, will change the merge into branch to master once #11701 lands.

@google-cla google-cla bot added the cla: yes label Nov 25, 2020
*/
static async compute_(trace) {
const cumulativeShift = trace.traceEvents
.filter(e => e.name === 'LayoutShift' && e.args && e.args.data && e.args.data.score)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can narrow the type by annotating the filter function to be a type guard. here's an example: https://github.com/GoogleChrome/lighthouse/pull/7817/files#diff-b66c6ac9b4b8c0726c736eb7479a9e4ff22c7b81bfa1b2b259c2cdfa220ea1f8R19

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh nvm the type here is kinda complicated, a ts ignore on L19 is probably better

Copy link
Collaborator

Choose a reason for hiding this comment

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

if this weren't on trace events I would suggest the map first and then a filter which is much easier in jsdoc type land :)

on the functionality itself though, we should normally be excluding layout shifts with had_recent_input=true, I don't expect it to have much effect but for comparison to the existing definition we should match it for now. also see fun stuff like #10960 :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

totes forgot about the recent input thing. that was a rough bug

@patrickhulce patrickhulce changed the title core(metrics): support CSP for all frames core(metrics): support CLS for all frames Nov 25, 2020
*/
static async compute_(trace) {
const cumulativeShift = trace.traceEvents
.filter(e => e.name === 'LayoutShift' && e.args && e.args.data && e.args.data.score)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this weren't on trace events I would suggest the map first and then a filter which is much easier in jsdoc type land :)

on the functionality itself though, we should normally be excluding layout shifts with had_recent_input=true, I don't expect it to have much effect but for comparison to the existing definition we should match it for now. also see fun stuff like #10960 :)

lighthouse-core/computed/metrics/timing-summary.js Outdated Show resolved Hide resolved
lighthouse-core/lib/minify-trace.js Show resolved Hide resolved
lighthouse-core/test/audits/metrics-test.js Outdated Show resolved Hide resolved
@adamraine adamraine marked this pull request as ready for review November 25, 2020 22:21
@adamraine adamraine requested a review from a team as a code owner November 25, 2020 22:21
@adamraine adamraine requested review from Beytoven and removed request for a team November 25, 2020 22:21
@adamraine
Copy link
Member Author

Both this and #11701 look ready to go. Any objections to merging this into #11701 and landing together?

@patrickhulce
Copy link
Collaborator

Any objections to merging this into #11701 and landing together?

image

Base automatically changed from lcp-all-frames-metrics to master November 26, 2020 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants