Font face resolver: print font faces from font families defined in all theme.json origins #6161

matiasbenedetto · 2024-02-22T17:20:23Z

What?

Font face resolver: If a font family name is repeated across theme.json origins (default, theme, custom), only the font faces from one origin are rendered, so the site lacks the other font faces added.

This problem was reported originally here: WordPress/gutenberg#58764

Why?

To print the missing font faces when a font family is defined in more than one place.

How ?

The parse_settings method of the WP_Font_Face_Resolver class uses an associative array to list all the font families that must be printed on the page. This PR replaces it with an indexed array because the associative array prevents having more than one font family with the same name, causing the bug that this PR fixes and seems unnecessary.

Testing instruction

Use a theme with at least one font family defined with some font faces.
Using the font library, install a font family with the same name as the existing in the theme.
Navigate the site's public frontend and check that all the font faces were printed to the page in the <style id="wp-fonts-local"></style> tag.

Notes

Note 1: Maybe, in the future, we could add some checking to avoid printing repeated font variants (same weight, style, character set, etc). I'm not completely sure that's needed, though, because the browser handles that. Anyways, that's out of the scope of this PR.

Note 2: this issue isn't related strictly to the Font Library. The font library made it visible because of the use of custom apart from theme theme.json data origin, and because of the existing UI is easy to test this using the Font Library.

Note 3: This is an alternative approach to WordPress/gutenberg#59119

Trac ticket: https://core.trac.wordpress.org/ticket/60605#ticket

…are defined in more than one origin (default, theme, custom).

github-actions · 2024-02-22T17:20:39Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props mmaattiiaass, hellofromtonya, arthur791004, ironprogrammer.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-02-22T17:45:22Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

tests/phpunit/tests/fonts/font-face/wp-font-face-tests-dataset.php

src/wp-includes/fonts.php

Co-authored-by: Tonya Mork <[email protected]>

tests/phpunit/tests/fonts/font-face/wpFontFaceResolver/getFontsFromThemeJson.php

Co-authored-by: Tonya Mork <[email protected]>

tests/phpunit/tests/fonts/font-face/wpFontFaceResolver/getFontsFromThemeJson.php

Co-authored-by: Tonya Mork <[email protected]>

hellofromtonya · 2024-02-22T22:35:08Z

tests/phpunit/tests/fonts/font-face/wpFontFaceResolver/getFontsFromThemeJson.php

+				$data = $theme_json_data->get_data();
+
+				// Add font families to the custom origin of theme json.
+				$data['settings']['typography']['fontFamilies']['custom'] = $this->get_custom_font_families( 'input' );


Whoopsie, the function does not understand $this.

How to fix? Move this function callback to be a method instead.

What about this way?fea3908

hellofromtonya · 2024-02-22T22:37:04Z

tests/phpunit/tests/fonts/font-face/wpFontFaceResolver/getFontsFromThemeJson.php

+		$add_custom_fonts = function ( $theme_json_data ) {
+			$data = $theme_json_data->get_data();
+			// Add font families to the custom origin of theme json.
+			$data['settings']['typography']['fontFamilies']['custom'] = $this->get_custom_font_families( 'input' );


Whoopsie, a function within a method does not understand $this.

How to fix? Move this function callback to be a method instead.

Sorry, I didn't notice it when I suggested moving to a variable.

This commit fixes it fea3908

ironprogrammer · 2024-02-22T23:43:26Z

Thanks for the fix here, @matiasbenedetto and @hellofromtonya!

Test Report

Environment

System: MacBook Pro Apple M1 Pro, macOS 14.3.1
Browser: Safari 17.3.1
Server: nginx/1.25.4, PHP 8.3.3, MySQL 8.0.27
WordPress: 6.5-beta2-57671-src
Theme: twentytwentyfour v1.0

Steps to Test Patch

With Twenty Twenty-Four active (includes three Cardo font faces), open the site editor, Styles > Typography, and in the Font Library dialog, install "Cardo 400 Italic" as an additional font (via the "Install Fonts" tab).
Under Typography > ELEMENTS > Text, assign the theme's Cardo (Regular weight) font. Text elements in the editor should reflect the Cardo font update. Click Save.
Open the site frontend, and observe that the serif font (used in headings, text, buttons, etc) does not match the editor (font falls back to "serif").
View source and check the content of <style id="wp-fonts-local">. Observe that only one Cardo @font-face was printed, the newly added "Cardo 400 Italic" face.
Apply patch.
Refresh the site frontend, and observe that the serif font displayed is now Cardo, and matches the editor.
View source and check the content of <style id="wp-fonts-local">. Observe that four Cardo @font-faces were printed: the three weights included with the theme, followed by the added "Cardo 400 Italic" face.

Actual Results

✅ On the site frontend, the theme's "Cardo" faces, as well as the additional "Cardo" face, are printed to the <style id="wp-fonts-local"> element.
✅ PHPUnit tests for fonts and fontface groups pass.

Supplemental Artifacts

Font displayed comparison before/after patch:

From view source during testing:

Before Patch `@font-face` Declarations -- missing theme fonts ❌

<style id="wp-fonts-local">
@font-face {
    font-family: Inter;
    font-style: normal;
    font-weight: 300 900;
    font-display: fallback;
    src: url('http://wp-src.test/wp-content/themes/twentytwentyfour/assets/fonts/inter/Inter-VariableFont_slnt,wght.woff2') format('woff2');
    font-stretch: normal;
}

@font-face {
    font-family: Cardo;
    font-style: italic;
    font-weight: 400;
    font-display: fallback;
    src: url('http://wp-src.test/wp-content/fonts/wlpxgwjKBV1pqhv93IE73W5OcCk.woff2') format('woff2');
}
</style>

After Patch `@font-face` Declarations -- including theme fonts ✅

<style id="wp-fonts-local">
@font-face {
    font-family: Inter;
    font-style: normal;
    font-weight: 300 900;
    font-display: fallback;
    src: url('http://wp-src.test/wp-content/themes/twentytwentyfour/assets/fonts/inter/Inter-VariableFont_slnt,wght.woff2') format('woff2');
    font-stretch: normal;
}

@font-face {
    font-family: Cardo;
    font-style: normal;
    font-weight: 400;
    font-display: fallback;
    src: url('http://wp-src.test/wp-content/themes/twentytwentyfour/assets/fonts/cardo/cardo_normal_400.woff2') format('woff2');
}

@font-face {
    font-family: Cardo;
    font-style: italic;
    font-weight: 400;
    font-display: fallback;
    src: url('http://wp-src.test/wp-content/themes/twentytwentyfour/assets/fonts/cardo/cardo_italic_400.woff2') format('woff2');
}

@font-face {
    font-family: Cardo;
    font-style: normal;
    font-weight: 700;
    font-display: fallback;
    src: url('http://wp-src.test/wp-content/themes/twentytwentyfour/assets/fonts/cardo/cardo_normal_700.woff2') format('woff2');
}

@font-face {
    font-family: Cardo;
    font-style: italic;
    font-weight: 400;
    font-display: fallback;
    src: url('http://wp-src.test/wp-content/fonts/wlpxgwjKBV1pqhv93IE73W5OcCk.woff2') format('woff2');
}
</style>

arthur791004

Using an indexed array looks good to me 👍

Note 1: Maybe, in the future, we could add some checking to avoid printing repeated font variants (same weight, style, character set, etc). I'm not completely sure that's needed, though, because the browser handles that. Anyways, that's out of the scope of this PR.

Yep, this is the only concern. We may print duplicate face-faces definitions that are defined by different origins and it has a chance to increase the response size a lot.

matiasbenedetto · 2024-02-23T13:33:31Z

Thanks, everyone, for the help here.

@youknowriad this seems ready to go into the beta. Could you take a look?

youknowriad · 2024-02-27T12:05:35Z

Commit https://core.trac.wordpress.org/changeset/57720

matiasbenedetto added 2 commits February 22, 2024 13:58

Print all the font families. Fix font families not rendering if they …

dbe5d87

…are defined in more than one origin (default, theme, custom).

Updates tests that realy on an not needed associative array

0ef55e5

matiasbenedetto mentioned this pull request Feb 22, 2024

Font Weight Conflicts Between Built-in and Installed Fonts WordPress/gutenberg#58764

Closed

format php

0be8d60