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

Reduce cumulative layout shift (CLS) by attempting to determine heights of embeds beforehand #4729

Open
westonruter opened this issue May 14, 2020 · 16 comments
Labels
Bento Editor Optimizer P2 Low priority WS:Perf Work stream for Metrics, Performance and Optimizer

Comments

@westonruter
Copy link
Member

westonruter commented May 14, 2020

Feature description

Cumulative Layout Shift (CLS) is a huge detriment to user experience on the web. It is also a huge challenge to solve in the case of 3rd-party embeds. For some AMP components, in particular amp-iframe and amp-list, elements that need to resize based on their contents need to provide an overflow button that needs to be clicked by the user to trigger the element to grow in height if the element is currently in or above the current viewport. The presence of this overflow button can be annoying for users and so it's important the initial height be set to be as close as possible to where the element will be once loaded so that, in the best case, no overflow button needs to be presented.

Nevertheless, for pragmatic reasons, other components do not have such overflow logic and they will resize to fit their contents regardless of being in the current viewport or above. For example, amp-gist:

<amp-gist data-gistid="0769f147e021e0ebfcb04a084987483a" layout="fixed-height" height="10"></amp-gist>
<p>Super interesting information!</p>

When the page loads at first, the user will see “Super interesting information!” momentarily until the Gist loads. This is a gross instance of CLS. The docs for the component call this out in the description for height:

data-gistid (required) The ID of the gist to embed. layout (required) Currently only supports fixed-height. height (required) The initial height of the gist or gist file in pixels. Note: You should obtain the height of the gist by inspecting it with your browser (e.g., Chrome Developer Tools). Once the Gist loads the contained iframe will resize to fit so that its contents will fit. data-file (optional) If specified, display only one file in a gist.

Similar guidance is provided for amp-twitter:

Twitter does not currently provide an API that yields fixed aspect ratio for embedded Tweets or Moments. Currently, AMP automatically proportionally scales the Tweet or Moment to fit the provided size, but this may yield less than ideal appearance. You might need to manually tweak the provided width and height. Also, you can use the media attribute to select the aspect ratio based on the screen width.

This issue, however, is not unique to Gists and Tweets, but it's also an issue for other embeds. For example, embedding Facebook posts also have this issue and the amp-facebook component lacks that same guidance.

All this being said, we should automatically provide the best approximate height for these components as much as possible.

One way this could be done, specifically in the block editor, is to render the component and then scrape the resulting height and then inject that as the height of the embed. This couldn't be done currently with the PHP Optimizer since we need a browser context to load the page, but perhaps the Node Optimizer could add Puppeteer as a dependency to obtain initial heights.

This would essentially be an augmentation of SSR.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • After having added an Embed block to a post, saving and viewing the AMP page should should require any layout shifting for the block on the frontend.
  • Bonus: Also set the height of embeds on non-AMP pages.

Implementation brief

  1. Check the initial post content for any embeds that lack heights, and watch for insertions or updates to Embed blocks.
  2. Once the Embed block clears its loading state, measure the height and add as a custom block attribute.
  3. Add a render_block filter that passes along the height block attribute to be inserted as a data-amp-embed-height HTML attribute on the figure wrapper element.
  4. Add toAMP_Embed_Sanitizer::sanitize() logic which finds all //figure[ @data-amp-embed-height ] and then copies the value to the ./div[ contains( @class, 'wp-block-embed__wrapper' ) ]/*[starts-with(name(), 'amp-')].

QA testing instructions

Demo

Changelog entry

@schlessera
Copy link
Collaborator

For some of these, we might still be able to calculate a close approximate height on the serverside based on the content from the embed, but it would mean supporting adding per-embed logic to do so.

For example, for a gist, you'll probably get very close by counting the number of lines and then calculating something like $top_chrome + ( $lines * $line_height ) + $horizontal_scrollbar? + $bottom_chrome. In most cases, this can yield a pixel perfect height estimate, so you'd be able to block the correct height right from the start.

To get the number of lines and the line height, you'd fetch the embed content from the server and parse the return HTML.

Image 2020-05-15 at 7 41 28 AM

Image 2020-05-15 at 7 42 00 AM

Given that there is direct support in AMP for something lie a gist via its specific component, I don't think it's necessarily bad to mirror that custom support in the Optimizer, and have a few lines of custom logic per component to calculate its height on the server-side.

@westonruter westonruter linked a pull request May 16, 2020 that will close this issue
3 tasks
@kmyram kmyram added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 5, 2020
@westonruter
Copy link
Member Author

Obtaining the dimensions of embedded content is going to be increasingly important as part of the Bento effort because components like amp-twitter, amp-facebook, etc will no longer be resizing themselves without user interaction. In other words, they will require overflow buttons on AMP pages to expand if they are in the first viewport. See ampproject/amphtml#35027 and ampproject/amphtml#34948 (comment).

While it is possible to approximate the height of embeds server-side, this would indeed need to be done uniquely for each embed. And then it would have to be maintained for each embed. I actually had to do this for the Pinterest embed in Jetpack via Automattic/jetpack#17086, since at the time amp-interest did not autoresize after the first layout (see ampproject/amphtml#10318 (comment)).

The code involves a lot of magic numbers: https://github.com/Automattic/jetpack/blob/36151c453f478f613204768725e236be8bd7949a/projects/plugins/jetpack/extensions/blocks/pinterest/pinterest.php#L70-L210

I think it will be much more scalable to do this client-side in the editor. When an embed is rendered, we can store the height as a custom block attribute which we then utilize to supply the height when rendering (either in an embed handler or embed sanitizer).

@westonruter
Copy link
Member Author

Added AC and IB.

@westonruter
Copy link
Member Author

The downside to the client-side approach is that any existing embeds will not have sizes supplied. To supply the initial heights, a site owner would have to touch it: open the post in the editor and re-save once the embeds have loaded. In order to supply the heights for all previous embeds in an automated way, this would involve having some UI that programmatically opens each post one-by-one in an editor and saves if they enter a dirty state.

Something similar in concept is the Convert to Blocks plugin, but it doesn't do batch updating of all posts. It still requires opening posts one-by-one.

Batch supplying heights retroactively to older posts is out of scope for this issue, but it's something interesting to consider for the future.

@westonruter
Copy link
Member Author

It seems we may need to check responsive breakpoints to determine the heights of the embeds. Adding a tweet I can see that on mobile it is ~500px high but on tablet/desktop it is 630px high:

responsive-embeds.mov
Mobile Tablet/Desktop
image image

That complicates things a bit. Do we capture just the mobile dimensions? Or do we capture mobile, tablet, and desktop and store them all with the embed's block attributes? But when we render the amp-twitter onto the page, what dimensions should we be rendering? Do we need to render separate components for each breakpoint via the media queries?

<amp-twitter class="mobile" width="auto" height="500" layout="fixed-height" data-tweetid="1405578489554743296" media="(max-width: 360px)"></amp-twitter>
<amp-twitter class="desktop" width="auto" height="630" layout="fixed-height" data-tweetid="1405578489554743296" media="(min-width: 361px)"></amp-twitter>

That doesn't seem ideal.

@schlessera
Copy link
Collaborator

Instead of using the media attribute and including multiple versions of a same element, we could use inline styles like we do with other parts of the SSR transformation, so that we can encode all breakpoints onto a single AMP element.

@schlessera
Copy link
Collaborator

schlessera commented Jun 28, 2021

Nevermind, there's no layout that would fit our needs and allow us to not specify the height, as we would need knowledge about the "aspect ratio" encoding into the HTML markup in all of the scenarios.

@westonruter
Copy link
Member Author

westonruter commented Jul 26, 2022

I did some work on a standalone app to help determine initial heights to reduce layout shift a year ago: https://googlechromelabs.github.io/layout-shift-terminator/

For more see: https://web.dev/embed-best-practices/

Granted, the functionality would be much improved with container queries live, but they're almost ready: https://caniuse.com/?search=container%20queries

So we could code specifically for them with the necessary browser flags enabled to test them.

@westonruter westonruter added the P0 High priority label Jul 26, 2022
@westonruter
Copy link
Member Author

westonruter commented Jul 26, 2022

This could be contributed eventually to the WordPress Performance plugin as well. See WordPress/performance#117 (comment) and GoogleChromeLabs/layout-shift-terminator#5.

@thelovekesh
Copy link
Collaborator

@westonruter I performed some research on the above-mentioned scenarios and here are the findings.

  • We can measure the height of the embed on the device on which the block editor is opened effectively. For other viewports, we can follow the same workaround as the layout shift terminator tool, but in that case, the main thread will be blocked for some time while the height gets measured for all viewports.

  • I found an interesting thing which is we'll still get CLS when an embed is added with height but it will be in the respect of the measured area.

See these lighthouse result:

  1. Embed added without height - https://web.dev/measure/?url=https%3A%2F%2Fthecodebulbs.github.io%2Fcls-test%2F
  2. Embed added after CLS termination(used above-mentioned tool) - https://web.dev/measure/?url=https%3A%2F%2Fthecodebulbs.github.io%2Fcls-test%2Ftweet

@westonruter
Copy link
Member Author

  • the main thread will be blocked for some time while the height gets measured for all viewports.

Are you saying this will cause jank in the editor? For the Facebook Video embed, I'm seeing two long tasks for HTML parsing and FB script execution:

image

This would particularly be annoying if the sizing is done after insertion while the user is still typing.

I guess a way around it would be to wait to do the sizing until the post-publish step. But that would require the user to wait a minute before publishing, which would be annoying.

Lastly, I wonder if there is anything that can be done with out-of-process iframes. From reading https://stackoverflow.com/q/11510483/93579 it seems that an iframe that gets sandbox="allow-scripts" may be forced into a separate process, at least in Chrome Canary. This would be something good to investigate further.

  • I found an interesting thing which is we'll still get CLS when an embed is added with height but it will be in the respect of the measured area.

I'm not sure I understand.

Could you add a <hr> after the example embeds so we can see layout shift in action?

@thelovekesh
Copy link
Collaborator

thelovekesh commented Aug 10, 2022

This would particularly be annoying if the sizing is done after insertion while the user is still typing.

For this, I am further investigating the use cases with iframe sandboxing as you suggested above. But furthermore, we will need to benchmark such scenarios which can affect editor performance.

Could you add a <hr> after the example embeds so we can see layout shift in action?

I have added <hr>. Please check now.

@westonruter
Copy link
Member Author

  • I found an interesting thing which is we'll still get CLS when an embed is added with height but it will be in the respect of the measured area.

I'm not sure what you mean here. Could you elaborate?

@thelovekesh
Copy link
Collaborator

By the above statement I mean the lighthouse is still showing CLS even though I have provided element height by using Layout Shift Terminator.

@westonruter
Copy link
Member Author

Strange. From the filmstrip, it doesn't seem that the initial height is being reserved. I can see the <hr> appearing immediately after the <blockquote> in the first screenshot:

image

There should be a significant amount of space.

@thelovekesh
Copy link
Collaborator

@westonruter I have created a POC to demonstrate the possibilities of terminating CLS by measuring dimensions in an iframe. But instead of handling this automatically, I have provided a custom setting, and I will explore more approaches to make this process automatic.

You can find the plugin here - https://github.com/rtCamp/wp-cls-terminator.

Please note I have temporarily added only a few breakpoints, which can be updated later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bento Editor Optimizer P2 Low priority WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
Archived in project
Development

No branches or pull requests

4 participants