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.

Sign up for GitHub

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

Webfonts: Preload fonts in the head to stop a flash of unstyled content #39391

Closed
wants to merge 6 commits into from
Closed

Webfonts: Preload fonts in the head to stop a flash of unstyled content #39391

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a21788
Webfonts: Preload fonts in the head to stop FOUC
scruffian Mar 11, 2022
16ac0d6
prevent security issues by sanitizing input from theme.json
scruffian Mar 11, 2022
529103a
prevent security issues by sanitizing input from theme.json
scruffian Mar 11, 2022
9f5fb53
allow themes to specify which fonts to preload
scruffian Mar 11, 2022
4aab001
Update lib/compat/wordpress-6.0/class-wp-webfonts.php
scruffian Mar 15, 2022
8dcf2b0
Update lib/compat/wordpress-6.0/class-wp-webfonts.php
scruffian Mar 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions lib/compat/wordpress-6.0/class-wp-webfonts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ class WP_Webfonts {
*/
private $stylesheet_handle = '';

/**
* Allowed file types
*/
private static $allowed_types = array( 'woff', 'woff2', 'ttf', 'eot', 'otf' );

/**
* Init.
*/
Expand All @@ -55,6 +60,9 @@ public function init() {

// Enqueue webfonts in the block editor.
add_action( 'admin_init', array( $this, 'generate_and_enqueue_editor_styles' ) );

// Preload the font files.
add_action( 'wp_head', array( $this, 'preload_webfonts' ) );
}

/**
Expand Down Expand Up @@ -165,6 +173,7 @@ public function validate_font( $font ) {
'font-feature-settings',
'font-variation-settings',
'line-gap-override',
'preload',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since preload is not a valid CSS prop, I think it would be best if we added it to the // exceptions section a few lines below, along with provider.
We'd also need to add a check in the local-provider class to make sure that no CSS is generated for this:

gutenberg/lib/compat/wordpress-6.0/class-wp-webfonts-provider-local.php

Lines 193 to 197 in 9f5fb53

// Skip "provider", since it's for internal API use,
// and not a valid CSS property.
if ( 'provider' === $key ) {
continue;
}

'size-adjust',
'src',
'unicode-range',
Expand Down Expand Up @@ -291,4 +300,24 @@ public function generate_styles() {

return $styles;
}

/**
* Preload webfonts to improve performance
*
* @return void
*/
public function preload_webfonts() {
foreach ( $this->get_fonts() as $font ) {
if ( ! empty( $font['preload'] ) && $font['preload'] ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is deeply nested causing the code to shift to the right. This can decrease readability. I'd suggest either breaking it up into private methods -or- converting this guard into a skip clause like this:

Suggested change
if ( ! empty( $font['preload'] ) && $font['preload'] ) {
// Skip this font if it's not configured for preloading.
if ( empty( $font['preload'] ) ) {
continue;
}

foreach( $font['src'] as $src ) {
$src_as_array = explode( '.', $src );
$file_type = end( $src_as_array );
Comment on lines +313 to +314
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micro-optimization suggestion:

Suggested change
$src_as_array = explode( '.', $src );
$file_type = end( $src_as_array );
$file_type_pos = strrpos( $src, '.' ) + 1;
$file_type = substr( $src, $file_type_pos );

Why? explode creates an array which consumes a bit more memory and time. To see it in action, here's a benchmark running on PHP 7.4 over 1 million cycles https://3v4l.org/EnOB1#v7.4.28.

Results:

Implementation Average μs
explode and end 0.2440 μs
strrpos and substr 0.0959 μs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW This code is not needed if the preload functionality instead grabs the font srcs from the provider's WP_Webfonts_Provider_Local::order_src() as suggested here.

if ( ! in_array( $file_type, self::$allowed_types ) ) {
$file_type = '';
}
echo '<link rel="preload" href="' . $src . '" as="font" type="font/' . $file_type . '" crossorigin />';
Copy link
Member

@aristath aristath Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WP_Webfonts_Provider_Local::order_src() method returns an array of URLs along with their file/mime types. Perhaps we should use that? It would make sense, especially if we move the implementation to the WP_Webfonts_Provider_Local object (where this should be IMO)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WP_Webfonts_Provider_Local::order_src() method returns an array of URLs along with their file/mime types. Perhaps we should use that?

Thinking about it, yes, this is a better approach to avoid redundancy.

I agree that this method is a better fit for the local provider (WP_Webfonts_Provider_Local) than here. In the future when Font Factory providers are bundled, this code will need changes as src is specific to locally hosted fonts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is rendering the HTML, the variables require escaping for security:

Suggested change
echo '<link rel="preload" href="' . $src . '" as="font" type="font/' . $file_type . '" crossorigin />';
printf(
'<link rel="preload" href="%s" as="font" type="font/%s" crossorigin />',
esc_url( $src ),
esc_attr( $file_type )
);

}
}
}
}
}