fix(browser): Set artificial zero-value CLS measurement to report no layout shift #12989
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lms24
commented
Jul 19, 2024
| </head> | ||
| <body> | ||
| <div id="content"> | ||
| Some content |
There was a problem hiding this comment.
There needs to be something visible on the page because we (rightfully) only send CLS measurements if FCP was also recorded.
Lms24
force-pushed
the
lms/fix-browser-cls-zero-values
branch
from
8976584
to
0ded265
Compare
size-limit report 📦
|
edwardgou-sentry
approved these changes
Jul 19, 2024
There was a problem hiding this comment.
I don't have any objections to this, I think it makes sense!
I'd like to confirm that it's okay to send a CLS measurement but no CLS attribution/source element as additional information.
I don't believe we apply any additional logic to cls sources in relay (we don't use this for calculation), and it just gets saved as a tag, so we should be safe here.
Lms24
commented
Jul 22, 2024
There was a problem hiding this comment.
Note: We have 3 more integration tests covering >0 CLS values which all still pass. So I don't think that this change somehow causes actual layout shift values to be ignored.
AbhiPrasad
approved these changes
Jul 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So this is a fun one:
The
PerformanceObserveremitsLayoutShiftentries whenever a layout shift occurs. If no layour shift occurs (which is great!), the observer will never emit any entries. Makes sense so far!Unfortunately, this is problematic for the Sentry product/UI. We can't differentiate between a CLS of 0 and not having received CLS data at all. So in both cases, we'd show users that the CLS score simply is not available. When in fact, it can be 0, which is a very good score.
This PR adds a workaround to artificially set a CLS of 0 right at the start of listening to CLS events. This way, we can differentiate between a CLS of 0 and no CLS at all. If a layout shift occurs later, the real CLS value will be emitted and the 0 value will be overwritten. We also only send this artificial 0 value if the browser supports reporting the
layout-shiftentry type to avoid skewing the CLS score.Note
There was some discussion around if this change should be guarded with an experimental flag. I don't think that this change has a high-impact potential for the majority of interactive applications (like the Sentry frontend for example). For example, even our docs, which appear rather static, do expose a tiny bit if layout shift on all pages as there's some dynamic data fetching and UI update going on to retrieve the latest package version. Also, I couldn't find a single page in Sentry with no layout shift. In both projects, there are a number of pages with 0 layout shift but as far as I can tell, this is because we round tiny layout shift measurements to 0. So this change wouldn't have any effects on such pages.
This change really affects apps the most that have fully statically generated pages (e.g. Astro by default or other SSG-created apps). And even then, there could still be layout shift (e.g. fonts loaded lazily or scripts injecting elements).
Adding to this, we have tests that show that actual layout shift (>0) is still reported correctly. I think it's fine to ship this with the next release. Also discussed this briefly with some members of the team and they agreed. Happy to still guard it if reviewers would prefer it though.