Make WordPress Core

Opened 5 months ago

Last modified 8 days ago

#58773 assigned defect (bug)

Improve oEmbed lazy loading

Reported by: adamsilverstein's profile adamsilverstein Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: needs-patch
Focuses: performance, sustainability Cc:

Description (last modified by adamsilverstein)

Why lazy load oEmbed iframes?

  • Lazy loading prevents wasted resources from loading if the user never scrolls down to view the oEmbed
  • Iframe lazy loading is handled directly by the browser, making the change simple (add a single attriute) and ensuring users get a great experience

Lazy loaded oEmbeds history

WordPress has lazy loaded iframes since version 5.7 and the original proposal included lazy loading for oEmbed iframes (as long as the provider also included width and height attributes).

Ultimately, dynamic addition of lazy loading of oEmbeds was removed from core because it was found to create issues, especially with embeds of other WordPress posts.

Proposed Solution

This ticket proposes slightly different approach that relies on altering the iframe instance on the fly for each oEmbed. In particular:

  • When the oEmbed iframe HTML is displayed, if it doesn't already contain a loading attribute, it is run through wp_iframe_tag_add_loading_attr to add the lazy loading iframe under the correct conditions
    • Will not add lazy loading to WordPress iframes because of the data-secret check
    • Only adds lazy loading to iframes with the height and width present.
    • Runs before the oembed_result filter, so developers can remove the attribute if desired.

Attachments (6)

58773.diff (1.8 KB) - added by adamsilverstein 5 months ago.
before-change.jpg (117.7 KB) - added by adamsilverstein 5 months ago.
lazy.jpg (197.5 KB) - added by adamsilverstein 5 months ago.
wpembed.jpg (191.4 KB) - added by adamsilverstein 5 months ago.
58773.2.diff (3.2 KB) - added by adamsilverstein 5 months ago.
58773.3.diff (2.6 KB) - added by adamsilverstein 5 months ago.

Download all attachments as: .zip

Change History (28)

#1 @adamsilverstein
5 months ago

  • Component changed from General to Embeds
  • Focuses performance sustainability added
  • Keywords needs-patch needs-unit-tests needs-testing added
  • Owner set to adamsilverstein
  • Status changed from new to assigned

#2 @adamsilverstein
5 months ago

  • Milestone changed from Awaiting Review to 6.4

This ticket was mentioned in PR #4822 on WordPress/wordpress-develop by @adamsilverstein.


5 months ago
#3

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

@adamsilverstein
5 months ago

#4 @adamsilverstein
5 months ago

Note in before-change.jpg a default YouTube embed does not contain a loading attribute (on trunk).

With the patch applied in lazy.jpg we see the loading="lazy" attribute be ing applied.

For a test post with a WordPress post embed, note that the data-secret attribute prevents the addition of the loading="lazy" attribute - see wpembed.jpg.

#5 @adamsilverstein
5 months ago

Steps to reproduce

  • Create a post with several pages worth of content, then paste a youtube (or other embeddable) link to create the embed block
  • Before applying the patch, view the post
    • note iframe content loads immediately on page load, including all resources
    • note YT iframe lacks loading attribute
  • Apply the patch
  • Clear the post cache or create a new poat with the same content ( wp embed cache clear POST_ID)
  • Check the post on the front end again.
    • checking the network panel, note that the YT embed isn't loaded until you scroll down on the page.
    • view the page source and note that loading="lazy" has been added to the YT iframe

Help needed

  • It would be good to manually go through all of the WordPress supported oEmbed types to make sure they all work correctly with this change applied
  • It would be nice to have some benchmarks here, although we'll see the biggest change for actual users in the field.

#6 @swissspidy
5 months ago

What was the issue with the WordPress oEmbed iframes? Can we make it work for them?

#7 @adamsilverstein
5 months ago

What was the issue with the WordPress oEmbed iframes? Can we make it work for them?

Possibly, I believe they were removed explicitly because of issues in the way they are rendered - there is JavaScript that communicates from inside the iframe to the containing page at load time that didn't work when we added lazy loading. Worth revisiting for sure, maybe we can fix that part.

#8 @adamsilverstein
5 months ago

  • Summary changed from Lazy Load oEmbed iframes to Lazy load oEmbed iframes

#9 @adamsilverstein
5 months ago

  • Description modified (diff)

#10 @adamsilverstein
5 months ago

I created a test site with a bunch of different oEmbeds (although not every type WP supports): https://oembeds-test.instawp.xyz/

In my testing some oEmbed endpoints like YouTube return an iframe and the approach in this ticket properly adds the loading="lazy" attribute. Other oEmbed endpoints instead return a script tag that generates the oEmbed iframe HTML at runtime when executed.

For example, the twitter oEmbed endpoint returns something like this:

<blockquote class="twitter-tweet" data-width="500" data-dnt="true">
<a href="https://twitter.com/example/status/1634286225552035873?ref_src=twsrc%5Etfw">March 10, 2023</a></blockquote>
<script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>"

When the widget.js script executes, it expands the tweet inline into a full visually rich iframe. Since this happens after load time we can't alter the iframe code. We might consider swapping defer for async or adding fetchpriority="low to these scripts as a way of de-prioritizing them after researching potential benefits or pitfalls.

#11 @adamsilverstein
5 months ago

Needs testing -

  • oEmbeds in block themes vs classic themes
  • Are oembed iframes passed to the content - if so, why doesn't the lazy loading work there? if not, why?

#12 follow-up: @adamsilverstein
5 months ago

After discussing this further with @flixos90 I re-tested and it turns out the regular content filters do work correctly for oEmbed iframes. The issue with my tests is that I was only testing with a single or two oEmbeds and wp_omit_loading_attr_threshold is now set at 3. Oof!

Querying httparchive (query below), I found that for WordPress sites that use an oEmbed block, the average number of oEmbeds is below 3:

origins with embed blockaverage_embed_block_count
484452.8925172876457887

I wonder if we should consider lowering the threshold for iframes back to 1? (or 0 if iframe in viewport initial loads are rare?) Maybe we can query httparchive to see how common the LCP element being an iframe is? or how many iframes we are already adding lazy to...

Here is the query I used to check for the oEmbed count:

WITH
  WPEmbeds AS (
    SELECT
      url,
      JSON_EXTRACT(payload, '$._cms.wordpress.has_embed_block') AS has_embed_block,
      CAST( JSON_EXTRACT(payload, '$._cms.wordpress.embed_block_count.total') AS FLOAT64 ) AS embed_block_count_total
    FROM `httparchive.pages.2023_05_01_desktop`
  ),
  CountedEmbeds AS (
    SELECT 
      COUNT( DISTINCT url ) AS origins,
      AVG( embed_block_count_total ) as average_embed_block_count
    FROM
      WPEmbeds
    WHERE embed_block_count_total != 0

  )
SELECT * 
FROM CountedEmbeds
Last edited 5 months ago by adamsilverstein (previous) (diff)

#13 in reply to: ↑ 12 @joemcgill
5 months ago

Replying to adamsilverstein:

After discussing this further with @flixos90 I re-tested and it turns out the regular content filters do work correctly for oEmbed iframes. The issue with my tests is that I was only testing with a single or two oEmbeds and wp_omit_loading_attr_threshold is now set at 3. Oof!

Querying httparchive (query below), I found that for WordPress sites that use an oEmbed block, the average number of oEmbeds is below 3

One consideration here (if I'm not mistaken) is that the wp_omit_loading_attr_threshold() is cumulative — meaning it will be the sum of all images AND iframes found in the page, not just the number of embeds. This means that WordPress will begin to apply lazy loading to the fourth object on the page regardless. Perhaps we should treat the threshold for iframes differently from images, but it would good to run some numbers on the number of embeds that end up being counted as a part of the LCP value to see if it would make a significant difference.

#14 @adamsilverstein
5 months ago

Perhaps we should treat the threshold for iframes differently from images, but it would good to run some numbers on the number of embeds that end up being counted as a part of the LCP value to see if it would make a significant difference.

Intuitively I feel this is likely the case and I agree it would be helpful to some solid numbers to help inform an improved approach.

@adamsilverstein commented on PR #4822:


5 months ago
#15

I am questioning whether that is an appropriate fix to the problem. Embeds should receive their loading attributes the same way that any other items in the_content do, as part of wp_filter_content_tags(), relying on the new wp_get_loading_optimization_attributes().

Agreed, after further testing filter in the_content is being applied; I commented on the trac ticket - I do think we should reconsider our approach to iframe lazy loading to see if we can improve things for most users, they likely need slightly different treatment than images do.

#16 @adamsilverstein
4 months ago

  • Summary changed from Lazy load oEmbed iframes to Improve oEmbed iframe lazy loading

#17 @adamsilverstein
3 months ago

  • Keywords needs-patch added; needs-testing has-patch has-unit-tests removed

#18 @spacedmonkey
3 months ago

Is there any issue around tracking. What if you were using an iframe to do tracking? Or a impression count on a video? I am not sure it is a blocker, but this change needs to be called out in the dev notes.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


2 months ago

#20 @spacedmonkey
2 months ago

  • Milestone changed from 6.4 to 6.5

This ticket was discussed in this week's performance bug scrub. As this hasn't got an update in 3 months, this feels unlikely to get into a state to commit.

@adamsilverstein if you can get a patch together then you move back to milestone.

#21 @adamsilverstein
2 weeks ago

  • Summary changed from Improve oEmbed iframe lazy loading to Improve oEmbed lazy loading

#22 @adamsilverstein
8 days ago

I started working on this as an experimental modules for the Performance Lab plugin in https://github.com/WordPress/performance/pull/886

This module attempts to optimize oEmbeds using two techniques:

  • For oEmbeds delivered as iframes, ensure the loading=lazy attribute is present
  • For oEmbeds built using script tags:
    • move the script src to a data attribute
    • add a page level IntersectionObserver that sets script src before the user scrolls to the oEmbed (using a 500px margin)

The impact of the change would of course depend of which and how many oEmbeds a site used, however I think this is worth exploring a bit further

Note: See TracTickets for help on using tickets.