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 #6674

Merged
merged 22 commits into from
Nov 30, 2021

Conversation

bartoszgadomski
Copy link
Contributor

@bartoszgadomski bartoszgadomski commented Nov 2, 2021

Summary

Fixes #6036

I checked a test website against the Page Experience tool with three default WordPress themes (Twenty Twenty-One, Twenty Twenty and Twenty Nineteen) and I can't see any suggestions under "take action", nor any issues reported related to fonts (eg. this: https://user-images.githubusercontent.com/1132541/113437622-92497600-9404-11eb-922f-88e7ab72a890.png).

Screenshot 2021-11-2 at 09 12 55

Fonts in default WordPress themes

Since we focus on three recent themes (as per #6036 (comment)):

  1. Twenty Twenty-One theme uses system font stack, custom font files are not loaded:

  2. Twenty Twenty uses "Inter var" font which is loaded from the local source file + "NonBreakingSpaceOverride" loaded inline in css file through "data:application/font-woff2":

  3. Twenty Nineteen uses "Hoefler Text" with "NonBreakingSpaceOverride" font for body (loaded from the local source file):

Changes in this PR

  • force the @font-face block to have the font-display:optional if the font-display rule is missing and a data: URL has been replaced with an external file URL (use case: force the NonBreakingSpaceOverride font to have font-display:optional in "Twenty Twenty" and "Twenty Nineteen" themes)
  • preload each font files - example for TwentyTwenty theme:
<link rel="preload" as="font" href="http://example.org/wp-content/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff2" crossorigin="">
<link rel="preload" as="font" href="http://example.org/wp-content/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff" crossorigin="">
<link rel="preload" as="font" href="http://example.org/wp-content/themes/twentytwenty/assets/fonts/inter/Inter-upright-var.woff2" crossorigin="">
<link rel="preload" as="font" href="http://example.org/wp-content/themes/twentytwenty/assets/fonts/inter/Inter-italic-var.woff2" crossorigin="">

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@bartoszgadomski bartoszgadomski marked this pull request as ready for review November 3, 2021 18:54
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2021

Plugin builds for 96b748b are ready 🛎️!

@westonruter
Copy link
Member

westonruter commented Nov 3, 2021

  • preload each font files - example for TwentyTwenty theme:
<link rel="preload" as="font" href="http://example.org/wp-content/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff2" crossorigin="">
<link rel="preload" as="font" href="http://example.org/wp-content/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff" crossorigin="">

This poses a challenge. The browser should only be preloading the font format that it supports. As I understand, a browser here will preload the woff font files and the woff2 font files even though it only will use the woff2 ones. It seems the best practice is to only preload the latest format which is the first in the list (e.g. woff2). Ref: https://stackoverflow.com/a/53376159/93579

@westonruter
Copy link
Member

In the latest commits I pushed up, I've addressed this by only preloading the first data URL which is converted to an external file. What I didn't follow up on, however, is going ahead and preloading the first font file for each other @font-face rule, such as Inter-upright-var and Inter-italic-var. Half of me wonders if if those should be preloaded always. In looking at the the Twenty Twenty style.css it has:

@font-face {
	font-family: "Inter var";
	font-weight: 100 900; /* stylelint-disable-line font-weight-notation */
	font-style: normal;
	font-display: swap;
	src: url(./assets/fonts/inter/Inter-upright-var.woff2) format("woff2");
}

@font-face {
	font-family: "Inter var";
	font-weight: 100 900; /* stylelint-disable-line font-weight-notation */
	font-style: italic;
	font-display: swap;
	src: url(./assets/fonts/inter/Inter-italic-var.woff2) format("woff2");
}

So both of these still have font-display:swap currently, since the font-display:optional is only being added in the case of where there is no font-display already. But should these rather be font-display:optional to prevent there from being any layout shift? Isn't the use of swap instead of optional here what would be flagged by PSI as an improvement to improve CLS?

And in the case of NonBreakingSpaceOverride, this was originally inlined as a data: URL in which case it would have no swap period since there would be no network delay. But in actuality we're saying that in this specific case this font is not critical and we are making it optional. If that is the case, then shouldn't the fix for Twenty Nineteen be theme-specific or specifically target this font? As it stands right now, the changes are being made to any theme not just those in core.

I think we need to identify: (1) what we can do generally for all themes to improve font performance, and (2) what we can target specifically for the core themes based on the specific fonts they're using.

* 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 a data: URL has been replaced with an external file URL, then we add a font-display:optional to the @font-face
* rule if one isn't already present. This prevents a flash of unstyled text (FOUT).
Copy link
Member

Choose a reason for hiding this comment

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

Er, I think I may have meant FOIT here.

@westonruter
Copy link
Member

westonruter commented Nov 4, 2021

Also, in the case of improving performance of fonts that are already external (e.g. Inter), we should consider contributing patches to WordPress core so that all sites will benefit, not just those running the AMP plugin. This would be part of the new Performance initiative.

@westonruter
Copy link
Member

Just ran across this interesting anecdote as well: https://twitter.com/simonhearne/status/1456014210157191172

Granted, this is not relevant to AMP pages since the CSS is all inlined. But for contributing patches to WordPress core, this is indeed something to be wary of.

@bartoszgadomski
Copy link
Contributor Author

Thank you @westonruter for your comments!

(1) what we can do generally for all themes to improve font performance

If a given theme uses one of Google Fonts, what if we'd add this functionality to AMP plugin:

  1. check the font license, and if we're allowed allowed: download that font from Google Fonts and self-host the font files with its loading css code,
  2. if the license does not allow us to download the font files, then we could just download and self-host the loading css code,
  3. inline the loading css code for those fonts,

That way we can save extra request(s) and load fonts locally, + use font-display: optional.

So, what are the consequences of this change in browser caching behavior? The main change is that font CDNs like Google Fonts and Adobe TypeKit now strictly make your site slower. They used to help with cross-site caching, but that benefit is gone. Instead they add expensive cross-origin requests (and their DNS lookups, TLS negotiations, etc.) into the critical path of loading your website.

With that it is clear that we should self-host all fonts on our primary domain for maximum performance. With fonts this can sometimes be problematic for licensing reasons, etc. but there is a good middle ground: Instead of self-hosting the fonts, self-host the loading code. For all common Font CDNs (even TypeKit with some digging, they default to JS based loading) this is simply a CSS file. Just download that CSS file. It won't bite 😄. Or, if your font provider likes to sometimes change it, just fetch the CSS file once during your build process. Then inline the CSS file into your HTML and you completely eliminate the expensive cross-origin request from your critical path. While this approach still downloads the fonts themselves from the CDNs this doesn't hurt when you are using our friend font-display: optional.

Source: https://www.industrialempathy.com/posts/high-performance-web-font-loading/#fonts-and-cdns

@westonruter
Copy link
Member

Yeah, that is an interesting area to explore. However, since these three core themes don't use Google Fonts I think that should be explored in another issue.

There is also a problem currently with downloading font files from Google Fonts. It turns out that Google Fonts serves different versions of font files based on the user agent. So there can be fonts tuned specifically for macOS vs Windows vs iOS, for example. If the font files are downloaded, then platform-optimized versions won't be used.

That way we can save extra request(s) and load fonts locally, + use font-display: optional.

In the general theme case, we'd also need to go a step further and determine if the font file being downloaded is suitable for being optional in the first place. Some fonts may be required for a theme to render as desired if a suitable fallback is not generally available. So I don't think we can automate the injection of font-display:optional in the case for themes generally. What we can do, however, is take the specific examples of the themes for 2019, 2020, and 2021 and make a judgement if their fonts have suitable fallbacks in place. If so, we can make them optional instead of swap (or rather rather contribute those changes to core itself, but it may make sense to do both so we can experiment with the improvement).

@bartoszgadomski
Copy link
Contributor Author

To summarize:

(1) in this PR we'd implement these general improvements for any themes:

  • if a given font is loaded from file and with font-display:optional, preload it
  • if a given font is loaded from file and without font-display property, add font-display:optional + preload it

Extra exception for core themes that uses NonBreakingSpaceOverride font: add font-display:optional + preload it.
By "preload it" I mean preloading only the newest format: woff2

(2) Additionally, we should propose below changes to core themes - Twenty Nineteen and Twenty Twenty:

  • make the NonBreakingSpaceOverride font being loaded from file with font-display:optional rather than being inlined as a data:

@westonruter
Copy link
Member

(1) in this PR we'd implement these general improvements for any themes:

  • if a given font is loaded from file and with font-display:optional, preload it
  • if a given font is loaded from file and without font-display property, add font-display:optional + preload it

I'm not sure. If something had font-display:optional then this would seem to indicate that it is not important and thus needn't be preloaded. On the other hand, if if a font has font-display:swap or (worse) font-display:block then these would indicate the font is very important and thus it should be preloaded so that it can render as soon as possible, as otherwise the page is not going to be complete without it.

(2) Additionally, we should propose below changes to core themes - Twenty Nineteen and Twenty Twenty:

  • make the NonBreakingSpaceOverride font being loaded from file with font-display:optional rather than being inlined as a data:

Taking a look again at the latest core themes and the fonts they use:

Themes Fonts
twentynineteen NonBreakingSpaceOverride (font-display:auto)
twentytwenty Inter var (font-display:swap), NonBreakingSpaceOverride (font-display:auto)
twentytwentyone None

The NonBreakingSpaceOverride fonts lack any font-display since they are inlined as data: URLs. Nevertheless, I think we can decide that these can safely be made font-display:optional when we externalize them, but only this specific font (and for these specific themes?).

The impact of this font can be seen on this page: https://wpzurich.ch/were-on-tv/ (via WordPress/twentynineteen#657)

Without Font With Font
image image

Notice the wider spaces appearing after the links. These spaces are both &nbsp;. If the page renders with such wider spaces, it doesn't seem overly bad and many pages won't even have a non-breaking space to begin with. So it seems detrimental to include the font for all pages even if it won't be needed.

Then for Twenty Twenty which uses Inter var, it currently has font-display:swap. So we should decide if the layout shift entailed by swap is warranted. If not, then we should make it optional. If, however, it is warranted (i.e. the font is super important) then we should leave swap as-is and then add the link[rel=preload] so that the font can load as soon as possible to minimize the CLS and LCP.

Without Font With Font
image image
with-and-without-inter-var.mov

In my opinion, the text in the header is what uses Inter var. In my opinion, the text is so incredibly close to the system font stack that it doesn't warrant the extra layout shift. Therefore, I think Inter var should also be made font-display:optional and since it isn't essential we don't need to add preloading.

So in summary, I think the best performance outcome would be for Twenty Nineteen, Twenty Twenty, and Twenty Twenty-One: make all the fonts font-display:optional and do not preload them.

Nevertheless, the logic here for preloading is actually added in vain because other themes actually do have fonts which must be loaded as soon as possible. In particular, any theme using the Genericons icon font needs to have that font preloaded since it necessarily has font-display:auto (block) as there is no fallback available.

So here's what I suggest: add a new argument to AMP_Style_Sanitizer (available in \AMP_Style_Sanitizer::$args with default value stored in \AMP_Style_Sanitizer::$DEFAULT_ARGS) which contains an array of the font family names and the font-display value they should each have. Perhaps something like so:

--- a/includes/sanitizers/class-amp-style-sanitizer.php
+++ b/includes/sanitizers/class-amp-style-sanitizer.php
@@ -166,6 +166,11 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
 		'skip_tree_shaking'              => false,
 		'allow_excessive_css'            => false,
 		'transform_important_qualifiers' => true,
+		'font_face_display_overrides'    => [
+			'NonBreakingSpaceOverride' => 'optional',
+			'Inter var'                => 'optional',
+			'Genericons'               => 'auto',
+		],
 	];
 
 	/**

Then when processing all the @font-face rules, we can check the font-family to see if it matches one of the keys in that array, and if so, override the font-display to match. If the font-display is auto, block, or swap then we should automatically add the preload link for the first font file.

Note that this font_face_display_overrides argument will need to be included in the cache key:

--- a/includes/sanitizers/class-amp-style-sanitizer.php
+++ b/includes/sanitizers/class-amp-style-sanitizer.php
@@ -1655,6 +1655,7 @@ private function get_parsed_stylesheet( $stylesheet, $options = [] ) {
 					'parsed_cache_variant',
 					'dynamic_element_selectors',
 					'transform_important_qualifiers',
+					'font_face_display_overrides',
 				]
 			),
 			[

How does this sound?

@bartoszgadomski
Copy link
Contributor Author

add a new argument to AMP_Style_Sanitizer [...] which contains an array of the font family names and the font-display value they should each have. [...] Then when processing all the @font-face rules, we can check the font-family to see if it matches one of the keys in that array, and if so, override the font-display to match. If the font-display is auto, block, or swap then we should automatically add the preload link for the first font file.

Implemented, ready for review @westonruter 👍

…improve-core-themes-fonts-experience

* 'develop' of github.com:ampproject/amp-wp: (344 commits)
  Add missing translation for external link screen-reader-text
  Add vertical centering of theme screenshots
  Remove apparently obsolete .theme-browser .theme style rule
  Regenerate package-lock.json at lockfileVersion 1
  Discontinue unhooking wp_post_preview_js() in favor of dev mode
  Mark script output by wp_comment_form_unfiltered_html_nonce() as being in dev mode
  Mark wp-polyfill as a dev mode script for paired browsing
  Update Gutenberg package dependencies
  Eliminate emoji handling altogether since most browsers now support natively
  Improve copy for Sandboxing Level drawer
  Set npm to be less than v7 in engines
  Downgrade to lockfileVersion 1
  Refactor threaded comments handling to remove script if no comment form is on the page
  Update Gutenberg package dependencies
  Omit AMP runtime on sandbox levels 1 & 2 if not needed
  Ensure Sandboxing::finalize_document() is only called when experiment is enabled
  Move Sandboxing Level to Advanced Settings
  Revert package-lock.json lockfileVersion to 1
  Update amphtml spec to 2110290545003
  Add screen reader text in visit site button
  ...
&&
1 <= count( $font_files )
) {
$this->dom->links->addPreload( $font_files[0], RequestDestination::FONT );
Copy link
Member

Choose a reason for hiding this comment

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

Good!

'font_face_display_overrides' => [
'NonBreakingSpaceOverride' => 'optional',
'Inter var' => 'optional',
'Genericons' => 'auto',
Copy link
Member

Choose a reason for hiding this comment

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

After considering this more, I think block is the better value to explicitly say we don't want the font to render anything until it loads.

Suggested change
'Genericons' => 'auto',
'Genericons' => 'block',

Copy link
Member

Choose a reason for hiding this comment

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

From what I read, browsers treat auto as “often similar” to block, but there seems some uncertainty.

Comment on lines 2705 to 2708
$properties = $ruleset->getRules( 'font-display' );
if ( isset( $properties[0] ) ) {
$ruleset->removeRule( $properties[0] );
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$properties = $ruleset->getRules( 'font-display' );
if ( isset( $properties[0] ) ) {
$ruleset->removeRule( $properties[0] );
}
foreach ( $ruleset->getRules( 'font-display' ) as $rule ) {
$ruleset->removeRule( $rule );
}

Copy link
Member

Choose a reason for hiding this comment

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

Or rather just:

Suggested change
$properties = $ruleset->getRules( 'font-display' );
if ( isset( $properties[0] ) ) {
$ruleset->removeRule( $properties[0] );
}
$ruleset->removeRule( 'font-display' );

includes/sanitizers/class-amp-style-sanitizer.php Outdated Show resolved Hide resolved
includes/sanitizers/class-amp-style-sanitizer.php Outdated Show resolved Hide resolved
Comment on lines 2718 to 2729
$properties = $ruleset->getRules( 'font-display' );
if (
(
( isset( $properties[0] ) && in_array( $properties[0]->getValue(), [ 'auto', 'block', 'swap' ], true ) )
||
( ! isset( $properties[0] ) ) // Defaults to 'auto', hence should be preloaded as well.
)
&&
1 <= count( $font_files )
) {
$this->dom->links->addPreload( $font_files[0], RequestDestination::FONT );
}
Copy link
Member

Choose a reason for hiding this comment

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

I just read Prevent layout shifting and flashes of invisible text (FOIT) by preloading optional fonts which recommends adding preload links to optional fonts as well, although it says:

Although it is not necessary to preload an optional font, it greatly improves the chance for it to load before the first render cycle, especially if it is not yet stored in the browser's cache.

Nevertheless, adding a preload will introduce network congestion as resources compete for bandwidth. So if the font truly is optional, IMO it seems preferable to not preload it.

*/
if ( $converted_count && 0 === count( $ruleset->getRules( 'font-display' ) ) ) {
if ( isset( $font_family ) && in_array( $font_family, array_keys( $this->args['font_face_display_overrides'] ), true ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

One more condition is missing here it seems, and that is if the first font URL in the list is a data: URL, then we must not add a preload.

Copy link
Member

Choose a reason for hiding this comment

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

Failing test case added in 7c6afe7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with e02b96a

Comment on lines 2706 to 2711
$font_file = plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename";
$data_url->getURL()->setString( $font_file );
if ( 'inline' === $first_src_type ) {
$first_src_type = 'file';
$font_file = $font_file;
}
Copy link
Member

Choose a reason for hiding this comment

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

Humm, $font_file = $font_file?

I think this can be just:

Suggested change
$font_file = plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename";
$data_url->getURL()->setString( $font_file );
if ( 'inline' === $first_src_type ) {
$first_src_type = 'file';
$font_file = $font_file;
}
$font_file = plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename";
$data_url->getURL()->setString( $font_file );
$first_src_type = 'file';

Copy link
Member

Choose a reason for hiding this comment

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

Done in 6c16113

&&
! empty( $font_file )
) {
$this->dom->links->addPreload( $font_file, RequestDestination::FONT );
Copy link
Member

Choose a reason for hiding this comment

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

I just realized this can't be done bere because when a parsed stylesheet is cached, this logic won't run. This opens a can of worms, as it means we have to collect the font URLs to preload and then pass them back all the way up to where we store the cached stylesheet, and then we need to add the preloading when we add the processed stylesheet to the document.

Copy link
Member

Choose a reason for hiding this comment

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

When I activated the Twenty Fifteen theme, for example, I'd only see the Genericons preload link the first time I visited a page. After reloading it would go away.

Failing test case added in 95c7da2 and fix implemented in b3b6579.

@westonruter westonruter added this to the v2.2 milestone Nov 17, 2021
@westonruter
Copy link
Member

@bartoszgadomski Please review the additional changes.

@milindmore22 This is ready to review as well.

@milindmore22
Copy link
Collaborator

@bartoszgadomski When I tried this PR, I didn't see font being preloaded

@bartoszgadomski
Copy link
Contributor Author

Hi @milindmore22!

Fonts used in Twenty Nineteen, Twenty Twenty and Twenty Twenty One themes are not preloaded, as we decided to preload fonts that uses font-display: 'auto', 'block' or 'swap' values only: https://github.com/ampproject/amp-wp/blob/fix/6036-improve-core-themes-fonts-experience/includes/sanitizers/class-amp-style-sanitizer.php#L2745-L2758

The only change we do for core themes is to override the font-display property values, but none of these trigger the preload: https://github.com/ampproject/amp-wp/blob/fix/6036-improve-core-themes-fonts-experience/includes/sanitizers/class-amp-style-sanitizer.php#L173-L177

To see preloading in action, you can use any theme that load custom fonts from local filesystem, i.e. Maxwell: https://wordpress.org/themes/maxwell/; this theme initially request fonts from Google Fonts, then save font files in local filesystem and create a CSS file with fonts definitions where all fonts have font-display: swap property.

Copy link
Collaborator

@milindmore22 milindmore22 left a comment

Choose a reason for hiding this comment

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

@bartoszgadomski looks good to me 👍🏼

@westonruter westonruter merged commit f98f3c0 into develop Nov 30, 2021
@westonruter westonruter deleted the fix/6036-improve-core-themes-fonts-experience branch November 30, 2021 05:27
@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
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
3 participants