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

tests(smoke): update CLS-AF expectation #12353

Merged
merged 2 commits into from
Apr 12, 2021
Merged

tests(smoke): update CLS-AF expectation #12353

merged 2 commits into from
Apr 12, 2021

Conversation

adamraine
Copy link
Member

CLS weighted scores were erroneous and should be fixed in ToT:
https://chromium.googlesource.com/chromium/src/+/042fbfb4cc6a675da0dff4bf3fc08622af42422b

@adamraine adamraine requested a review from a team as a code owner April 12, 2021 16:56
@adamraine adamraine requested review from jckr and removed request for a team April 12, 2021 16:56
@google-cla google-cla bot added the cla: yes label Apr 12, 2021
@brendankenny
Copy link
Member

Interesting that this affects us. From the change description, at least, I wouldn't expect handling zoom would change things for our smoke test.

@adamraine
Copy link
Member Author

This was the full bisect list:
https://chromium.googlesource.com/chromium/src/+log/02c23acd7d6c319ae7976208159e202abd246934..8384f1379a4346fc78da6945a335d947e75632f8

I didn't notice any other suspect changes.

@brendankenny
Copy link
Member

oh, I think you're definitely right, I'm just trying to understand why our number would change. I have no idea what the larger implications of PropertyTreeState::Root() vs frame.LocalFrameRoot().ContentLayoutObject()->FirstFragment().LocalBorderBoxProperties() are, though :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

There's some weirdness here that @adamraine, @paulirish, and I looked into a little bit but haven't figured out yet. e.g. when I try out the crbug's demo https://output.jsbin.com/wegixex using m91 and m92 (where the change landed), I'm not getting different CLS score or weightedScoreDelta in Lighthouse (have to hack around NO_FCP errors). If this change implies that our defaults run into a CLS change with m92, I would have expected a change on that page as well, but maybe there's something about the iframe relative sizing that just happens to work out?

However, when we've landed clsAF smoke changes before (#11874 and #12034) I don't think we actually verified the numbers, we just trusted what Chrome gave us. Should we do the same here to unblock CI and file a follow-up issue to make sure we do our due diligence before making all-frame stuff visible in the report?

@brendankenny brendankenny merged commit 33d48d2 into master Apr 12, 2021
@brendankenny brendankenny deleted the cls-af-smoke-fix branch April 12, 2021 20:14
@brendankenny
Copy link
Member

when I try out the crbug's demo https://output.jsbin.com/wegixex using m91 and m92 (where the change landed), I'm not getting different CLS score or weightedScoreDelta in Lighthouse

@patrickhulce pointed out that the demo page has a <meta name="viewport" content="width=device-width"> but our smoke test does not, which would affect zooming. Adding a meta viewport to the smoke test makes it so CLS doesn't change from m91 to m92, so that probably solves that mystery.

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.

4 participants