[Fonts API] Refactor Theme JSON global functions into WP_Fonts_Resolver #50811

anton-vlasenko · 2023-05-20T10:49:51Z

What?

There are 2 global functions within the lib/experimental/fonts-api/register-fonts-from-theme-json.php file that will be needed in Core as part of the API. These functions are large and doing multiple tasks, as such are difficult to debug and understand.

Why?

To improve code quality, readability, and prepare these functionality for future and Core, lib/experimental/fonts-api/register-fonts-from-theme-json.php and its functions need a redesign / refactoring. It's a better fit for a Core-only static class wrapper pattern (i.e. a class with static methods).

How?

Rename the file to class-wp-fonts-theme-json-handler.php.
Load the file into memory.
Add a Core-only static class wrapper called WP_Fonts_Theme_Json_Handler.
Add a public static method, coping the code from each global function.
Split each method into smaller single-purpose private static methods.
Update the tests.
Move the add_action( 'init', 'gutenberg_register_fonts_from_theme_json', 21 ); into the fonts-api.php file and update it to use the new static method instead of the global callback function.
Update where gutenberg_add_registered_fonts_to_theme_json() is used to now call its static method.
Update the DocBlocks as they are inaccurate.

Testing Instructions

Manual Testing

How these functions work and what they do did not change. This means the functionality should work exactly as it did for, i.e. scope of this PR should be a pure refactoring. Manual testing is focused on ensuring no changes happened to the functionality, i.e. works the same as in trunk.

Setting up your local test site:

Make sure Gutenberg is activated: Plugins > Gutenberg.
Install and activate this test plugin.
Activate the TT3, if it is not already.

Testing the theme's fonts get automatically registered and enqueued into the Fonts API:

Open the Site Editor > Styles > Typography UI.
Select Heading.
In the Fonts selector, verify the theme's fonts are present for selection.
Go to the front-end.
In your browser's dev tools in the Inspector (HTML) tab, search for wp-fonts-local. Expectation: It should be in the <head> and contain the @font-face styles for the theme's fonts.

Testing plugin fonts are added to the Theme JSON data layer:

Open the Site Editor > Styles > Typography UI.
Select Heading.
Select a Google Font for all headings:
- For Fonts: select Playfair Display.
- For Appearance: select Bold Italic.
Select the "Save" button twice, which saves your user selections.
Go to the front-end and refresh the page.
In your browser's dev tools in the Inspector (HTML) tab:
- Search for wp-fonts-jetpack-google-fonts. Expectation: The <style> element should be in the <head> and the CSS in it should only contain @font-face styles for Playfair Display.
- Search for wp-fonts-local. Expectation: It should be in the <head> and contain the @font-face styles for the theme's fonts.

Automated Testing

Run the following:

npm run test:unit:php -- --group fontsapi

All tests should pass.

github-actions · 2023-05-20T11:24:15Z

Flaky tests detected in c146215.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5083809805
📝 Reported issues:

[Flaky Test] No Behaviors should be the default as defined in the core theme.json #50923 in /test/e2e/specs/editor/various/behaviors.spec.js

hellofromtonya · 2023-05-22T16:24:49Z

lib/experimental/fonts-api/class-wp-fonts-theme-json-handler.php

+ *
+ * @since X.X.X
+ */
+class WP_Fonts_Theme_Json_Handler {


I'm rethinking this new class. Since I first wrote the issue's scope, a new resolver class WP_Fonts_Resolver was merged into the API. I'm thinking the static functions here can be merged into that resolver class instead. Why? These methods are doing resolver work. Rather than adding another class, I propose consolidating the functionality into the resolver.

To retain @anton-vlasenko's work, I'll open a new PR but cherry-pick Anton's work as a starting point.

hellofromtonya

Blocking this PR from merge. A new PR will be opened to consolidate the functionality into the new WP_Fonts_Resolver class.

hellofromtonya · 2023-05-25T17:50:38Z

As a POC (proof of concept), I created PR #50914. It does the following:

Moves the code into the existing WP_Fonts_Resolver class instead of a new class.
Does more refactoring for readability and reuse.
Moves the registered "origin" to the WP_Fonts as a constant to encapsulate that knowledge in one place.
Renames one method to better reflect what it does: add_missing_fonts rather than add_registered_fonts.
A bit of girlscouting for adding @since, changing "webfont" to "font", etc.

I'll now add each of the changes to this PR to keep it all in the original work @anton-vlasenko created.

Tasks:

Rebase this PR on top of trunk to bring in the latest changes including the new WP_Fonts_Resolver.
Move the code to WP_Fonts_Resolver.
Refactor additional pieces.
Rename to ::add_missing_fonts_to_theme_json().
Girlscouting.

…n-handler.php.

…istered_fonts_to_theme_json() to static methods.

…me_json to WP_Fonts_Theme_Json_Handler::register_fonts_from_theme_json.

…_theme_json to WP_Fonts_Theme_Json_Handler::add_registered_fonts_to_theme_json.

…lies.

Which fonts get added to the Theme JSON data depends on if in the Site Editor or not. Site Editor = registered fonts. Else = enqueued fonts. Renaming this method better reflects what it does.

Why? To centralize what the origin value is in one place, rather than dispersing the internal details outside of the class.

hellofromtonya · 2023-05-25T20:00:05Z

Test Report

Env:

OS: macOS
Browser: Chrome, Firefox, Edge
LocalHost: Local
WP: 6.2.2
Plugins: Gutenberg and fonts test plugin
Theme: TT3

Results

Testing the theme's fonts get automatically registered and enqueued into the Fonts API

✅ style#wp-fonts-local is in the <head> in the front-end and each iframed editor.

✅ All of the TT3 fonts have @font-face styles in that element.

Testing plugin fonts are added to the Theme JSON data layer

In the Site Editor:
✅ Theme and test plugin's fonts are available for selection.

✅ After selecting the Playfair Display for headings and refreshing on the front-end:

✅ style#wp-fonts-jetpack-google-fonts is in the <head> and the CSS in it only contains @font-face styles for Playfair Display.
✅ style#wp-fonts-local is in the <head> and the CSS contains all of TT3 fonts @font-face styles.

Conclusion

✅ Works as expected

hellofromtonya

@anton-vlasenko the work you did was excellent. Well done 👏

With the new WP_Fonts_Resolver, I moved your work into it, which eliminated the need for another class. Sorry for the confusion.

I've updated the PR accordingly and made a couple more refactorings.

All of the PHPUnit tests are passing ✅

Manual testing works as expected ✅

This PR is ready for merging.

ironprogrammer · 2023-05-25T21:03:02Z

Test Report

Environment

Hardware: MacBook Pro Apple M1 Pro
OS: macOS 13.3.1
Browser: Safari 16.4
Server: nginx/1.23.4
PHP: 7.4.33
WordPress: 6.3-alpha-55505-src
Theme: twentytwentythree v1.1
Active Plugins:
- webfonts-jetpack-test v1.0.0-ironprogrammer
- gutenberg v15.9.0-rc.1 (PR applied)

Actual Results

With TT3 only:

✅ In Site Editor > Styles > Typography, font pickers for each Element contain the theme's registered fonts.
✅ In the frontend, head>script#wp-fonts-local contains the theme's registered fonts.

With TT3 and the fonts test plugin, setting the global typography for headings and text:

✅ In Site Editor > Styles > Typography, font pickers for each Element contain the theme's registered fonts, plus the fonts registered by the test plugin.
✅ In the frontend, head>script#wp-fonts-local contains the theme's registered fonts.
✅ In the frontend, head>script#wp-fonts-jetpack-google-fonts contains only the user-selected fonts.

Additional Information

Note that as expected, block-level Typography font pickers (in site editor and post editor) also contained all registered fonts, which when user-selected, were subsequently enqueued in the frontend. This is consistent with behavior on trunk, helping validate that functional behavior from this PR remains unchanged.

anton-vlasenko · 2023-05-29T23:31:00Z

@anton-vlasenko the work you did was excellent. Well done 👏

Thank you, @hellofromtonya. I'm glad you like it.

BTW, I've noticed a typo in this PR.
WP_Fonts_Resolver::set_tyopgraphy_settings_array_structure should be renamed to WP_Fonts_Resolver::set_typography_settings_array_structure.

Probably, this doesn't deserve a separate PR and could be fixed in one of the following PRs (it's a private method anyway).

anton-vlasenko changed the title ~~Update/refactor register fonts from theme json php~~ [Fonts API] Refactor theme.json to Fonts API handler global functions into static class wrapper May 20, 2023

anton-vlasenko marked this pull request as ready for review May 22, 2023 08:42

anton-vlasenko requested a review from spacedmonkey as a code owner May 22, 2023 08:42

anton-vlasenko linked an issue May 22, 2023 that may be closed by this pull request

[Fonts API] Refactor theme.json to Fonts API handler global functions into static class wrapper #50250

Closed

anton-vlasenko mentioned this pull request May 22, 2023

[Fonts API] Refactor theme.json to Fonts API handler global functions into static class wrapper #50250

Closed

anton-vlasenko requested review from hellofromtonya, ironprogrammer and aristath and removed request for aristath May 22, 2023 08:49

anton-vlasenko added [Feature] Fonts API and removed [Feature] Fonts API labels May 22, 2023

hellofromtonya removed the request for review from spacedmonkey May 22, 2023 14:00

hellofromtonya reviewed May 22, 2023

View reviewed changes

hellofromtonya requested changes May 22, 2023

View reviewed changes

hellofromtonya mentioned this pull request May 24, 2023

[Fonts API] Refactor theme.json global functions into the WP_Fonts_Resolver #50914

Closed

3 tasks

anton-vlasenko added 13 commits May 25, 2023 12:55

Rename register-fonts-from-theme-json.php to class-wp-fonts-theme-jso…

8f7db42

…n-handler.php.

Guard the class.

9aef1bb

Move gutenberg_register_fonts_from_theme_json() and gutenberg_add_reg…

2ae9a5e

…istered_fonts_to_theme_json() to static methods.

Rename WP_Fonts_Theme_Json_Handler::gutenberg_register_fonts_from_the…

2b6ca3c

…me_json to WP_Fonts_Theme_Json_Handler::register_fonts_from_theme_json.

Rename WP_Fonts_Theme_Json_Handler::gutenberg_add_registered_fonts_to…

d3e5875

…_theme_json to WP_Fonts_Theme_Json_Handler::add_registered_fonts_to_theme_json.

Move part of the code to WP_Fonts_Theme_Json_Handler::parse_font_fami…

e86d6f9

…lies.

Fix PHPDoc block.

a7121fe

Move anonymous function into a static method.

e61fda9

Fix PHPDoc block.

0069976

Move methods within the file.

87c7f37

Rename to ::get_font_families.

12c0684

Rename ::get_families to ::get_font_families.

1b457a2

Fix CS.

367cc15

anton-vlasenko added 5 commits May 25, 2023 12:55

Fix @Covers

c8b384a

Update PHPDoc blocks.

a6e8a03

Rename files.

6b2a4a9

Rename files.

326da9c

Update PHPDoc blocks.

6411d24

hellofromtonya force-pushed the update/refactor-register-fonts-from-theme-json-php branch from 1f949e0 to 6411d24 Compare May 25, 2023 17:55

hellofromtonya added 5 commits May 25, 2023 13:14

Relocate code to WP_Fonts_Resolver

901182f

Rename add_registered_fonts to add_missing_fonts.

0a4a7f1

Which fonts get added to the Theme JSON data depends on if in the Site Editor or not. Site Editor = registered fonts. Else = enqueued fonts. Renaming this method better reflects what it does.

Define registered origin on WP_Fonts

0ed7cc2

Why? To centralize what the origin value is in one place, rather than dispersing the internal details outside of the class.

Additional refactorings

1214f8d

Girlscouting: since, webfont, spacing.

c146215

hellofromtonya self-requested a review May 25, 2023 19:41

hellofromtonya approved these changes May 25, 2023

View reviewed changes

hellofromtonya merged commit b95148f into trunk May 25, 2023

hellofromtonya deleted the update/refactor-register-fonts-from-theme-json-php branch May 25, 2023 21:05

hellofromtonya changed the title ~~[Fonts API] Refactor theme.json to Fonts API handler global functions into static class wrapper~~ [Fonts API] Refactor Theme JSON global functions into WP_Fonts_Resolver May 25, 2023

github-actions bot added this to the Gutenberg 16.0 milestone May 25, 2023

ramonjd mentioned this pull request Jun 5, 2023

Plugin: Synchronize PHP changes for WordPress 6.3 release #51077

Closed

ndiego added [Type] Code Quality Issues or PRs that relate to code quality [Feature] Fonts API labels Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fonts API] Refactor Theme JSON global functions into WP_Fonts_Resolver #50811

[Fonts API] Refactor Theme JSON global functions into WP_Fonts_Resolver #50811

anton-vlasenko commented May 20, 2023 •

edited by hellofromtonya

Loading

github-actions bot commented May 20, 2023 •

edited

Loading

hellofromtonya May 22, 2023 •

edited

Loading

hellofromtonya left a comment

hellofromtonya commented May 25, 2023 •

edited

Loading

hellofromtonya commented May 25, 2023

hellofromtonya left a comment

ironprogrammer commented May 25, 2023

anton-vlasenko commented May 29, 2023

[Fonts API] Refactor Theme JSON global functions into WP_Fonts_Resolver #50811

[Fonts API] Refactor Theme JSON global functions into WP_Fonts_Resolver #50811

Conversation

anton-vlasenko commented May 20, 2023 • edited by hellofromtonya Loading

What?

Why?

How?

Testing Instructions

Manual Testing

Automated Testing

github-actions bot commented May 20, 2023 • edited Loading

hellofromtonya May 22, 2023 • edited Loading

Choose a reason for hiding this comment

hellofromtonya left a comment

Choose a reason for hiding this comment

hellofromtonya commented May 25, 2023 • edited Loading

hellofromtonya commented May 25, 2023

Test Report

Results

Testing the theme's fonts get automatically registered and enqueued into the Fonts API

Testing plugin fonts are added to the Theme JSON data layer

Conclusion

hellofromtonya left a comment

Choose a reason for hiding this comment

ironprogrammer commented May 25, 2023

Test Report

Environment

Actual Results

Additional Information

anton-vlasenko commented May 29, 2023

anton-vlasenko commented May 20, 2023 •

edited by hellofromtonya

Loading

github-actions bot commented May 20, 2023 •

edited

Loading

hellofromtonya May 22, 2023 •

edited

Loading

hellofromtonya commented May 25, 2023 •

edited

Loading