WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 13 days ago

#44632 accepted enhancement

Properly enqueue wp-embed

Reported by: alpipego Owned by: westonruter
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch has-unit-tests
Focuses: javascript, performance Cc:

Description

The wp-embed JS gets enqueued on wp_head instead of wp_enqueue_scripts source:trunk/src/wp-includes/default-filters.php#L532; thus it gets printed in wp_print_footer_scripts although it's not registered that way.

The script should be properly registered with 'in_footer' = true and enqueued on wp_enqueue_scripts.

I'll be happy to provide the patch, if desired.

Attachments (2)

44632.diff (561 bytes) - added by ocean90 6 months ago.
44632.2.diff (1.6 KB) - added by westonruter 2 weeks ago.

Download all attachments as: .zip

Change History (19)

#1 @swissspidy
3 years ago

I can't remember off the top of my head why we added it this way in the feature plugin...

If we change this behavior we have to make sure it doesn't break anything.

@ocean90
6 months ago

#2 @ocean90
6 months ago

  • Keywords has-patch added

We probably can't change how the script is enqueued since it would break any usage of remove_action( 'wp_head', 'wp_oembed_add_host_js' );. But I don't see any harm in setting the in_footer argument as done in 44632.diff.

Possible workaround:

<?php
/**
 * Improves handling of `wp-embed` script enqueue.
 *
 * By default the script is enqueued on all pages, registered on `wp_head` with
 * priority 10. This prevents third-party plugins to handle this properly which
 * expect all scripts/styles to be registered on `wp_enqueue_scripts`, also hooked
 * into `wp_head` with priority 1.
 *
 * This function unregisters the default action and registers it via `wp_enqueue_scripts`.
 * It also makes sure to enqueue the script in the footer which it isn't by default.
 *
 * @link https://core.trac.wordpress.org/ticket/44632
 */
function trac_44632_wp_embed(): void {
        if ( ! has_action( 'wp_head', 'wp_oembed_add_host_js' ) ) {
                return;
        };

        remove_action( 'wp_head', 'wp_oembed_add_host_js' );

        wp_scripts()->add_data( 'wp-embed', 'group', 1 );
        add_action( 'wp_enqueue_scripts', 'wp_oembed_add_host_js' );
}
add_action( 'wp_head', 'trac_44632_wp_embed', 0 );

@westonruter
2 weeks ago

#3 follow-up: @westonruter
2 weeks ago

Related to this is #44306 in which someone reports the messages from the iframes are being sent too early when the script is being printed in the footer. But the footer is where this should be printed regardless (or at least not in the head).

Nevertheless, I also just noticed that the wp-embed script is printed on every page unconditionally. Isn't this incorrect and a negative impact on page performance? Shouldn't it only ever be printed if there is an oEmbed on the page which actually outputs an iframe with the wp-embedded-content class?

Secondly, this script seems it would benefit from the defer attribute being added to it. But the race condition described in #44306 should be figured out. This may entail inlining the script before the first iframe.wp-embedded-content instead. So what about doing just that? What about filtering embed_oembed_html to prepend an inlined wp-embed script before post embeds? See 44632.2.diff.

#4 follow-up: @westonruter
2 weeks ago

I see there is also already such inlining being done in get_post_embed_html() for internal post embeds. Perhaps that just needs to be done for all post embeds, including external ones.

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


2 weeks ago

  • Keywords has-unit-tests added
  • Prevent loading wp-embed script unconditionally on every page (in the footer) in favor of conditionally printing the script inline before post embeds. By printing the script inline before the post embed, the race condition reported in Core-44306 should also be addressed.

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

#6 @westonruter
2 weeks ago

Pull request implements the inlining of wp-embed.js for both oEmbed post embeds and the "HTML Embed" markup provided for users to copy. It also fixes what seems to be bug in the latter, where the markup didn't include the secret on the blockquote and iframe meaning the blockquote would not be hidden.

#7 @westonruter
2 weeks ago

  • Focuses javascript performance added
  • Milestone changed from Awaiting Review to 5.9

#8 @westonruter
2 weeks ago

  • Owner set to westonruter
  • Status changed from new to accepted

#9 in reply to: ↑ 3 ; follow-up: @swissspidy
2 weeks ago

Replying to westonruter:

Nevertheless, I also just noticed that the wp-embed script is printed on every page unconditionally. Isn't this incorrect and a negative impact on page performance? Shouldn't it only ever be printed if there is an oEmbed on the page which actually outputs an iframe with the wp-embedded-content class?

I would have to do some digging to recall why we originally added the script in the header. But because it's printed in the header, it needs to be done unconditionally, as it's too early to know whether it's actually going to be needed or not.

Similar to the wp-emoji JS.

Secondly, this script seems it would benefit from the defer attribute being added to it. But the race condition described in #44306 should be figured out. This may entail inlining the script before the first iframe.wp-embedded-content instead. So what about doing just that? What about filtering embed_oembed_html to prepend an inlined wp-embed script before post embeds? See 44632.2.diff.

AFAIK in such a situation the browser will pause parsing the HTML until the script has been loaded, so that's not ideal.

(EDIT: I see now you've inlined the script for this reason)

Just printing in the footer sounds reasonable to me.

To solve the race condition, I think wp-embed-template.js should

  • not send the message on load if the secret is missing
  • send the message as soon as the secret has been added (which means wp-embed.js has been loaded)
Last edited 2 weeks ago by swissspidy (previous) (diff)

#10 in reply to: ↑ 4 @swissspidy
2 weeks ago

Replying to westonruter:

I see there is also already such inlining being done in get_post_embed_html() for internal post embeds. Perhaps that just needs to be done for all post embeds, including external ones.

That's not what the function does. get_post_embed_html() is for providing the embed code via the oEmbed restpoint, in case other platforms want to embed a blog post.

#11 @prbot
2 weeks ago

swissspidy commented on PR #1745:

  • Prevent loading wp-embed script unconditionally on every page (in the footer) in favor of conditionally printing the script inline before post embeds. By printing the script inline before the post embed, the race condition reported in Core-44306 should also be addressed.

This causes lots of duplicated inline scripts on the page, doesn't it? That doesn't sound ideal.

Why can't this be 1 external script in the footer? With wp-embed-template.js being updated to only send the postMessage once it has a secret.

  • Add missing secret to blockquote and iframe in embed code provided for users to copy. This ensures that the inlined wp-embed.js script can hide the blockquote when the iframe loads.

That's a weird one, I thought https://core.trac.wordpress.org/ticket/34451 solved that years ago, but seems like it didn't.

So it sounds like you're testing this on non-WP sites? Otherwise I don't see why this is necessary 🤔

In WP, wp_filter_oembed_result adds these attributes already

#12 @prbot
2 weeks ago

westonruter commented on PR #1745:

This causes lots of duplicated inline scripts on the page, doesn't it? That doesn't sound ideal.

While not ideal, this is standard for embeds. When embedding tweets for example, every single copy will include the same script: <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>. At least here the script is inlined and subsequent copies would short-circuit:

https://github.com/WordPress/wordpress-develop/blob/2382765afa36e10bf3c74420024ad4e85763a47c/src/js/_enqueues/wp/embed.js#L26-L28

Why can't this be 1 external script in the footer? With wp-embed-template.js being updated to only send the postMessage once it has a secret.

I think for the reason outlined in Core-44306. Namely, if there is a post embed and the iframe for that post embed loads before the page finishes loading and before the wp-embed.js script executes, then it could be that the iframe'd post embed window could call postMessage _before_ wp.receiveEmbedMessage() has been added as the message event listener, and then it wouldn't get sized. So the message event should be added before the iframe occurs on the page to be the most robust.

That's a weird one, I thought https://core.trac.wordpress.org/ticket/34451 solved that years ago, but seems like it didn't.

So it sounds like you're testing this on non-WP sites? Otherwise I don't see why this is necessary 🤔

Yes, for example: https://wp-embed-script-loading.glitch.me/#current-html-embed-code

https://i0.wp.com/user-images.githubusercontent.com/134745/136827788-4fe9b032-d6a3-447c-8ff3-777f1f9fbda7.png

I copied the HTML Embed code to embed on another site, and you can see that the blockquote doesn't get hidden as it does when it is embedded in WordPress:

https://i0.wp.com/user-images.githubusercontent.com/134745/136827848-d24a92b8-138e-4947-ab3a-50297bc6bb88.png

In WP, wp_filter_oembed_result adds these attributes already

Humm. But wp_filter_oembed_result isn't running on the embed_html filter. So yeah, it seems the non-WP embedding is the problem I was trying to fix.

#13 in reply to: ↑ 9 @westonruter
2 weeks ago

Replying to swissspidy:

I would have to do some digging to recall why we originally added the script in the header. But because it's printed in the header, it needs to be done unconditionally, as it's too early to know whether it's actually going to be needed or not.

I think the reason may be related to #44306, where the message event should be added before iframes load.

To get around this, the script could be printed right before the post embeds appear on the page. You're right that this is not ideal since it does block rendering, but better multiple blocking inline scripts than a one external one?

To solve the race condition, I think wp-embed-template.js should

  • not send the message on load if the secret is missing
  • send the message as soon as the secret has been added (which means wp-embed.js has been loaded)

But wp_filter_oembed_result is adding the secret to the initial HTML, so it doesn't seem that watching for the secret location hash will work since it would always be present as I see?

Just printing in the footer sounds reasonable to me.

If the race condition message passing situation can be figured out, then great. But I think this should also conditionally print the script in the footer if there was actually any instance of a post embed printed on the page.

#14 follow-up: @swissspidy
2 weeks ago

But wp_filter_oembed_result is adding the secret to the initial HTML, so it doesn't seem that watching for the secret location hash will work since it would always be present as I see?

Ah, right.

Before I typed that, I actually wanted to propose another idea:

The host script (wp-embed.js) could send a read postMessage to all the embeds on the page once it's loaded. So the individual embeds could respond to that by sending the height message in reply.

So the flow would be:

  1. Host site renders all embeds
  2. All iframes send their height message on load, just like today
  3. wp-embed.js finishes loading, sends ready message to all iframes
  4. All iframes send their height message in response to the ready message

Of course, if the script somehow would be loaded already earlier, the flow would be simpler:

  1. Host site renders all embeds
  2. wp-embed.js finishes loading, sends ready message to all iframes
  3. If an iframe receives the ready message but isn't loaded yet, it doesn't do anything
  4. All iframes send their height message on load, ignoring the ready which was received

But I think this should also conditionally print the script in the footer if there was actually any instance of a post embed printed on the page.

Definitely.

#15 @prbot
2 weeks ago

swissspidy commented on PR #1745:

This causes lots of duplicated inline scripts on the page, doesn't it? That doesn't sound ideal.

While not ideal, this is standard for embeds.

Right. For non-WP it makes sense like this, but we should keep avoiding this in WP context like we already do today.

I think for the reason outlined in Core-44306. [...]

Let's try to nail the race condition fix before attempting to improve loading order. A good opportunity to hit two birds with one stone.

#16 in reply to: ↑ 14 @westonruter
13 days ago

Replying to swissspidy:

The host script (wp-embed.js) could send a read postMessage to all the embeds on the page once it's loaded. So the individual embeds could respond to that by sending the height message in reply.

So it sounds like (1) iframed windows should send the height as soon as they load, (2) they should also send the height when the parent page requests it (in addition to when the page is resized):

  • src/js/_enqueues/wp/embed.js

    a b  
    104104                for ( i = 0; i < iframes.length; i++ ) {
    105105                        source = iframes[ i ];
    106106
    107                         if ( ! source.getAttribute( 'data-secret' ) ) {
     107                        secret = source.getAttribute( 'data-secret' );
     108                        if ( ! secret ) {
    108109                                /* Add secret to iframe */
    109110                                secret = Math.random().toString( 36 ).substr( 2, 10 );
    110111                                source.src += '#?secret=' + secret;
     
    117118                                iframeClone.removeAttribute( 'security' );
    118119                                source.parentNode.replaceChild( iframeClone, source );
    119120                        }
     121
     122                        source.contentWindow.postMessage( {
     123                                message: 'ready',
     124                                secret: secret
     125                        } );
    120126                }
    121127        }

And then wp-embed-template.js would to add a message event listener to listen for ready.

If the height didn't change, then the message should no-op, doing something like this:

  • src/js/_enqueues/wp/embed.js

    a b  
    5757                                continue;
    5858                        }
    5959
    60                         source.removeAttribute( 'style' );
    61 
    6260                        /* Resize the iframe on request. */
    63                         if ( 'height' === data.message ) {
     61                        if ( 'height' === data.message && data.value !== source.height ) {
     62                                source.removeAttribute( 'style' );
     63
    6464                                height = parseInt( data.value, 10 );
    6565                                if ( height > 1000 ) {
    6666                                        height = 1000;

The new ready message being sent from the host to the post embed would depend, naturally, on both WordPress sites being updated to the latest version of WordPress.

Is this what you have in mind?

#17 @swissspidy
13 days ago

Yup, precisely.

Both sites needing to be updated is not ideal short term, but this new ready message makes this more stable long term.

Note: See TracTickets for help on using tickets.