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

Webfonts: remove unused font-face declarations #39332

Closed
creativecoder opened this issue Mar 10, 2022 · 16 comments
Closed

Webfonts: remove unused font-face declarations #39332

creativecoder opened this issue Mar 10, 2022 · 16 comments
Assignees

Comments

@creativecoder
Copy link
Contributor

creativecoder commented Mar 10, 2022

What problem does this address?

The initial Webfonts API adds @font-face declarations for every font-family that is registered, regardless if it is used in the css for the page, or not.

With a few fonts, this is not too bad, but with additional font weights and styles, it can quickly add up. One example: Jetpack is adding a Google fonts module, and with ~30 Google fonts registered, there are over 1500 font-face declarations, adding well over 500 KB (uncompressed) to the page source.

What is your proposed solution?

Remove unused font-face declarations on each page to reduce the size of what is added to the page source.

Related tasks

@creativecoder
Copy link
Contributor Author

creativecoder commented Mar 10, 2022

@zaguiini has approached this in a couple of different ways, so far. See #39036, #39327 and #39099.

I wanted to have this issue as a hub for discussing the problem and potential solutions.

@zaguiini
Copy link
Member

zaguiini commented Mar 11, 2022

After several iterations, I believe this issue is near the finish line. It's in good state but I'd really appreciate feedback.

We have 3 PRs:

#39361

This PR changes how the internals of the wp_webfonts() singleton works by allowing instance reset. This is necessary so the tests can run in isolation and be deterministic. Without it, tests in the following PRs will always fail.

#39327

This changes how the Webfonts API works a bit by allowing webfont registration without immediately enqueueing.

The change comes in the mindset, where instead of having to filter out unused webfonts, we're including the fonts we want to actually see in the front-end. This allows for more predictable behavior on which fonts are included in the output. It's more extensible and WP-developer friendly as well, since it follows common patterns defined by wp_styles() and wp_scripts().

It does not maintain backward compatibility with how wp_register_webfont used to work since it's not yet released to production, so breaking changes are allowed.

#39399

This is the PR actually includes the registered webfonts in the output. It does so by scanning (and caching) content through all standardized post types in WordPress, including global styles, templates, template parts, posts, pages, and widgets.

@mtias
Copy link
Member

mtias commented Mar 15, 2022

I think we need to clarify the use cases a bit to determine the best course of action. For example, a user should be able to register a set of fonts they like and see them as options in a UI like global styles, font pairs, etc. But only the fonts the user has assigned within their theme.json representation should be generating front-end enqueues. Does that seem reasonable? Are there other use cases missing? I'd say that the font-family selector of blocks should only be showing the set that comes from global styles, not every registered font.

cc @aristath @hellofromtonya

@mtias mtias added the [Priority] High Used to indicate top priority items that need quick attention label Mar 15, 2022
@zaguiini
Copy link
Member

Hi @mtias,

Yeah, all use-cases you described are covered.

We are also aiming to let plugin developers use the API through the familiar wp_register and wp_enqueue APIs, so we're taking a mindset where instead of registering + enqueueing a font to the front-end at the same time, this process is made separately so they can have more control on what's exactly outputted.

@jeyip
Copy link
Contributor

jeyip commented Mar 21, 2022

Status Updates

#39361 Change class properties from static to instance members (Merged)

#39327 Expose enqueueing method instead of directly enqueueing fonts on registration

We've since added a few updates and addressed all code review.

  1. We returned to auto-generation of webfont ids (instead of user provided ids).
  2. We also moved src attribute URL processing of local fonts to the local provider.

We're waiting on further code review.

#39399 Scan content for webfonts and enqueue them

This PR is ready for further review. While waiting on feedback, @zaguiini drafted a PR linked below that takes an alternative approach to enqueue only the fonts that are used in the frontend.

#39559 Scan content for webfonts and enqueue them (Attempt #2)

This PR is an alternative to #39399. Instead of scanning all content on save and caching it, we instead scan blocks in the pre-render hook as well as the global styles object, and only enqueue fonts for blocks that are displayed in the frontend.

cc @aristath

@zaguiini
Copy link
Member

zaguiini commented Mar 22, 2022

Status Update

Separation between registration and enqueueing

The first version of the Webfonts API is a big monolith -- we're registering a font face and directly emitting the registering font family in which the font face belongs to the front-end, regardless if they're being used or not. That's why this issue exists in the first place: it quickly adds up in the size of the file that's sent to the front-end.

The second version allows separation between registration (fonts that show up in the editor and can be picked) and enqueueing (fonts that actually end up being emitted in the front-end). It does so by exposing wp_register_webfont and wp_enqueue_webfont methods.

The wp_register_webfont method accepts a webfont object, a font face for a certain font family. It then registers font faces for the said font family. By this time, the font is ready to be picked in the editor at will by the user. Once a font is registered, its font family can be enqueued with wp_enqueue_webfont, which accepts a font family string (say Roboto). When called, the method will make this font family ready to show up in the front-end as inlined CSS.

The new approach lives here: #39559.

Webfont scanning

Webfonts picked in the editor end up as block metadata. On page load, the pre_render_block filter runs for each rendered block and we're intercepting that looking for used font families. If it's using a font family that was registered using the Webfonts API, we enqueue it to the front-end. That happens implicitly as wp_enqueue_webfont was not called programmatically: it's a side effect of the block metadata scanning.

Its code lives here: #39593.

Performance impact and caching

In previous iterations of this issue, we were with a different mindset of scanning the group of blocks -- namely post types such as wp_post, wp_page, wp_template, and others. This proved to be expensive, computationally speaking. So much that we were relying on caching techniques so the toll was mitigated.

This introduced fragile, hard-to-read, and reason about solutions, both for the people writing (@jeyip and I, mostly) and reviewers. @creativecoder then came with similar concerns and we agreed that it was not good enough. That's when we decided to go the pre_render_block route. From a technical point of view, it's a much simpler solution -- it's easier to think about it and we're not digging too much into implementation details and registered post types.

Instead of scanning (introducing new logic over template fetching and parsing) we're now hooking into a process that already happens (the pre_render_block filter), whether we use it or not this filter always runs. Because of that, we're letting go of caching strategies since it'd be incredibly hard, if not impossible, to cache block rendering and we're not even sure that this is a thing.

"But Luis, wouldn't that greatly impact the server processing?"

Not really. On our tests, it added about 20 ms on every page load in a very constrained Docker container, which looks like a totally viable tradeoff considering the simplicity of the new solution. You can compare:

pre_render_block approach template and post parsing with caching
image image
Average running time: 725ms Average running time: 733ms

trunk runs on 712ms on average.

It is much much simpler, and less fragile since pre_render_block will run for every post type that the front-end uses, be it a template, template part, post, page, widget, navigation, and so on. You can imagine that'd be a nightmare to maintain should Gutenberg include more post types. This approach greatly simplifies that.

Fonts from theme.json parsing

Regarding global styles, @creativecoder raised a valid concern when trying to get the slug from font families used in the theme.json file: we're relying on string parsing through a regular expression, and that's as fragile as it gets.

Ideally, the font family slug would be exposed through some property (like fontSlug) when used, instead of referencing a CSS variable. The current state of it is understandable and works well for the front-end, but introduces us to this problem when trying to parse global styles.

One option would be to include the fontSlug property when referencing a webfont registered through theme.json, this way removing that fragile maintenance point. We could then deprecate the fontFamily attribute and only use fontSlug.

Let me exemplify that: we have source-serif-pro, which is registered in theme.json. That font is later referenced in several block presets, such as https://github.com/WordPress/twentytwentytwo/blob/trunk/theme.json#L224. It is done like:

"fontFamily": "var(--wp--preset--font-family--source-serif-pro)",

And to get that information inside the Webfonts API, we need to use a RegEx like font-family--(?P<slug>.+)\)$/'. The problem with that is that the way we reference registered fonts might change in the future, thus this solution could easily break. What sounds better would be to add a property called fontSlug which would serve the same purpose, but only contain the font family slug. Something like:

"fontSlug": "source-serif-pro"

We could introduce that new property and deprecate fontFamily, so when all themes are using the new property, it'd be possible to remove fontFamily and rely solely on fontSlug.

Separation of concerns within the WP_Webfonts class

With every iteration over the Webfonts API, we're noticing the WP_Webfonts class, a does-it-all class, getting bigger and bigger. We're thinking of separating between:

  • WP_Webfonts, the class which would orchestrate the whole Webfonts API;
  • WP_Webfont, which would hold a webfont family and all its font faces;
  • WP_Webfont_Registry, which would be in charge of grouping webfonts.

There are possibly other ways of making the WP_Webfonts class do less, such as including a class like WP_Webfont_Printer which would be responsible of reading the enqueued webfont registry and printing the styles, removing a lot of the burden of the WP_Webonts class. Anyway, there's food for thought there... But we're already working on separation of concerns in https://github.com/WordPress/gutenberg/tree/try/separate-concerns-wp-webofnts.

@creativecoder
Copy link
Contributor Author

I've been thinking a lot about our approach here while testing the related PRs over the past week+. Here are some thoughts on the implementation:

Font enqueuing for block typography settings should happen at the block level

The font family settings for blocks are provided by the block supports typography module (https://github.com/WordPress/gutenberg/blob/3739bd76e236ff6ed5431022dbd5684fc7f93ade/lib/block-supports/typography.php). I think that anything related to how font settings are structured, serialized/deserialized, etc should stay contained in that module, and helper functions added, if needed, for accessing settings.

The fact that blocks are stored in various post types (posts, pages, template parts, etc) is incidental, and the font enqueuing logic shouldn't need to be aware of this. If we can handle enqueuing used fonts during block rendering, that seems best. Enqueuing block styles/scripts might be analogous, ideally assets that are needed are only enqueued when we've confirmed a block is being rendered on the page, and the rendering of the block itself should trigger that process.

It looks like the newest approach in #39593 starts to address this, using the pre_render_block hook.

theme.json needs an option for enqueuing vs registering fonts

As we've seen from comments on the initial PRs (#39036 (comment)), there are good use cases for both registering and enqueuing fonts from theme.json. Sometimes you want that font-family to be there for use in the theme's stylesheet, or a specific block may be missing typography settings you want to apply manually with css.

I think we'll need to add a property that forces enqueuing the font from theme.json when the theme author wants to guarantee that font is available, even if not used in a block setting or Global styles.

The WP_Webfonts class should not be aware of how font settings are stored

With every iteration over the Webfonts API, we're noticing the WP_Webfonts class, a does-it-all class, getting bigger and bigger.

Agreed! I think this class' responsibility should be limited to registering/enqueuing fonts and aggregating styles from font providers, and not concerned at all with where or how fonts are stored. For example, I think enqueuing fonts from Global styles settings and hooking into block rendering should be handled elsewhere. We may not need a class for this, as it could be handled by utility functions.

Similarly, any filters that are added should not happen here, both to separate concerns, but also because plugins could create additional instances of this class which would then duplicate those hooks and run the functions multiple times unnecessarily.

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Mar 24, 2022

I'm concerned about the some of the proposed changes in the Webfonts API as it's being shifted away from being source agnostic to being solely specific for only theme.json themes. The original intent of this API is for it to become a Core API that supports web fonts regardless of if they are registered by a theme.json powered theme, a legacy theme, or a plugin(s).

The API IMO should not be aware of the theme.json or global styles settings or tasks. That specific knowledge and how-to is better encapsulated within the theme.json parser/handler, global styles handler, and/or a new service that preps before registration (sits in between).

For example, if there are customizations from the global styles that need to be registered within the Webfonts API, the discovery work should be done elsewhere and then the array of webfonts arguments (its configuration) registered with the API.

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Mar 24, 2022

Awareness: Webfonts API will grow into more than only supporting local font file assets. It will have bundled remote font factory / foundry providers too such as Google Fonts.

I raise this point for awareness as improvements made need to should not block additional providers being bundled within Core or added by extenders.

@zaguiini
Copy link
Member

zaguiini commented Mar 28, 2022

Thanks a lot for the comments, @hellofromtonya! Progress update:

Register and enqueue webfonts by font-family #39559

The PR is ready for review and ready to be merged after approval.

Handle font face conflicts

When registering through several providers, or really just different font faces for the same family, we're using its slug (derived from the font family name) as the aggregator.

The problem is that with multiple providers, the same font face might get registered twice.

To address that, we're thinking about wrapping the webfont array into a WP_Webfont class, which would encapsulate a font face declaration. This class would have a method like is_equal, which would be called by the registry, and then we'd determine whether to override or to add the font face to its list.

Scan content for font families and enqueue them #39593

This PR will be reworked so we can move the logic from the WP_Webfonts and collocate it where it makes more sense. I do agree that we want to minimize the work done in WP_Webfonts and want it to act as an aggregator of providers and webfonts.

Register font faces through wp_enqueue_webfont

It's handy to register and enqueue a webfont through wp_enqueue_webfont by passing the webfont object. We thought about adding this functionality in #39559 but we soon realized it is not only out of the scope of the PR, but also a little bit more complex than initially thought. We're doing that in a separate PR.

Selectively enqueue font faces of a font family

A great idea proposed by @mattwiebe. Webfonts are a little different than styles. Let's say you have 7 weights to Roboto, but you're using a single one in the editor. It doesn't make sense to include all weights. We need a way to allow the enqueueing of a single font face, meaning you can include in the front-end only the font faces you're actually using, ignoring the other registered weights. This falls out of the scope of both PRs proposed right now, and it feels less critical at this moment, so we'll focus on delivering the first two PRs and then think about this.

Lazy loading of webfonts in the editor

Enqueueing all fonts so they're available in the editor circles back to the original problem. It's a necessary evil for now, but I'm thinking about how could we mitigate that as well. The most obvious solution right now is to scan the content, just like we do in the front-end, for the immediately-needed webfonts (i.e. the ones that should load when the editor boots up). Then, we need to expose the font itself in the picker, but only the slug/name. Right after they're picked, we should dispatch a request to a REST API endpoint which would give us the font-face declaration for the picked font family, and that declaration would be inserted into the page and the font would get loaded. That's one option, and there's a lot of room for thought here.

@aristath
Copy link
Member

With a few fonts, this is not too bad, but with additional font weights and styles, it can quickly add up. One example: Jetpack is adding a Google fonts module, and with ~30 Google fonts registered, there are over 1500 font-face declarations, adding well over 500 KB (uncompressed) to the page source.

That is a valid concern, but at the same time, perhaps Jetpack should NOT be doing that?? Why does a plugin need to register 30 webfonts? Isn't there a better way for the plugin to do this?
Instead of refactoring the implementation here to accommodate a scenario like the one described, perhaps the scenario itself should be reconsidered?
If a random plugin registers 10 scripts that it doesn't use, we're not going to refactor the WP_Scripts API...

@mtias
Copy link
Member

mtias commented Mar 30, 2022

@aristath I think the use case is less about what Jetpack might be doing and more about having the ability for a user or theme to manage fonts in their libraries without incurring a penalty on the front-end. For example, a theme might have a couple styles with different font pairs, and it'd be a nice experience to be able to switch between the styles on the fly in the admin editor (all registered fonts accessible) while minimizing the ones that get enqueued on the front just to those that are actually used. Same if we allow users to upload font assets so they can manage their fonts and switch between them easily. It's not that disimilar to blocks registering their scripts and all getting enqueued in the editor but not necessarily on the front.

@aristath
Copy link
Member

Thank you for the extra context @mtias 👍

@mtias
Copy link
Member

mtias commented Mar 31, 2022

I think I'm fully caught up with the developments now and wanted to summarize a few things:

  • The main goal is to avoid loading every registered font on the front-end while still loading them in the site editor (back-end) so people can choose and manage them.
  • On the front we should enqueue only the fonts that theme.json / user styles are using, or those that a theme enqueues directly (using wp_enqueue_webfont).
  • It's not necessary now to go all the way towards inspecting what blocks are actually using as instances, nor to async load fonts in the editor. Those optimizations can follow up later.

I think this is a pretty straightforward path. The main thing to clarify is that theme.json declares the fonts that are actually active among all those that might be registered. A user should be able to swap theme.json files (style variations) and get a different set of fonts, but a single theme.json won't be registering multiple presets of fonts.

@zaguiini
Copy link
Member

zaguiini commented Apr 1, 2022

Right. Time for the action plan for theme.json fonts registration + enqueueing!

Problem

We want to enqueue font families referenced within theme.json[settings][typography][fontFamilies] and programmatically through the Webfonts API. It should also be possible to register fonts directly through the same path inside the theme.json file.

The theme.json file should declare which fonts should be enqueued from the set of fonts that are registered, either programmatically or inside theme.json.

It is possible, and that's expected, to enqueue fonts by using the wp_enqueue_webfont method directly. What we're trying to do here is to give theme developers a way of picking only a subset of the registered fonts to show up in the front-end, but without needing to dig down into PHP.

Proposed solution

All the following snippets are inserted in settings.typography.fontFamilies[] inside the theme.json file.

Enqueue a font family

{
  "fontFamily": "Roboto",
  "slug": "roboto",
  "name": "Roboto"
},

This declaration would enqueue all font faces registered under the Roboto font family.

Enqueueing a subset of a font family

{
  "fontFamily": "Roboto",
  "slug": "roboto",
  "name": "Roboto",
  "fontFaces": [
    {
      "fontFamily": "Roboto",
      "fontStyle": "normal",
      "fontStretch": "normal",
      "fontWeight": "400"
    }
  ]
}

Specifying the fontFaces property would allow selective filtering of the font faces that are registered.

All of the snippets above consider the font family being registered programmatically, through the Webfonts API.

Registering and enqueueing font faces through a single provider

Adding the provider property on the root level would register and enqueue all font faces described in fontFaces.

{
  "fontFamily": "Roboto",
  "provider": "local",
  "slug": "roboto",
  "name": "Roboto",
  "fontFaces": [
    {
      "fontFamily": "Roboto",
      "fontStyle": "normal",
      "fontStretch": "normal",
      "fontWeight": "400",
      "src": [ "file:./assets/fonts/Roboto-Regular.ttf" ]
    }
  ]
}

In this case, The local provider would take care of handling all Roboto font faces.

Registering and enqueueing font faces through multiple providers

{
  "fontFamily": "Roboto",
  "slug": "roboto",
  "name": "Roboto",
  "fontFaces": [
    {
      "provider": "local",
      "fontFamily": "Roboto",
      "fontStyle": "normal",
      "fontStretch": "normal",
      "fontWeight": "400",
      "src": [ "file:./assets/fonts/Roboto-Regular.ttf" ]
    },
    {
      "provider": "google-fonts",
      "fontFamily": "Roboto",
      "fontStyle": "bold",
      "fontStretch": "normal",
      "fontWeight": "700",
      "src": [ "file:./assets/fonts/Roboto-Bold.ttf" ]
    }
  ]
}

Specifying provider at the font face level would allow different providers to provide for the same font family.

Questions

How to differentiate between fonts that should be handled by the Webfonts API vs fonts that should not

It might be hard to differentiate between fonts that should be handled by the Webfonts API and fonts that are just system fonts. For example:

{
  "fontFamily": "\"DM Sans\", sans-serif",
  "fontSlug": "dm-sans",
  "slug": "heading-font",
  "name": "Headings (DM Sans)",
  "google": "family=DM+Sans:ital,wght@0,400;0,500;0,700;1,400;1,500;1,700"
},
{
  "fontFamily": "Roboto",
  "slug": "roboto",
  "name": "Roboto"
},

Ignoring the google property there in the first block, they're exactly the same. There's no way to tell which fonts should be handled by the API and which ones should not.

My first idea was to add a property like "external": true which would tell that this font is registered somewhere else and should be handled by the API. Ideally, we'd omit that information and make the Webfonts API transparent to the theme.json file.

But I also do not like implicit things. I think being explicit, especially in those cases, helps the developer understand what's missing when debugging or, when something fails, they'll know exactly what failed, or where the font is coming from!

But, if we do want to be implicit and do not add the property... We can mark the font as external when running the gutenberg_add_registered_webfonts_to_theme_json by comparing the registered fonts vs the fonts that are in theme.json. We could then flag those fonts and only work upon them. This is just one option, though, and I'm all open to suggestions!


Prefix Webfonts API attributes in theme.json

Should we prefix Webfonts API attributes? We have provider and (maybe) external. How about adding a prefix so there's no name clashing in the future? Something like webfonts:provider or webfonts:external. It's easy to understand and would clarify a lot about who is using this property.


Additional comments

With those changes, a thing that really excites me is the possibility to enqueue a single font face if needed. I did a draft in my local machine and I came up with:

wp_enqueue_webfont(
	'Roboto',
	array(
		'fontFamily' => 'Roboto',
		'fontWeight' => 700,
		'fontStyle' = > 'bold', 
	)
);

And that's so good! I'm really looking forward to that. Example usage:

function gutenberg_enqueue_webfonts_from_theme_json() {
	$theme_settings = WP_Theme_JSON_Resolver_Gutenberg::get_theme_data()->get_settings();

	// Bail out early if there are no settings for webfonts.
	if ( empty( $theme_settings['typography'] ) || empty( $theme_settings['typography']['fontFamilies'] ) ) {
		return;
	}

	// Look for fontFamilies.
	foreach ( $theme_settings['typography']['fontFamilies'] as $font_families ) {
		foreach ( $font_families as $font_family ) {
			// If no font faces defined.
			if ( ! isset( $font_family['fontFaces'] ) ) {
				// Enqueue the entire family.
				wp_webfonts()->enqueue_webfont( $font_family );
				continue;
			}

			// Loop through all the font faces, enqueueing each one of them.
			foreach ( $font_family['fontFaces'] as $font_face ) {
				wp_webfonts()->enqueue_webfont( $font_family, $font_face );
			}
		}
	}
}

add_filter( 'wp_loaded', 'gutenberg_enqueue_webfonts_from_theme_json' );

Of course, this is just a draft, but it highlights the idea of enqueueing fonts registered programmatically. This could be changed to include guards that would register font faces that are explicitly declared in theme.json, like in the snippet above where I'm passing fontFaces[].src!

I transformed those thoughts in a PR: #39988.

@bph bph added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 14, 2022
@hellofromtonya hellofromtonya removed the [Priority] High Used to indicate top priority items that need quick attention label Jun 23, 2022
@hellofromtonya hellofromtonya removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jan 18, 2023
@hellofromtonya
Copy link
Contributor

The Fonts API is now completely designed from when this issue was opened. It now extends from and reuses Core's WP_Dependencies. As such, the data structures and strategies are vastly different from the scope of what this issue is tracking.

Some of the linked work here are good enhancements. Given the changes in the API, each of the enhancements will need separate consideration. They no longer fit well into the scope of this issue.

Rather, I think they fit better as separate enhancements within the Fonts API Ongoing Roadmap #41479.

I'm closing this issue. Thank you everyone for your contributions 👏 Amazing work!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants