[Fonts API] Relocate which fonts to print into wp_print_fonts() #50151

hellofromtonya · 2023-04-27T20:42:13Z

What?

Relocates the decision making code about which fonts to print for the iframed editor out of the script-loader and into the wp_print_fonts() function.

Why?

When in the Site Editor, all registered fonts should be printed for user previewing and selection. Else, all enqueued fonts should be printed. This kind of decision making should be in the Fonts API, rather than externally in the script-loader.

How?

This PR places the decision making logic into wp_print_fonts().

How to identify the iframed editor assets function is invoking the printing?

This function needs to know if the request to print is coming from the iframed editor for decision making about which fonts to print. This is done by passing true to the first parameter of wp_prints_fonts().

Notes: true for $handles is not used by WP_Dependencies (including in wp_print_styles() and wp_print_scripts(). Using it avoids performance hits and BC concerns with the other strategies considered (expand the next section more more details).

Strategies considered

Approach 1: Adding a parameter to the function's signature

Pros:

straight-forward approach as the invoking code in the script-loader files sets it to true.
avoids the overhead of firing a hook.
avoids third-party hooking into this part of the code, i.e. a hook is available later in the call stack.
more performant than the other approaches.

Cons:

Deviates from wp_print_styles() and wp_print_scripts().
Adds a method parameter which must be maintained for BC once in WP Core.

Approach 2: Pass true to the first parameter as flag to indicate to populate the iframed editor handles.

Pros:

No additional function parameter is needed, meaning no future BC concerns for adding a parameter.
Unique flag: true is not used in wp_print_styles() or wp_print_scripts().
No performance hit.

Cons:

Could it cause confusion? 🤔

Approach 3: Add or use a hook.

Approach 3.1: Hook into the `'block_editor_settings_all'` to modify the `'__unstableResolvedAssets`' setting.

In WP Core, the call stack is this:

_wp_get_iframed_editor_assets()
invokes
_wp_get_iframed_editor_assets()
which will invoke
wp_print_fonts()

There is a filter after this code called 'block_editor_settings_all'.

Gutenberg uses this approach to modify the settings for the upcoming release.

Pros:

No extra hook or function parameter to maintain for BC.

Cons:

Once in Core, the code would be separate from _wp_get_iframed_editor_assets(), which would make maintaining more complex.
Contributors would need to stay aware of changes in _wp_get_iframed_editor_assets() that could require changes within wp_print_fonts() of which setting or subsetting to modify. (Conversely if invoking directly within _wp_get_iframed_editor_assets() all knowledge is contained in one place for the iframed editor assets.)
Performance hit: adding and firing a hooked callback has a performance cost.

Approach 3.2: Add a hook in `_wp_get_iframed_editor_assets()`.

Pros:

Wires internal and external iframed editor asset functionality together (vs using a hook later in the call stack).

Cons:

Adds another hook that must be maintained forever due to BC.
Performance hit: firing a hook has a performance cost.

TODO

Relocate from 6.2 compat file.
Relocate from 6.3 compat file.
Explore options of how to tell if the request is from the iframed editor or not.
Add tests to validate.

Testing Instructions

Visually Test:

Set up on your test site:

Activate the Gutenberg plugin.
Activate TT3 theme.

Before applying this PR:

Go to the Site Editor > Styles > Typography.
Using your browser's dev tools, search the HMTL for wp-fonts-local <style> element.
Copying the internals of that element. You'll use this to compare after applying this PR.

After applying this PR:

Go to the Site Editor > Styles > Typography.
Using your browser's dev tools, search the HMTL for wp-fonts-local <style> element.
Compare the styles to the before styles from above. They should match.

Running the automated tests

All tests should pass when running the PHPUnit tests:

npm run test:unit:php -- --filter Tests_Fonts_WpPrintFonts

hellofromtonya · 2023-05-02T20:19:37Z

Now that PR #50225 has been merged into trunk, I'll rebase this PR on top of it.

This simplifies the code as it removes the need to modify and then reset the `done` and `queue` properties.

aristath · 2023-05-03T12:29:10Z

lib/experimental/fonts-api/fonts-api.php

+		// Print all registered fonts for the iframed editor.
+		if ( true === $handles ) {
+			$handles = $registered;
+		}
+


Shouldn't this condition be moved above the if ( empty( $handles ) ) { check a few lines above? 🤔

Good observation @aristath! Yes, I agree with you.

In looking further at this area of code, there's a tiny tiny micro-optimization opportunity which will benefit the front-end, i.e. when false then skip over these decision-making reassignment conditionals as they are unnecessary.

Changes done here fd7ce33

Combines the empty() and true checks into a if/elseif structure. For a tiny micro-optimization, wraps these checks within a not equal to `false`. Why? When using the default (i.e. on the front-end and with classic themes): * Skips running `empty()`. * Skips reassigning `false` to `$handle`. These skips gain a tiny tiny performance boost.

anton-vlasenko

I've tried to follow the testing steps, and after applying the patch I couldn't find the style#wp-fonts-local element.

Printing `$handles` does not work (for some reason that needs to be investigated further). Overwriting the queue and done properties does print and has been working since 6.2 alpha. This commit restores the overwriting properties approach.

hellofromtonya · 2023-05-08T19:36:38Z

@anton-vlasenko thank you for testing. You're right. It does not print within the Site Editor or other block editors (like Posts).

Why?
In investigating, there's something wrong within the Fonts API or WP_Dependencies when injecting $handles into do_items() inside of an ob_ circuit. Not sure yet of the root cause.

How I fixed it:
I've restored the overwriting of queue and done properties, like it was in the 6.2-compat file. In my testing, fonts are again being printed in the Site Editor.

Restoring that logic keeps the PR's scope focused on "relocating" the logic. A follow-up PR can come later for changing the approach to copying registered fonts to $handle (instead of overwriting properties).

Can you re-test please @anton-vlasenko ?

github-actions · 2023-05-08T19:49:29Z

Flaky tests detected in dfd8657.
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/4918768728
📝 Reported issues:

[Flaky Test] should warn of unsaved changes before loading reset revision #50419 in /test/e2e/specs/site-editor/user-global-styles-revisions.spec.js

anton-vlasenko

I've retested this PR after applying the changes in dfd8657, and it worked in my tests. The code makes sense, so I approve this PR.

anton-vlasenko · 2023-05-08T19:56:11Z

Why? In investigating, there's something wrong within the Fonts API or WP_Dependencies when injecting $handles into do_items() inside of an ob_ circuit. Not sure yet of the root cause.

How I fixed it: I've restored the overwriting of queue and done properties, like it was in the 6.2-compat file. In my testing, fonts are again being printed in the Site Editor.

Restoring that logic keeps the PR's scope focused on "relocating" the logic. A follow-up PR can come later for changing the approach to copying registered fonts to $handle (instead of overwriting properties).

Thank you for the thorough explanation, @hellofromtonya!

hellofromtonya · 2023-05-08T19:58:19Z

Thanks for retesting and approving the PR @anton-vlasenko!

hellofromtonya · 2023-05-08T20:06:21Z

Cherry-pick for minor notes:

This PR requires #50225 to be cherry-picked first. Why? It uses code from PR #50225.

cc @bph

autumnfjeld · 2023-05-09T07:39:07Z

cc: @arthur791004

Moves decision-making logic for iframed editor from the compat script-loader files into the Fonts API.

bph · 2023-05-09T07:53:11Z

I just cherry-picked this PR to the release/15.7 branch to get it included in the next release: 5682ad4

hellofromtonya added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality [Feature] Fonts API labels Apr 27, 2023

hellofromtonya self-assigned this Apr 27, 2023

WordPress deleted a comment from github-actions bot May 1, 2023

hellofromtonya mentioned this pull request May 1, 2023

[Fonts API] Remove global from wp_print_fonts() #50225

Merged

hellofromtonya force-pushed the try/move-printing-logic-to-wp_print_fonts branch from 5663329 to 6ce3620 Compare May 1, 2023 21:15

hellofromtonya requested review from ironprogrammer and anton-vlasenko May 2, 2023 16:07

hellofromtonya marked this pull request as ready for review May 2, 2023 16:07

hellofromtonya requested a review from spacedmonkey as a code owner May 2, 2023 16:07

hellofromtonya force-pushed the try/move-printing-logic-to-wp_print_fonts branch from 1a9ae3f to cf9942a Compare May 2, 2023 16:12

hellofromtonya removed the request for review from spacedmonkey May 2, 2023 16:12

hellofromtonya added 5 commits May 2, 2023 15:22

Relocate from 6.2 script-loader

0af2d56

Add flag to 6.3-script-loader call

6147c9b

Add tests

6dd2c35

Assign registered to $handles.

a64362c

This simplifies the code as it removes the need to modify and then reset the `done` and `queue` properties.

Set $handles to true

d95eb1c

hellofromtonya force-pushed the try/move-printing-logic-to-wp_print_fonts branch from cf9942a to d95eb1c Compare May 2, 2023 20:25

aristath reviewed May 3, 2023

View reviewed changes

hellofromtonya force-pushed the try/move-printing-logic-to-wp_print_fonts branch from c1a255a to 338d4aa Compare May 3, 2023 14:30

hellofromtonya force-pushed the try/move-printing-logic-to-wp_print_fonts branch from 338d4aa to fd7ce33 Compare May 3, 2023 14:49

hellofromtonya requested a review from aristath May 4, 2023 17:53

anton-vlasenko reviewed May 4, 2023

View reviewed changes

Restore overwriting queue and done.

dfd8657

Printing `$handles` does not work (for some reason that needs to be investigated further). Overwriting the queue and done properties does print and has been working since 6.2 alpha. This commit restores the overwriting properties approach.

hellofromtonya added the Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release label May 8, 2023

hellofromtonya added this to the Gutenberg 15.7 milestone May 8, 2023

hellofromtonya requested a review from anton-vlasenko May 8, 2023 19:41

hellofromtonya mentioned this pull request May 8, 2023

Google Fonts are not shown in the Editor preview Automattic/wp-calypso#76649

Closed

anton-vlasenko approved these changes May 8, 2023

View reviewed changes

hellofromtonya merged commit fa1d3b0 into trunk May 8, 2023

hellofromtonya deleted the try/move-printing-logic-to-wp_print_fonts branch May 8, 2023 20:07

bph pushed a commit that referenced this pull request May 9, 2023

[Fonts API] Relocate which fonts to print into wp_print_fonts() (#50151)

5682ad4

Moves decision-making logic for iframed editor from the compat script-loader files into the Fonts API.

bph removed the Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release label May 9, 2023

ramonjd mentioned this pull request Jun 5, 2023

Plugin: Synchronize PHP changes for WordPress 6.3 release #51077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fonts API] Relocate which fonts to print into wp_print_fonts() #50151

[Fonts API] Relocate which fonts to print into wp_print_fonts() #50151

hellofromtonya commented Apr 27, 2023 •

edited

Loading

hellofromtonya commented May 2, 2023

aristath May 3, 2023

hellofromtonya May 3, 2023

anton-vlasenko left a comment

hellofromtonya commented May 8, 2023

github-actions bot commented May 8, 2023

anton-vlasenko left a comment

anton-vlasenko commented May 8, 2023

hellofromtonya commented May 8, 2023

hellofromtonya commented May 8, 2023

autumnfjeld commented May 9, 2023

bph commented May 9, 2023

[Fonts API] Relocate which fonts to print into wp_print_fonts() #50151

[Fonts API] Relocate which fonts to print into wp_print_fonts() #50151

Conversation

hellofromtonya commented Apr 27, 2023 • edited Loading

What?

Why?

How?

How to identify the iframed editor assets function is invoking the printing?

Approach 1: Adding a parameter to the function's signature

Approach 2: Pass true to the first parameter as flag to indicate to populate the iframed editor handles.

Approach 3: Add or use a hook.

Approach 3.1: Hook into the 'block_editor_settings_all' to modify the '__unstableResolvedAssets' setting.

Approach 3.2: Add a hook in _wp_get_iframed_editor_assets().

TODO

Testing Instructions

Visually Test:

Running the automated tests

hellofromtonya commented May 2, 2023

aristath May 3, 2023

Choose a reason for hiding this comment

hellofromtonya May 3, 2023

Choose a reason for hiding this comment

anton-vlasenko left a comment

Choose a reason for hiding this comment

hellofromtonya commented May 8, 2023

github-actions bot commented May 8, 2023

anton-vlasenko left a comment

Choose a reason for hiding this comment

anton-vlasenko commented May 8, 2023

hellofromtonya commented May 8, 2023

hellofromtonya commented May 8, 2023

autumnfjeld commented May 9, 2023

bph commented May 9, 2023

hellofromtonya commented Apr 27, 2023 •

edited

Loading

Approach 3.1: Hook into the `'block_editor_settings_all'` to modify the `'__unstableResolvedAssets`' setting.

Approach 3.2: Add a hook in `_wp_get_iframed_editor_assets()`.