Font faces defined in variations aren't loaded in the browser until the variation switch is saved and the page is refreshed

[matiasbenedetto]

Description

When switching style variations, the font faces defined in those variations are not loaded in the editor but after the page is reloaded.

Step-by-step reproduction instructions

1. Switch to a theme style variation with custom font faces defined.
2. Check that the fonts are not loaded in the editor until the page is refreshed (browser reload).

Screenshots, screen recording, code snippet

Look at the headings when a theme style variation is applied, they render the default font until the page is refreshed:

2024-03-18.16-39-33.mp4

Environment info

• Gutenberg trunk

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[mikachan]

Related to #46397.

[justintadlock]

Just noting that this has become an even more evident issue now that typography variations are shipping with WordPress 6.6. Themers are already building less-than-ideal workarounds to "fix" this.

For example, Assembler is loading every font from theme.json: https://github.com/Automattic/themes/blob/f5bc619fa06d7c95b2e649f8780001f5992035c8/assembler/theme.json#L124-L771

For my own theme, I've been hooking into block_editor_settings_all and adding the font faces to the css property when in the Site Editor: https://github.com/x3p0-dev/x3p0-ideas/blob/59fee935708ee8ed14b90e0f044da03840d54d5f/src/Editor.php#L82-L157

[carolinan]

I started looking at the font face resolver class, but I have not worked with this feature before so I am not sure if I am approaching this the right way.

In the deprecated _wp_theme_json_webfonts_handler(), the font family setting was collected from both the global settings and
WP_Theme_JSON_Resolver::get_style_variations();
See https://core.trac.wordpress.org/browser/branches/6.6/src/wp-includes/deprecated.php#L5438

But in WP_Font_Face_Resolver, get_fonts_from_theme_json, they are only collected from the global settings.
https://core.trac.wordpress.org/browser/branches/6.6/src/wp-includes/fonts/class-wp-font-face-resolver.php#L28

Shouldn't the variations only be added back to this existing method?
I could not find any discussions where I could learn why it was not included in the new class and method.

[carolinan]

@matiasbenedetto Hi, are you able to provide some insight on the question above?

[matiasbenedetto]

Shouldn't the variations only be added back to this existing method?
@matiasbenedetto Hi, are you able to provide some insight on the question above?

👋 I wonder if loading all the fonts from style variations solves this issue because it could lead to inconsistencies between the editor and the frontend view.

For example, let's say we are loading the fonts from the theme + the style variations using the backend font face resolver instead of just the theme one as we do today. Let's say we have a theme with the font 'Abel' loaded and included by default, and 'Inter' included in a style variation. Now imagine this situation: the user is editing a template with elements with 'Abel' font set. The user enables the 'Inter' style variation and sets 'Inter' as a font in some elements/blocks. In this case, both the elements set to 'Abel' and 'Inter' will render the right font, but in the front, only the elements set to 'Inter' will render the right font because 'Abel' assets won't be loaded.

The inconsistency could be avoided by loading all the fonts from variations in both the editor and the frontend. Still, I'm not sure if that's the right thing to do because we would be adding extra CSS contents to the frontend that is not needed and probably not logically correct in the sense that if the style variations are not active, it probably should not have any impact on the frontend. If the theme has only one variation, the impact would be minimal, but if the theme has many variations, the problem could be more impactful.

[carolinan]

I don't understand. How would combining the font families from theme.json and styles/style.json prevent fonts from theme.json from loading?

[carolinan]

It needs to be solved, because first adding all fonts to theme.json and make all those presets available by default is not a good
user experience nor a good developer experience.

[matiasbenedetto]

How would combining the font families from theme.json and styles/style.json prevent fonts from theme.json from loading?

I didn't mean that combining fonts will prevent fonts from loading; I mean that loading all the fonts from the style variations in the editor but loading only the fonts from an active variation in the frontend could lead to inconsistencies around how the fonts look in the editor vs the frontend.

[carolinan]

I am not sure that the expected result would be for the "Abel" font to remain unchanged when I switch typography presets.

I tried to test this in Twenty Twenty-Four.
Using the default style variation, I selected the heading block in the home template, the one that says
"A commitment to innovation and sustainability".
I set this to use the font Inter. Note that I am doing this change on the block level, not using global styles.
Then I go to Styles and select Fossile.
Then I select the same block, and it is no longer set to Inter.

[justintadlock]

I think some of the conversation has went beyond the scope of the ticket and is describing a separate problem about what the user selects and what happens on the front end. I think that should be a separate ticket with its own discussion.

The problem at hand is that fonts for global style variations are not loaded in two places:

• The variations list in the sidebar (for theme and typography variations)
• The preview frame when a variation is selected

Based on my testing, @carolinan correctly identified the code that needs to be addressed, which is WP_Font_Face_Resolver::get_fonts_from_theme_json(). Currently, this only pulls font faces from wp_get_global_settings() instead of all style variations: https://core.trac.wordpress.org/browser/branches/6.6/src/wp-includes/fonts/class-wp-font-face-resolver.php#L28

I propose that we update that method (or create a separate method) that grabs the font faces from all variations. It should:

• Get settings from wp_get_global_settings().
• Get all style variations via WP_Theme_JSON_Resolver::get_style_variations().
• Loop through the variations and merge the $variation['settings']['typography']['fontFamilies'] with those from wp_get_global_settings().
• Ensure there are no duplicate font faces in the merged settings.
• Then, call static::parse_settings() (as it does now).

Just working out the basic process in my head now.

Solving for theme authors now (WP 6.6)

Because I needed for this to work now instead of later, I built out a solution for theme authors to use within a theme. I also did this because I wanted to understand the inner workings of this a bit better.

Because WP_Font_Face_Resolver has so many private methods in it, it made it pretty much impossible to extend or use in any practical way. So I first had to create a custom class (some of the code from the getFonts() method of this class is likely the bit we can reuse for a solution for this ticket):

<?php
declare(strict_types=1);

use WP_Theme_JSON_Resolver;

class ThemeslugFontFaceResolver
{
	/**
	 * Returns an array of font definitions to be plugged into functions
	 * like `wp_print_font_faces()` for enqueueing font stylesheets.
	 *
	 * @since 1.0.0
	 */
	public static function getFonts(): array
	{
		$families   = [];
		$settings   = wp_get_global_settings();
		$variations = WP_Theme_JSON_Resolver::get_style_variations();

		// Loop through and store each variation's font families.
		foreach ($variations as $variation) {
			if (empty($variation['settings']['typography']['fontFamilies'])) {
				continue;
			}

			foreach ($variation['settings']['typography']['fontFamilies'] as $group) {
				$families = array_merge($families, $group);
			}
		}

		// Bail early if there are no defined font families.
		if ([] === $families) {
			return [];
		}

		$fonts = static::parseFamilies($families);

		// Because Core could be potentially loading a font that's
		// already defined in our variations, let's remove those to
		// avoid loading them a second time.
		if (! empty($settings['typography']['fontFamilies'])) {
			$global_families = [];

			foreach ($settings['typography']['fontFamilies'] as $group) {
				$global_families = array_merge($global_families, $group);
			}

			if ([] !== $global_families) {
				$fonts = array_diff_key(
					$fonts,
					static::parseFamilies($global_families)
				);
			}
		}

		return $fonts;
	}

	/**
	 * Parses the an array of font families and grabs any font-face
	 * definitions from them.
	 *
	 * @since 1.0.0
	 */
	private static function parseFamilies(array $families): array
	{
		$faces = [];

		foreach ($families as $family) {
			if (! isset($family['fontFace']) || ! isset($family['fontFamily'])) {
				continue;
			}

			$name = static::parseName($family['fontFamily']);

			if (! $name || isset($faces[$name])) {
				continue;
			}

			$faces[$name] = static::convertProperties($family['fontFace'], $name);
		}

		// Just a little nicer sorting. 😊
		ksort($faces);

		return $faces;
	}

	/**
	 * Parses a font-family name. Ensures that we only use the first name if
	 * presented a comma-separated list.
	 *
	 * @since 1.0.0
	 */
	private static function parseName(string $family): string
	{
		if (str_contains($family, ',')) {
			$family = explode(',', $family)[0];
		}

		return trim($family, "\"'");
	}

	/**
	 * Ensures that font-face properties are converted to a form that can be
	 * used in CSS.
	 *
	 * @since 1.0.0
	 */
	private static function convertProperties(array $definitions, string $name): array
	{
		$converted = [];

		foreach ($definitions as $properties) {
			$properties['font-family'] = $name;

			if (! empty($properties['src'])) {
				$properties['src'] = static::toThemeFileUri(
					(array) $properties['src']
				);
			}

			$converted[] = static::toKebabCase($properties);
		}

		return $converted;
	}

	/**
	 * Replaces file URI references from JSON to the theme file URI.
	 *
	 * @since 1.0.0
	 */
	private static function toThemeFileUri(array $src): array
	{
		$placeholder = 'file:./';

		foreach ($src as $key => $url) {
			if (str_starts_with($url, $placeholder)) {
				$src[ $key ] = get_theme_file_uri(
					str_replace($placeholder, '', $url)
				);
			}
		}

		return $src;
	}

	/**
	 * Converts property names/keys to kebab-case.
	 *
	 * @since 1.0.0
	 */
	private static function toKebabCase(array $data): array
	{
		foreach ($data as $key => $value) {
			unset($data[$key]);
			$data[ _wp_to_kebab_case($key) ] = $value;
		}

		return $data;
	}
}

Then I needed to load the resolved font faces in the Site Editor (this needed to load in both the sidebar variation previews and the preview frame on the right) using that class:

add_action('load-site-editor.php', 'themeslug_load_site_editor');

function themeslug_load_site_editor(): void
{
	add_action('enqueue_block_assets', 'themeslug_site_editor_fonts');
}

function themeslug_site_editor_fonts(): void
{
	ob_start();
	wp_print_font_faces(\ThemeslugFontFaceResolver::getFonts());
	$content = ob_get_clean();

	wp_register_style('themeslug-fonts', false);
	wp_add_inline_style('themeslug-fonts', trim(strip_tags($content)));
	wp_enqueue_style('themeslug-fonts');
}

[carolinan]

I agree that it is never ideal when something is loaded/included and not used.
But the same thing will happen if the theme author must include all fonts in theme.json first, and the user don't use all of them on the same page?

I am not sure how to update this resolver through Gutenberg: but if we cant find a better way, a core ticket can be created.

[matiasbenedetto]

I've been trying a client-side solution in the editor for this issue that could work. I'll submit a PR to explore that asap.

[matiasbenedetto]

I think #65019 is ready for review.

[noisysocks]

@matiasbenedetto: Is this a bug or an enhancement? The issue is labeled bug but the PR that closes it is labeled enhancement.

[matiasbenedetto]

I think it's a bug. It's not enhancing anything but fixing an unexpected behavior.

[ndiego]

@matiasbenedetto @mikachan is this still something we are aiming to complete for 6.7, or should it be punted?

[matiasbenedetto]

@matiasbenedetto @mikachan is this still something we are aiming to complete for 6.7, or should it be punted?

@ndiego yes, I'd appreciate reviews on the latest changes: #65019 (comment)

[matiasbenedetto]

Added a core trac ticket for this: https://core.trac.wordpress.org/ticket/62231#ticket

[matiasbenedetto]

I submitted an alternative fix to #65019 that requires only changes in core and is probably simpler. This is more in line with @carolinan 's idea, which I initially thought wouldn't work, but it seems to be working fine in WordPress/wordpress-develop#7581

[ironprogrammer]

Dropping in to verify that the solution in WordPress/wordpress-develop#7581 also worked for me, reported on Trac #62231. I've also moved that ticket to the 6.7 milestone for visibility.

More tests/confidence checks on the Trac ticket would be brilliant! 🙏🏻

[getdave]

WordPress/wordpress-develop#7581 was committed to I believe this can now be closed.

cc'ing @matiasbenedetto who can reopen if this is not correct.

[matiasbenedetto]

@getdave, yes, it's fine to close this. It should be fixed in core by WordPress/wordpress-develop#7581