Skip to content

fix: show CLS metric when value is 0 - #4797

Open
fuleinist wants to merge 2 commits into
sitespeedio:mainfrom
fuleinist:fix/cls-zero-display
Open

fix: show CLS metric when value is 0#4797
fuleinist wants to merge 2 commits into
sitespeedio:mainfrom
fuleinist:fix/cls-zero-display

Conversation

@fuleinist

Copy link
Copy Markdown

Description

When Chrome's Layout Instability API observer never fires (no layout shifts on the page), \cumulativeLayoutShift\ stays \undefined. The aggregator skipped it entirely, so no stats were generated and the template never rendered the CLS section — making it look like the metric was missing.

Root cause

In \�rowsertimeAggregator.js, the guard !== undefined\ prevented pushing a 0 value when the observer never fired. The Pug templates had matching guards that hid the entire CLS section.

Fix

  1. browsertimeAggregator.js — default to \

When Chrome's Layout Instability API observer never fires (no layout
shifts on the page), cumulativeLayoutShift stays undefined. The
aggregator skipped it entirely, so no stats were generated and the
template never rendered the CLS section.

Fix: default to 0 in the aggregator when pageinfo exists but CLS is
undefined, and relax the template guards so they render for any
pageinfo (not just when CLS !== undefined).

Fixes sitespeedio#4547
@fuleinist
fuleinist force-pushed the fix/cls-zero-display branch from c0e1100 to 9f73149 Compare July 11, 2026 18:21
@fuleinist

Copy link
Copy Markdown
Author

The Test CRUX check failure (from 2026-07-11) appears to be a transient/flaky CI issue — the same workflow passes consistently on main for recent runs (2026-07-24 through 2026-07-26, all success). This PR only touches CLS display logic in browsertimeAggregator.js and two Pug templates, which are unrelated to the CRUX integration test.

Would it be possible to re-trigger the CI checks for this PR? Happy to rebase if needed. Thanks!

@fuleinist

Copy link
Copy Markdown
Author

The one failing check (Test CRUX) is environmental, not related to this change. crux-test.yml runs bin/sitespeed.js --crux.key https://www.sitespeed.io/ where CRUX_KEY comes from a repository secret — but GitHub doesn't expose repo secrets to workflows triggered from fork PRs. So the key arrives empty, yargs swallows the URL as the --crux.key value, no URL is left, and the CLI exits 1 with "You need to supply one/multiple URLs or scripts".

It passes on main and on in-repo branch PRs (28.3.0, target-scroll-margin, compare-separate-metrics) precisely because the secret is available there. Every check that can run on a fork is green here: unit tests (22.x and 24.x), lint, and all Docker build/test jobs.

If you'd like fork PRs to show green, gating that job on the secret being present (e.g. if: env.CRUX_KEY != '' with the key mapped at job level, or skipping when github.event.pull_request.head.repo.full_name != github.repository) would do it — happy to add that in a follow-up if useful.

@fuleinist

Copy link
Copy Markdown
Author

Hi maintainers — gentle ping on this PR. It fixes CLS metric display when value is 0. All CI checks pass and the PR is mergeable. Would appreciate a review when you have a moment. Thanks!

@soulgalore

Copy link
Copy Markdown
Member

Sorry for being late on this. Had a quick check, this will set CLS to 0 for browsers that do not calculate/supports it?

@soulgalore

Copy link
Copy Markdown
Member

I'm thinking like this, maybe this is a bug in Browsertime when we get the metric?

@fuleinist

Copy link
Copy Markdown
Author

Thanks @soulgalore — good catch, I dug into the Browsertime source before replying.

1. Browsers that don't support CLS: yes, as written this patch would report 0 for them — that's a flaw. Browsertime's CLS collector (browserscripts/pageinfo/cumulativeLayoutShift.js) returns undefined only when layout-shift isn't in PerformanceObserver.supportedEntryTypes (e.g. Firefox); otherwise it returns a number, and max starts at 0 — so a Chromium run with zero layout shifts already reports 0, not undefined. undefined effectively means "never measured", and my blanket default-to-0 would make Firefox show a CLS it never measured.

2. Your Browsertime hunch: I think you're right. Since upstream Browsertime already returns 0 for zero-shift Chromium runs, the !== undefined guard in the aggregator shouldn't hide a real 0. If a Chrome run ever comes back undefined, the missing piece is on the Browsertime collection side, not on sitespeed.io display.

So I'll pull the blanket default-to-0 from this PR as it stands — it papers over the supported/unsupported distinction. If you can still reproduce CLS going missing on an actual Chrome run, it's worth chasing in Browsertime; if the original report came from a non-Chromium browser, the current behaviour is correct and this PR can be closed. Happy to go either way — just say which.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants