Make WordPress Core

Opened 13 months ago

Closed 3 months ago

Last modified 3 months ago

#58773 closed defect (bug) (fixed)

Improve oEmbed lazy loading

Reported by: adamsilverstein's profile adamsilverstein Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.6 Priority: normal
Severity: normal Version: 4.5
Component: Embeds Keywords: has-patch has-unit-tests
Focuses: accessibility, 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 13 months ago.
before-change.jpg (117.7 KB) - added by adamsilverstein 13 months ago.
lazy.jpg (197.5 KB) - added by adamsilverstein 13 months ago.
wpembed.jpg (191.4 KB) - added by adamsilverstein 13 months ago.
58773.2.diff (3.2 KB) - added by adamsilverstein 13 months ago.
58773.3.diff (2.6 KB) - added by adamsilverstein 13 months ago.

Download all attachments as: .zip

Change History (37)

#1 @adamsilverstein
13 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
13 months ago

  • Milestone changed from Awaiting Review to 6.4

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


13 months ago
#3

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

#4 @adamsilverstein
13 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
13 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
13 months ago

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

#7 @adamsilverstein
13 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
13 months ago

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

#9 @adamsilverstein
13 months ago

  • Description modified (diff)

#10 @adamsilverstein
13 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
13 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
13 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 13 months ago by adamsilverstein (previous) (diff)

#13 in reply to: ↑ 12 @joemcgill
13 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
13 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:


13 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
12 months ago

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

#17 @adamsilverstein
11 months ago

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

#18 @spacedmonkey
11 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.


10 months ago

#20 @spacedmonkey
10 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
8 months ago

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

#22 @adamsilverstein
8 months 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

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


7 months ago

#24 @mukesh27
7 months ago

  • Milestone changed from 6.5 to Future Release

This ticket was discussed during the Performance bug scrub. Since this feature has been thoroughly tested in the Performance lab module, it has been decided to move it to the Future Release category.

Props @joemcgill @flixos90

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


3 months ago
#25

  • Keywords has-patch has-unit-tests added; needs-patch removed
  • Use opacity:0 and pointer-events:none instead of clip: rect(1px, 1px, 1px, 1px); to hide the iframe while it is loading. When the iframe is lazy-loaded, Chromium does not reliably load the iframe when it is clipped but it does load it when it is made transparent.
  • Update wp_iframe_tag_add_loading_attr() to no longer short-circuit when the iframe is a post embed.

Trac ticket: https://core.trac.wordpress.org/ticket/58773

#26 @westonruter
3 months ago

  • Milestone changed from Future Release to 6.6

I think I have a solution for fixing lazy-loading of post embed iframes. See https://github.com/WordPress/performance/pull/1192

I found that Chrome didn't reliably load the lazy-loaded post iframe when it had the clip: rect(1px, 1px, 1px, 1px) style present. Strangely, the iframe would load when navigating to the page but it wouldn't load when reloading the page. However, if the clip is replaced with opacity:0; pointer-events:none; then it works as expected. I tested in Chrome, Safari, and Firefox. I've opened a PR to apply this change to core as well: https://github.com/WordPress/wordpress-develop/pull/6474

When testing this, remember to insert three images into the post content prior to the post embed. This I had to first insert 3 images into the post content followed by the post embeds. This is because wp_omit_loading_attr_threshold() returns 3 so that lazy-loading is only applied to images and iframes after the 3rd.

#27 @westonruter
3 months ago

  • Focuses accessibility added

Adding a11y focus since this switches away from clip.

#28 @westonruter
3 months ago

Update: After chatting with @joedolson, I found out that visibility:hidden works just as well if not better than opacity:0. Not only does the lazy-loaded iframe load consistently across Chrome, Safari, and Firefox but there is also an actual accessibility benefit to using visibility:hidden over clip: the entire iframe is made non-interactive. With clip a user tabbing through links on a page will also encounter tab stops inside the clipped iframe, which is particularly confusing to sighted users navigating by keyboard as focus seems to get lost on the page for several tab presses. In contrast, when an iframe is hidden with visibility then no tab stops occur in the iframe. The PR has been updated to apply this change.

Side note: I also discovered that clip is deprecated!

Last edited 3 months ago by westonruter (previous) (diff)

#29 @westonruter
3 months ago

  • Version set to 4.5

#30 @westonruter
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58143:

Embeds: Enable lazy-loading of post embeds and fix keyboard a11y for hidden iframes.

Chrome unreliably loads a lazy-loaded iframe when it is hidden using clip: rect(1px, 1px, 1px, 1px). Instead of using clip, a lazy-loaded iframe can also be hidden with visibility:hidden which results in it loading not only in Chrome but all other browsers. With this change applied, the hard-coded check to prevent lazy-loading post embeds is now removed. An added benefit to using visibility:hidden is that the entire iframe in this case is not interactable, meaning that users navigating the document with the keyboard will not unexpectedly encounter tab stops inside of the hidden iframe, as can happen now with clip when the JS fails to reveal the loaded iframe. Note also that the clip property is deprecated.

Lastly, when such a post embed iframe is rendered in an RSS feed, the style attribute is now removed using the HTML Tag Processor as opposed to using string replacement.

Fixes #58773.
Props westonruter, joemcgill, swissspidy, joedolson, adamsilverstein.

@westonruter commented on PR #6474:


3 months ago
#31

Committed to trunk in r58143 (b4889e4)

Note: See TracTickets for help on using tickets.