Make WordPress Core

Opened 2 months ago

Closed 8 weeks ago

Last modified 8 weeks ago

#60605 closed defect (bug) (fixed)

Font face resolver: If a font family name is repeated across theme.json origins only the font faces from one origin are rendered.

Reported by: mmaattiiaass's profile mmaattiiaass Owned by: youknowriad's profile youknowriad
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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 issue was originally reported here:
https://github.com/WordPress/gutenberg/issues/58764

Change History (6)

This ticket was mentioned in PR #6161 on WordPress/wordpress-develop by @mmaattiiaass.


2 months ago
#1

  • Keywords has-patch has-unit-tests added

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

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

## How ?
parse_settings 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.

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

## Note
Note 1: Maybe in the future we could add some checking to avoid printing the same font variant (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.

@ironprogrammer commented on PR #6161:


2 months ago
#2

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

  1. 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).
  2. 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.
  3. 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").
  4. 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.
  5. Apply patch.
  6. Refresh the site frontend, and observe that the serif font displayed is now Cardo, and matches the editor.
  7. 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:_

https://github.com/WordPress/wordpress-develop/assets/824344/c7903a68-57ed-4a07-945d-a689c15044fb

_From view source during testing:_

<details>
<summary>Before Patch @font-face Declarations -- missing theme fonts ❌</summary>

</details>

<details>
<summary>After Patch @font-face Declarations -- including theme fonts ✅</summary>

</details>

@mmaattiiaass commented on PR #6161:


2 months ago
#3

Thanks, everyone, for the help here.

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

#4 @youknowriad
8 weeks ago

  • Owner set to youknowriad
  • Resolution set to fixed
  • Status changed from new to closed

In 57720:

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

This commit updates the theme.json style generation to allow a font family name to be repeated across theme.json origins (default, theme, custom).

Props mmaattiiaass, hellofromtonya, arthur791004, ironprogrammer.
Fixes #60605.

#5 @youknowriad
8 weeks ago

  • Milestone changed from Awaiting Review to 6.5
Note: See TracTickets for help on using tickets.