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.

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

Improve page experience for fonts in core themes via preloading, preconnecting, and using font-display:optional #6036

Closed
milindmore22 opened this issue Apr 2, 2021 · 8 comments · Fixed by #6674
Assignees
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. CSS Groomed P1 Medium priority Performance WS:Core Work stream for Plugin core
Milestone

Comments

@milindmore22
Copy link
Collaborator

Bug Description

Improve page experience of core themes by preloading and preconnecting fonts, as suggested by Pixi let's preload core theme fonts which are loaded through URL also use font-display property suggested font-display: optional

As we discussed core themes are still widely used, and also used as Reader Mode themes, so let's add these small improvements.

Ref: ampproject/amp.dev#5531

Core themes and fonts Tested

Themes Fonts Source
twentyten None
twentyeleven None
twentytwelve Open Sans Google Fonts
twentythirteen Source Sans Pro, Bitter, genericons Google Fonts, Theme
twentyfourteen Lato, Genericons Google Fonts, Theme
twentyfifteen Noto Serif, Noto Sans, Inconsolata Google Fonts, Theme
twentysixteen Merriweather, Montserrat, Inconsolata Google Fonts
twentyseventeen Libre Franklin Google Fonts
twentynineteen NonBreakingSpaceOverride Theme
twentytwenty Inter var, NonBreakingSpaceOverride Theme, AMP Plugin
twentytwentyone None

Expected Behaviour

preconnect and dns-prefetch
<link href=https://fonts.gstatic.com rel="dns-prefetch preconnect" crossorigin>

preload
<link rel="preload" href="https://mywebsite.com/wp-content/themes/twentytwenty/assets/fonts/inter/Inter-italic-var.woff2" as="font" crossorigin>

Steps to reproduce

  1. Go to Themes
  2. Activate any Core theme
  3. Chack AMP page on Page Experiance
  4. Check Preload Fonts

Screenshots

font-display:optional Preload
image image

Additional context

  • WordPress version: 5.7
  • Plugin version: 2.0.11
  • Gutenberg plugin version (if applicable):
  • AMP plugin template mode: Standard Mode
  • PHP version: 7.4.10
  • OS: MacOS Catalina
  • Browser: Chrome
  • Device: Mac Air

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@milindmore22 milindmore22 added the Bug Something isn't working label Apr 2, 2021
@westonruter
Copy link
Member

For Genericons, we definitely will need to use font-display:block because this is an icon font and with swap we'd get unknown glyph boxes rendered momentarily and for optional we'd get permanent boxes on the first visit. The performance problems of using an icon font are surely the reason SVG was used in Twenty Seventeen.

In the table, Genericons should be added to Twenty Fifteen and Twenty Sixteen. Note there is a NoIconFontIsUsed linter check which doesn't yet include Genericons, but it should. Once that happens, we'll get linting errors for these themes. Not sure how we'd be able to fix that, as we'd need to somehow convert Genericons into SVGs and rewrite any stylesheet references to use them.

For the data: URL fonts that we are externalizing, the logic is found here:

// Attempt to transform data: URLs in src properties to be external file URLs.
$converted_count = 0;
foreach ( $src_properties as $src_property ) {
$value = $src_property->getValue();
if ( ! ( $value instanceof RuleValueList ) ) {
continue;
}
/*
* The CSS Parser parses a src such as:
*
* url(data:application/font-woff;...) format('woff'),
* url('Genericons.ttf') format('truetype'),
* url('Genericons.svg#genericonsregular') format('svg')
*
* As a list of components consisting of:
*
* URL,
* RuleValueList( CSSFunction, URL ),
* RuleValueList( CSSFunction, URL ),
* CSSFunction
*
* Clearly the components here are not logically grouped. So the first step is to fix the order.
*/
$sources = [];
foreach ( $value->getListComponents() as $component ) {
if ( $component instanceof RuleValueList ) {
$subcomponents = $component->getListComponents();
$subcomponent = array_shift( $subcomponents );
if ( $subcomponent ) {
if ( empty( $sources ) ) {
$sources[] = [ $subcomponent ];
} else {
$sources[ count( $sources ) - 1 ][] = $subcomponent;
}
}
foreach ( $subcomponents as $subcomponent ) {
$sources[] = [ $subcomponent ];
}
} elseif ( empty( $sources ) ) {
$sources[] = [ $component ];
} else {
$sources[ count( $sources ) - 1 ][] = $component;
}
}
/**
* Source file URL list.
*
* @var string[] $source_file_urls
*/
$source_file_urls = [];
/**
* Source data URL collection.
*
* @var URL[] $source_data_url_objects
*/
$source_data_url_objects = [];
foreach ( $sources as $i => $source ) {
if ( $source[0] instanceof URL ) {
$value = $source[0]->getURL()->getString();
if ( 'data:' === substr( $value, 0, 5 ) ) {
$source_data_url_objects[ $i ] = $source[0];
} else {
$source_file_urls[ $i ] = $value;
}
}
}
// Convert data: URLs into regular URLs, assuming there will be a file present (e.g. woff fonts in core themes).
foreach ( $source_data_url_objects as $i => $data_url ) {
$mime_type = strtok( substr( $data_url->getURL()->getString(), 5 ), ';' );
if ( ! $mime_type ) {
continue;
}
$extension = preg_replace( ':.+/(.+-)?:', '', $mime_type );
$guessed_urls = [];
// Guess URLs based on any other font sources that are not using data: URLs (e.g. truetype fallback for inline woff2).
foreach ( $source_file_urls as $source_file_url ) {
$guessed_url = preg_replace(
':(?<=\.)\w+(\?.*)?(#.*)?$:', // Match the file extension in the URL.
$extension,
$source_file_url,
1,
$count
);
if ( 1 === $count ) {
$guessed_urls[] = $guessed_url;
}
}
/*
* Guess some font file URLs based on the font name in a fonts directory based on precedence of Twenty Nineteen.
* For example, the NonBreakingSpaceOverride woff2 font file is located at fonts/NonBreakingSpaceOverride.woff2.
*/
if ( $stylesheet_base_url && $font_basename ) {
$guessed_urls[] = $stylesheet_base_url . sprintf( 'fonts/%s.%s', $font_basename, $extension );
$guessed_urls[] = $stylesheet_base_url . sprintf( 'fonts/%s.%s', strtolower( $font_basename ), $extension );
}
// Find the font file that exists, and then replace the data: URL with the external URL for the font.
foreach ( $guessed_urls as $guessed_url ) {
$path = $this->get_validated_url_file_path( $guessed_url, [ 'woff', 'woff2', 'ttf', 'otf', 'svg' ] );
if ( ! is_wp_error( $path ) ) {
$data_url->getURL()->setString( $guessed_url );
$converted_count++;
continue 2;
}
}
// As fallback, look for fonts bundled with the AMP plugin.
$font_filename = sprintf( '%s.%s', strtolower( $font_basename ), $extension );
$bundled_fonts = [
'nonbreakingspaceoverride.woff',
'nonbreakingspaceoverride.woff2',
'genericons.woff',
];
if ( in_array( $font_filename, $bundled_fonts, true ) ) {
$data_url->getURL()->setString( plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename" );
$converted_count++;
}
} // End foreach $source_data_url_objects.
} // End foreach $src_properties.
/*
* If a data: URL has been replaced with an external file URL, then we add a font-display:swap to the @font-face
* rule if one isn't already present. This prevents FO
*
* If no font-display is already present, add font-display:swap since the font is now being loaded externally.
*/
if ( $converted_count && 0 === count( $ruleset->getRules( 'font-display' ) ) ) {
$font_display_rule = new Rule( 'font-display' );
$font_display_rule->setValue( 'swap' );
$ruleset->addRule( $font_display_rule );
}

This is where we'd need to add the link[rel=preload] elements and make the switch to optional:

if ( $converted_count && 0 === count( $ruleset->getRules( 'font-display' ) ) ) {
$font_display_rule = new Rule( 'font-display' );
$font_display_rule->setValue( 'swap' );
$ruleset->addRule( $font_display_rule );
}

For Google Fonts, the font-display tweak would need to be done here:

if ( $href !== $normalized_url ) {
$element->setAttribute( 'href', $normalized_url );
}
/*
* Make sure rel=preconnect link is present for Google Fonts stylesheet.
* Note that core themes normally do this already, per <https://core.trac.wordpress.org/ticket/37171>.
* But not always, per <https://core.trac.wordpress.org/ticket/44668>.
* This also ensures that other themes will get the preconnect link when
* they don't implement the resource hint.
*/
$needs_preconnect_link = (
'https://fonts.googleapis.com/' === substr( $normalized_url, 0, 29 )
&&
0 === $this->dom->xpath->query( '//link[ @rel = "preconnect" and @crossorigin and starts-with( @href, "https://fonts.gstatic.com" ) ]', $this->dom->head )->length
);
if ( $needs_preconnect_link ) {
$link = AMP_DOM_Utils::create_node(
$this->dom,
'link',
[
'rel' => 'preconnect',
'href' => 'https://fonts.gstatic.com/',
'crossorigin' => '',
]
);
$this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order.
}
return;

For Google Fonts, we'd be adding display=optional to the $normalized_url and update the href. Note that WordPress core as of https://core.trac.wordpress.org/ticket/47282 opted to actually go with display=fallback rather than display=swap. The FastGoogleFontsDisplay linter rule seems to be fine with swap, fallback, or optional. The Twenty Twenty theme is using “Inter var” and has a font-display:swap. It seems there is a newer fontDisplay check which is explicitly checking for optional.

@westonruter
Copy link
Member

@westonruter
Copy link
Member

As noted in #5678 (comment), let's only worry about maximizing the performance of the three most recent core themes. These are the ones bundled with core now, and they are the three that we'll always show as Reader themes (even if not installed).

@westonruter westonruter changed the title Improve page experience of core themes by preloading and preconnecting fonts Improve page experience for fonts in core themes via preloading, preconnecting, and using font-display:optional Apr 30, 2021
@westonruter
Copy link
Member

preconnect and dns-prefetch
<link href=https://fonts.gstatic.com rel="dns-prefetch preconnect" crossorigin>

I've filed an issue to address this separately: #6156.

@westonruter
Copy link
Member

For Google Fonts needing to use font-display:optional, I've also filed this upstream suggestion for an Optimizer transformer: ampproject/amp-toolbox#1229. If we have the right fallbacks in place for core themes, this transformer could be enabled by default.

@westonruter
Copy link
Member

I've also identified that core is adding preconnect resource hints for Google Fonts but is failing to also add dns-prefetch. Opened ticket with patch to fix that upstream: https://core.trac.wordpress.org/ticket/53122

@westonruter westonruter changed the title Improve page experience for fonts in core themes via preloading, preconnecting, and using font-display:optional Improve page experience for fonts in core themes via preloading, preconnecting, and using font-display:optional Jun 8, 2021
@westonruter westonruter added this to the v2.2 milestone Jun 8, 2021
@westonruter westonruter added the P1 Medium priority label Jun 18, 2021
@westonruter
Copy link
Member

One of the areas of optimization is to force the NonBreakingSpaceOverride font to have font-display:optional. But it seems this is already the case in the plugin. See WordPress/twentynineteen#585 and WordPress/gutenberg#11405.

@fellyph
Copy link
Collaborator

fellyph commented Dec 8, 2021

QA Passed:

Themes Fonts display preconnect
twentyten None
twentyeleven None
twentytwelve Open Sans fallback yes
twentythirteen Source Sans Pro, Bitter, genericons fallback yes
twentyfourteen Lato, Genericons fallback yes
twentyfifteen Noto Serif, Noto Sans, Inconsolata fallback yes
twentysixteen Merriweather, Montserrat, Inconsolata fallback yes
twentyseventeen Libre Franklin fallback yes
twentynineteen NonBreakingSpaceOverride
twentytwenty Inter var, NonBreakingSpaceOverride Theme, AMP Plugin
twentytwentyone None  

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. CSS Groomed P1 Medium priority Performance WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants