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

Ramp "intersect-resources" to 100% (third try) #29733

Merged
merged 3 commits into from
Aug 7, 2020

Conversation

dreamofabear
Copy link

Fixes #25428. 1% ads regression appears fixed in canary after #29581.

@google-cla google-cla bot added the cla: yes label Aug 7, 2020
@dreamofabear dreamofabear requested a review from samouri August 7, 2020 05:11
@kristoferbaxter
Copy link
Contributor

Restarted End to End tests, approving.

@dreamofabear
Copy link
Author

The protocol adapters e2e test failure is actually legitimate. The intersection callback happens before amp-script.css is loaded, so it detects the amp-script element as having height: 0. This means layoutCallback() is never called.

I think the core issue is that amp-script.css technically causes content jumping by resizing the element.

@samouri
Copy link
Member

samouri commented Sep 10, 2020

The protocol adapters e2e test failure is actually legitimate. The intersection callback happens before amp-script.css is loaded, so it detects the amp-script element as having height: 0. This means layoutCallback() is never called.

There's a larger pattern that this exposed regarding a new requirement w/ intersect-resources. Before IO, measures were always done after build. With IO, measures will all likely happen pre-build, and therefore we need elements to have correct width/height even before build (for those that don't explicitly mutate).

New case: #30167

@dreamofabear
Copy link
Author

Good catch. This is actually a coincidental invariant since "size change after build" can cause CLS.

@samouri
Copy link
Member

samouri commented Sep 10, 2020

Different bug but also related to IO. This one about using an outdated premeasure and incorrectly skipping layout:

#30188

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
* Enable intersect-resources in 100% prod.

* Temporarily skip problematic test.
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.

Resource scheduling with IntersectionObserver
4 participants