#64178 closed defect (bug) (fixed)
Increased styles_inline_size_limit pushes down oEmbed discovery tags
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 6.9 | Priority: | normal |
| Severity: | normal | Version: | 6.9 |
| Component: | Embeds | Keywords: | has-patch needs-unit-tests |
| Focuses: | performance | Cc: |
Description
A consequence of #63018 seems to be that it pushes down oEmbed discovery meta tags. WordPress only fetches the first 150 KB of a page to scan for those. So now if you try to embed a page such as https://make.wordpress.org/core/2021/10/12/proposal-for-a-performance-team/, the tags are not be within those 150 KB anymore and embedding doesn't work.
Changing priority of wp_oembed_add_discovery_links might help. Or fetching more than 150 KB, but that seems wasteful.
Change History (14)
This ticket was mentioned in Slack in #core by swissspidy. View the logs.
6 weeks ago
#4
@
5 weeks ago
Changing the priority is going to be complicated because there are many instances of ecosystem code removing the action:
<?php remove_action( 'wp_head', 'wp_oembed_add_discovery_links' );
See GitHub Search (1.7k files) and WPDirectory (145 plugins).
This ticket was mentioned in PR #10449 on WordPress/wordpress-develop by @westonruter.
5 weeks ago
#5
- Keywords has-patch added
This addresses an issue whereby if there is a lot of markup printed at wp_head (e.g. inline styles from Core-63018), WordPress sites looking for the oEmbed discovery links will fail to find them because they only read the first 150KB of the HTML response, by default:
Note that scripts and styles are printed at priorities 8 and 9:
In order to ensure the oEmbed links remain discoverable, it seems prudent to print them early, specifically right after other similar links are printed, namely feed_links() and feed_links_extra().
It is not so simple as changing the action priority from 10 to 4:
This is because there are many plugins which remove the oEmbed discovery links via:
remove_action( 'wp_head', 'wp_oembed_add_discovery_links' );
See GitHub Search (1.7k files) and WPDirectory (145 plugins).
Aside: The better way to do this would be to do:
add_filter( 'oembed_discovery_links', '__return_empty_string' );
In order to maintain backwards-compatibility with the ecosystem which unhooks the action in this way, it is necessary to add a _second instance_ of wp_oembed_add_discovery_links running at wp_head with the earlier priority of 4. Inside of wp_oembed_add_discovery_links(), when it runs first at priority 4, it can check if it has been unhooked at priority 10, and if so, short-circuit. Otherwise, it can continue with printing the oEmbed discovery links, but also preventing them from being duplicated by itself doing:
remove_action( 'wp_head', 'wp_oembed_add_discovery_links' );
In order to facilitate this, it is necessary to extend the has_action()/has_filter() functions with a new $priority parameter to be able to check whether a given callback is added at a specific priority. This is something I've missed in the past.
A similar approach to backwards-compatibility has been done recently, for with emoji in example r60910. The difference there is that a new function was introduced, whereas here the function remains the same.
- [ ] Add tests
Trac ticket: https://core.trac.wordpress.org/ticket/64178
#6
@
5 weeks ago
I have a solution I like for changing the priority: https://github.com/WordPress/wordpress-develop/pull/10449
However, I think it may make sense to back off of increasing the styles_inline_size_limit as well. Instead of bumping from 20K to 50K, perhaps it would make more sense to meet in the middle at 35K or 40K.
Note the graphs showing the LCP-TTFB for Fast 4G (raw data) and broadband (raw data) from #63018. In the two graphs, the uncached curve starts to flatten out at 40K:
Reducing the inline limit wouldn't solve the problem for the page in question, however, since the oEmbed discover links appear at byte 178,831. So reducing reducing 10K-15K of inline CSS would still leave them above the 150K threshold for discovery.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
5 weeks ago
#10
@
5 weeks ago
I've opened #64186 specifically to document the addition of the $priority parameter to has_action()/has_filter().
@westonruter commented on PR #10449:
5 weeks ago
#11
I've opened https://github.com/WordPress/wordpress-develop/pull/10456 to introduce the has_filter()/has_action() change in isolation. Tests have been added there.
#14
@
5 weeks ago
As of 6.9-beta3, the amount of inlined styles (aside from global-styles-inline-css), amount to 49K. This is still above the 40K limit. Maybe this indicates a bug in the wp_maybe_inline_styles() logic for determining when to cut off inlining.
In any case, the issue with oEmbed discovery tags is now resolved, as they are printed before the styles.
There's quite a bit more inline CSS on that page than expected, namely 98,691 bytes:
Table derived via:
Of these styles, ~57KB appear to have been automatically inlined via
wp_maybe_inline_styles(). This is larger than the 50,000styles_inline_size_limit, however.There is a very large global styles inline style at almost 40KB.
I'm also noticing that there are stylesheets being enqueued for blocks which don't actually occur on the page (e.g.
wporg-google-map). This would seem to indicate on-demand enqueues aren't working as expected per #64099. This may be due to how the P2 theme works, however, in that it has some client-side rendering logic which may necessitate adding more CSS up front than is actually needed, just in case it is needed later?