Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Font API] Use registered fonts for Block Editor iframe. #49646

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Apr 6, 2023

What?

Closes #49645

Fixes which fonts get their @font-face styles generated the specific iframe:

  • Site Editor: uses all registered fonts.
  • Block Editor: uses all enqueued fonts.

Why?

Outside of the Site Editor, only the enqueued fonts are to be used. Why? To avoid a potential performance impact when a site has one or more plugins or scripts that are registering fonts with the Fonts API.

In the Site Editor, all registered are to be used. Why? To allow users to preview typography choices before selecting and updating the global styles.

How?

Uses $pagenow to identify if the web page is the Site Editor. If yes, then modifies the queue to use the registered fonts and restores the queue after the @font-face styles are generated.

Testing Instructions

  1. On your test site, activate the TT3 theme and Gutenberg plugin.
  2. Install and activate this test plugin.
  3. In the fonts tester plugin, copy / paste the following code in the Global_Styles::enqueue_global_styles_fonts() found in the /vendor/automattic/jetpack-google-fonts-provider/src/introspectors/class-global-styles.php file. This change fixes a bug (by removing the admin check) for enqueuing the plugin's fonts in the editor's iframe.
	/**
	 * Enqueue fonts used in global styles settings.
	 *
	 * @return void
	 */
	public static function enqueue_global_styles_fonts() {
		if ( ! function_exists( 'wp_enqueue_webfonts' ) || ! function_exists( 'wp_enqueue_fonts' )  ) {
			return;
		}

		$global_styles_fonts = self::collect_fonts_from_global_styles();

		$fonts_to_enqueue = array();
		foreach ( $global_styles_fonts as $font ) {
			$font_is_registered = Utils::is_font_family_registered( $font );

			if ( $font_is_registered ) {
				$fonts_to_enqueue[] = $font;
			}
		}

		if ( ! empty( $fonts_to_enqueue ) ) {
			if ( function_exists( 'wp_enqueue_fonts' ) ) {
				wp_enqueue_fonts( $fonts_to_enqueue );
			} else {
				wp_enqueue_webfonts( $fonts_to_enqueue );
			}
		}
	}
  1. Open a post by going to Posts > and selecting or adding a post.
  2. In your browser, open Dev Tools to the Inspector tab.
  3. Search for wp-fonts-jetpack-google-fonts.
    The expected behavior:
    • <style id="wp-fonts-jetpack-google-fonts"> should not exist.
    • <style id="wp-fonts-local"> should exist (these are the theme's defined fonts).
  4. Then go to the Site Editor, i.e. Appearance > Editor.
  5. Search for wp-fonts-jetpack-google-fonts.
    The expected behavior:
    • <style id="wp-fonts-jetpack-google-fonts"> should exist.
    • <style id="wp-fonts-local"> should exist (these are the theme's defined fonts).

@hellofromtonya hellofromtonya force-pushed the fix/render-enqueued-fonts-block-editor-iframe branch from de755bd to 1647b01 Compare April 6, 2023 19:18
@github-actions
Copy link

github-actions bot commented Apr 6, 2023

Flaky tests detected in 1647b01.
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/4632308306
📝 Reported issues:

@aristath
Copy link
Member

This looks good to me 👍
The only thing I'm not sure of, is the location of these tweaks. Right now they are in the 6.2 folder, perhaps they should be added to the 6.3 folder instead? 🤔

@hellofromtonya
Copy link
Contributor Author

The only thing I'm not sure of, is the location of these tweaks. Right now they are in the 6.2 folder, perhaps they should be added to the 6.3 folder instead? 🤔

@aristath The fix is for 6.2+, as the iframe changes were introduced in 6.2. Currently, the break is in the 6.2 compat file.

@aristath
Copy link
Member

Thank you for the clarification!

@hellofromtonya
Copy link
Contributor Author

You're welcome. Good question and observation! And thanks for reviewing this PR @aristath

To capture the site editor identification condition in one place.

Why?

To improve readability and maintability, especially if the identification
logic changes in the future.
@ironprogrammer
Copy link
Contributor

ironprogrammer commented Apr 10, 2023

Thanks for the PR, @hellofromtonya! 🙌🏻

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 (with patch applied from PR description)
    • gutenberg v15.5.0

Actual Results

  • ✅ In the block editor, <style id="wp-fonts-jetpack-google-fonts"> does not exist in the editor iframe.
  • ✅ In the site editor, <style id="wp-fonts-jetpack-google-fonts"> does exist in the editor iframe, and contains all fonts registered by the test plugin.
  • ✅ In either editor, <style id="wp-fonts-local"> exists with the theme's default registered fonts.

@aristath aristath merged commit d39f350 into trunk Apr 11, 2023
@aristath aristath deleted the fix/render-enqueued-fonts-block-editor-iframe branch April 11, 2023 05:56
@github-actions github-actions bot added this to the Gutenberg 15.6 milestone Apr 11, 2023
@bph bph added [Type] Bug An existing feature does not function as intended Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release labels Apr 11, 2023
@Mamaduka
Copy link
Member

@hellofromtonya, does this need to be backported into the next WP minor release?

@hellofromtonya
Copy link
Contributor Author

hellofromtonya commented Apr 13, 2023

does this need to be backported into the next WP minor release?

@Mamaduka For WP 6.2, no this area of the code is not in the 6.2 release.

What about the next Gutenberg release? Let's wait, ie don't ship yet. Why? To avoid breaking sites that are using Google Fonts via Jetpack's package as there's a bug in that package preventing user-selected (global styles) fonts from enqueuing and rendering.

I'm thinking its best to ship this PR's fix when the #40363 feature enhancement is also committed into trunk. That way all plugins and classic themes (without a theme.json file) will continue to work.

@hellofromtonya hellofromtonya removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Apr 13, 2023
@hellofromtonya hellofromtonya removed this from the Gutenberg 15.6 milestone Apr 13, 2023
@hellofromtonya
Copy link
Contributor Author

I've removed this PR from backporting to WP 6.2 minor. Why? This code is not in 6.2 or WP Core. Why? The Fonts API is still experimental and has not yet been backported to WP Core.

I also removed this PR from the next GB release. Why? It needs to be bundled with the #40363 feature enhancement which (when implemented) will automatically enqueue all user selected fonts, selected from the Site Editor's Global Typography UI. Why? One of the most popular Google Fonts implementation has a bug in it where its fonts will not enqueue. The bug was hidden until this PR fixed the Fonts API for the iframe assets. Delaying the shipment of this PR's fix avoids breaking sites.

@Mamaduka
Copy link
Member

@hellofromtonya, thanks for the clarification. I'm double-checking PR marked for the next WP minor release, and PR had the label.

As for the Gutenberg release, this was included in the next version's RC, so we'll need to exclude it manually. If the PR is blocked by #40363, then we can create "double-revert" PRs (one to remove from the trunk and one to add back) and use the label "Blocked."

Cc-ing @ndiego and @bph as they're coordinating the release.

@hellofromtonya hellofromtonya added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Apr 17, 2023
hellofromtonya added a commit that referenced this pull request Apr 17, 2023
@hellofromtonya
Copy link
Contributor Author

Thank you @Mamaduka. Yes, I do see it was included in 15.6 RC1. #49863 will revert this PR from trunk and thus will require a RC2. Added the labels to the revert PR cc @bph @ndiego

Sorry for the confusion and delays (was out sick last week). In hindsight, I should have added noted its dependency in the description and marked the PR as blocked to avoid it being committed.

hellofromtonya pushed a commit that referenced this pull request Apr 17, 2023
@hellofromtonya
Copy link
Contributor Author

Note: The revert has been committed into trunk. Once its dependency is resolved, then a revert of the revert (or another PR for reinstate this PR's commit) will be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Blocked Used to indicate that a current effort isn't able to move forward [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fonts API] All registered fonts are being rendered outside of the Site Editor
5 participants