Blockbase: Register Fonts via Webfonts API instead of theme.json #6794

jffng · 2022-12-21T19:53:44Z

Changes proposed in this Pull Request:

This PR changes the way Blockbase registers fonts, from theme.json to use the Webfonts API directly instead.

This is done so that we can conditionally load them — only if Jetpack fonts are not active.

To test:

Checkout this PR, activate Blockbase locally (no other plugins active), ensure custom fonts load as expected in the editors and front-end:

Checkout this version of blockbase onto a wpcom simple site (you can use npm run deploy:push:blockbase, verify there are no duplicates and you see all the Jetpack fonts, e.g. with Zoologist:

Related issue(s):

Solves #6765, alternative to #6777 which failed for reasons I still can't quite identify.

More reasoning why

Quoting @simison here for why I went with this approach instead:

Although I think we should still just disable Blockbase fonts entirely when Jetpack fonts module is active instead of doing this kind of "merge both sources" stuff, as Jetpack fonts:

works with all themes and experience is hence more consistent especially considering theme switches,

is a single source that's easier to keep up to date. Blockbase seems to already have outdated fonts list. Current either/or setup is way too complex, and this PR is actually making it even more complicated (merged lists) although it does fix an issue.

Jetpack fonts are now loaded from WP.com instead of Google

jffng · 2022-12-21T19:54:37Z

blockbase/inc/fonts/custom-fonts.php

+	}
 }
+
+add_action( 'init', 'enqueue_global_styles_fonts', 100 );


Note I just moved all action and filter registrations to the bottom since it seems to be more of a convention to do this way.

jffng · 2022-12-21T19:55:05Z

blockbase/functions.php

+/**
+ * Do not use the custom fonts provided by Blockbase if Jetpack fonts module is active
+ */
+if ( ! class_exists( 'Jetpack' ) || ! Jetpack::is_module_active( 'google-fonts' ) ) {


@simison is this a safe way to check?

I think so! but let's @Automattic/jetpack-crew for 2nd opinion

Clever doing module check instead of "is dotcom" check 👍

Yes, it should work. The module will always be considered as "active" on WordPress.com simple, and is force-activated on WoA, so you'll end up using the module on both platforms at all times.

I tested this PR in .com simple sites and the file didn't get loaded. 👍

Is there any chance that the font module will become part of a standalone plugin at some point? If so, it might be better to do it like this:

Suggested change

if ( ! class_exists( 'Jetpack' ) || ! Jetpack::is_module_active( 'google-fonts' ) ) {

if ( ! class_exists( \Automattic\Jetpack\Modules::class ) || ! ( new \Automattic\Jetpack\Modules() )->is_active( 'google-fonts' ) ) {

(feel free to put a use Automattic\Jetpack\Modules; at the top of the file instead of using a fully-qualified class name here, of course).

jffng · 2022-12-21T20:02:14Z

Ah I just realized this is going to break Blockbase fonts for sites that are not running Gutenberg which exposes the public webfonts API currently.

jeffikus · 2022-12-21T20:39:57Z

@jffng I like this approach. I especially like that you've separate out the json - that seems more efficient.

About Blockbase breaking for sites not running Gutenberg, I remember discussing our assumption was that Blockbase was assuming Gutenberg was active, but gracefully degrades when it isn't, or am I remembering that incorrectly?

simison · 2022-12-21T21:29:26Z

blockbase/inc/fonts/custom-fonts.php

+					'font-display' => 'fallback',
+					'provider'     => $font_family['provider'],
+				),
+			)


FYI using wp_register_webfonts with this array signature is fine today, we'll just need to migrate to new format once the new API stabilizes, as this exact format got deprecated.

More in WordPress/gutenberg#43492

Gotcha. If this approach is acceptable, we can open a follow-up PR to change the format and merge it after the new API lands in Gutenberg 14.9.

Yep exactly. We'll also need to migrate Jetpack Automattic/jetpack#28054

More info here: Automattic/jetpack#28063

jffng · 2022-12-21T21:40:36Z

@jeffikus

About Blockbase breaking for sites not running Gutenberg, I remember discussing our assumption was that Blockbase was assuming Gutenberg was active, but gracefully degrades when it isn't, or am I remembering that incorrectly?

That sounds right, by this assumption do you think it's acceptable to only have the system font available?

Note that this would only be until the public Webfonts API lands in Core.

jeffikus · 2022-12-21T22:17:02Z

@jffng I am comfortable with that being the assumption in order to get the best out of the theme. System fonts seems a good fallback on .org - .com won't have that problem though.

@mikachan @ianstewart @matiasbenedetto @vcanales @madhusudhand @scruffian @MaggieCabrera any objections to this assumption?

@jffng we would also need to test this with the Blockbase premium themes and merge it across to the premium version of blockbase. But that's a separate implementation. Let's get the main Blockbase implementation complete first.

simison · 2022-12-22T08:57:33Z

do you think it's acceptable to only have the system font available?

Sort of related to adding more safe web fonts in Gutenberg WordPress/gutenberg#39552

mikachan · 2022-12-22T10:49:36Z

any objections to this assumption?

I like this approach, all sounds good to me 👍

jffng · 2022-12-22T17:37:44Z

@jeffikus were you able to test this, locally or on dotcom?

simison · 2022-12-27T15:52:40Z

blockbase/theme.json

 					"name": "System Font"
-				},
-				{
-					"fontFamily": "Arvo, serif",


Since these fonts were Arvo, serif previously here, will sites continue functioning if the font now is registered with just Arvo in global styles?

Yes, since the slug is just Arvo and thus generated CSS variable that applies the font is the same.

simison · 2022-12-27T15:54:10Z

blockbase/theme.json

-					"provider": "blockbase-fonts"
-				},
-				{
-					"fontFamily": "'Helvetica Neue','Helvetica', 'Arial', sans-serif",


Since these Helvetica+Arial fonts are from the OS, should you keep them here?

Oh yeah, we should keep this in theme.json.

Done in 6bf79ff

simison · 2022-12-27T15:57:15Z

were you able to test this, locally or on dotcom?

I did test on .com simple sites — works fine (#6794 (comment)). Just a bit concerned if old sites continue working (https://github.com/Automattic/themes/pull/6794/files#r1057756452).

pbking · 2023-01-05T13:32:59Z

About Blockbase breaking for sites not running Gutenberg, I remember discussing our assumption was that Blockbase was assuming Gutenberg was active, but gracefully degrades when it isn't, or am I remembering that incorrectly?

That sounds right, by this assumption do you think it's acceptable to only have the system font available?

While that's the assumption I also have this scenario feels different in that the additional fonts ARE currently available with Blockbase (or any of the children) without Gutenberg activated. So removing that functionality without waiting for an opportunity to refactor it seems like breakage rather than a degradation.

jffng · 2023-01-09T02:31:47Z

@pbking that's a fair point. This approach would break fonts for Blockbase + children that are not running the plugin, until the refactor webfonts API lands in core. And we don't really know what the surface area of sites that really affects is.

without waiting for an opportunity to refactor it

Do you have any other ideas for how we might refactor this in the meantime? I do not know why #6777 did not work.

pbking · 2023-01-11T14:02:30Z

@jffng the only alternative I can think of that doesn't negatively impact non-plugin-users is to add the fonts to Blockbase. I have opened a change to do that here: #6820

hellofromtonya · 2023-01-18T13:48:01Z

Heads up:
The Fonts API architectural work is now complete.

The API renaming work merged today into Gutenberg trunk. For example, wp_register_webfonts() is now wp_register_fonts().
Registering fonts through a theme's theme.json file is NOT impacted as the APIs automatically ingest, process, register, and enqueue defined fonts.
Directly registering fonts via wp_register_fonts(): the data structure to register changed:
- Variations are grouped to their font-family
- The font-family is the key for this group of variations

For example:

foreach ( $fonts_to_register as $font_family ) {
	wp_register_fonts(
		array(
			$font_family => array(
				array(
					'font-family'  => $font_family,
					'font-weight'  => '100 900',
					'font-style'   => 'normal',
					'font-display' => 'fallback',
					'provider'     => 'jetpack-google-fonts',
				),
				array(
					'font-family'  => $font_family,
					'font-weight'  => '100 900',
					'font-style'   => 'italic',
					'font-display' => 'fallback',
					'provider'     => 'jetpack-google-fonts',
				),
			),
		)
	);
}

Also added notes to Jetpack's Google Fonts Provider Automattic/jetpack#28063 (comment).

jffng · 2023-01-19T01:41:35Z

Amazing, thank you for the update @hellofromtonya! The plan is for this updated API to make it into 6.2, right?

hellofromtonya · 2023-01-19T11:31:52Z

The plan is for this updated API to make it into 6.2, right?

Yes, hopeful for it to land in WP 6.2 🤞 @jffng

pbking · 2023-01-23T20:54:33Z

I would like to ship this change (or some version of it), but not until 6.2 lands. Until then #6831 should get wpcom where it needs to be.

jffng added 2 commits December 21, 2022 11:34

Register custom fonts conditionally.

31bce23

Change and to or.

77295cd

jffng added the [Theme] Blockbase label Dec 21, 2022

jffng commented Dec 21, 2022

View reviewed changes

jffng requested review from a team and simison December 21, 2022 19:55

simison reviewed Dec 21, 2022

View reviewed changes

Simplify font list.

f10111e

simison mentioned this pull request Dec 24, 2022

[Webfonts] Upgrade the Google Fonts Provider to use the new Fonts API Automattic/jetpack#28063

Closed

simison reviewed Dec 27, 2022

View reviewed changes

Add back Helvetica Neue.

6bf79ff

pbking changed the title ~~Blockbase: defer to Jetpack fonts~~ Blockbase: Register Fonts via Webfonts API instead of theme.json Mar 3, 2023

	if ( ! class_exists( 'Jetpack' ) \|\| ! Jetpack::is_module_active( 'google-fonts' ) ) {
	if ( ! class_exists( \Automattic\Jetpack\Modules::class ) \|\| ! ( new \Automattic\Jetpack\Modules() )->is_active( 'google-fonts' ) ) {

Blockbase: Register Fonts via Webfonts API instead of theme.json #6794

Are you sure you want to change the base?

Blockbase: Register Fonts via Webfonts API instead of theme.json #6794

Uh oh!

Conversation

jffng commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Related issue(s):

More reasoning why

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jffng commented Dec 21, 2022

Uh oh!

jeffikus commented Dec 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jffng Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simison Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jffng commented Dec 21, 2022

Uh oh!

jeffikus commented Dec 21, 2022

Uh oh!

simison commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikachan commented Dec 22, 2022

Uh oh!

jffng commented Dec 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jffng Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simison commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbking commented Jan 5, 2023

Uh oh!

jffng commented Jan 9, 2023

Uh oh!

pbking commented Jan 11, 2023

Uh oh!

hellofromtonya commented Jan 18, 2023

Uh oh!

jffng commented Jan 19, 2023

Uh oh!

hellofromtonya commented Jan 19, 2023

Uh oh!

pbking commented Jan 23, 2023

Uh oh!

Reviewers

jffng commented Dec 21, 2022 •

edited

Loading

jffng Dec 21, 2022 •

edited

Loading

simison Dec 22, 2022 •

edited

Loading

simison commented Dec 22, 2022 •

edited

Loading

jffng Jan 9, 2023 •

edited

Loading

simison commented Dec 27, 2022 •

edited

Loading