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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 [Story performance] Make inactive pages not trigger LCP #35323

merged 12 commits into from Jul 23, 2021


Copy link

@mszylkowski mszylkowski commented Jul 20, 2021

By updating the way the inactive pages are positioned, we can make the inactive pages not count towards LCP. In this PR, we can set the pages with distance="1" to be outside the viewport only if they are not loaded (or previously visited), so that there's no background flash of black in between page advancements (unless the second page is not loaded already).


This PR will not affect the desktop 3 panels layout since the inactive pages are seen on the sides. The new desktop 1-panel UI does not show the inactive pages, so it is fixed by this PR (it doesn't set the mode to desktop). Can be tested with: AMP.toggleExperiment('amp-story-desktop-one-panel', true)

@mszylkowski mszylkowski marked this pull request as ready for review July 21, 2021 21:49
Copy link

Hey @gmajoulet, @newmuis! These files were changed:


@mszylkowski mszylkowski self-assigned this Jul 21, 2021
@mszylkowski mszylkowski added this to In progress in wg-stories Sprint via automation Jul 21, 2021
Copy link

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

It took me quite some time to understand that your testing samples are overriding AMP.toggleExperiment('amp-story-desktop-one-panel', true).
I'll merge it so it gets to prod one week earlier, but in the future let's use the conventional experiment toggles

@gmajoulet gmajoulet merged commit 1cd405b into ampproject:main Jul 23, 2021
wg-stories Sprint automation moved this from In progress to Done Jul 23, 2021
Copy link

Two follow ups:

  • Please verify that this PR works with text blocks. The images take some time to load while text blocks are instantly displayed. I wonder if your CSS class that's added pretty late also hides an element that's rendered with no delay. if we only have one test case it needs to look like what we see in the wild most often. Text nodes usually trigger LCP, we should test this
  • Please remove the unconventional experiment override through URL parameter. If you want to make it easy to toggle the experiment, add a button that uses the AMP.toggleExperiment method.

Copy link
Contributor Author


Text blocks will start outside of the page so they don't count towards LCP when activating this PR, but also didn't happen before. Since images load later, there was a chance that the image could be behind the current page when it loads, triggering LCP at that point. So, this PR fixes images but doesn't affect text on inactive pages, since they were never a problem.

Also, making a PR now that doesn't toggle the experiment on the demo stories if the URL param is not present

Copy link

So all the cases we had where the LCP element was on page 2 was only images, never text nodes?

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

Successfully merging this pull request may close these issues.

None yet

3 participants