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

Implementing a Webfonts API #36394

Closed
wants to merge 34 commits into from
Closed

Implementing a Webfonts API #36394

wants to merge 34 commits into from

Conversation

aristath
Copy link
Member

@aristath aristath commented Nov 11, 2021

Description

This PR ports port the webfonts API to Gutenberg:

Porting the PR from Core:

Ports the API from WordPress/wordpress-develop#1736.

That patch was thoroughly discussed with core & theme developers and approved by core committers (@felixarntz, @hellofromtonya, @SergeyBiryukov), but the API was not merged in core on time for WP 5.9.
It was deemed a better choice to port the API to Gutenberg, where it can be implemented for theme.json and the font-family pickers (see comment on https://core.trac.wordpress.org/ticket/46370#comment:111)

Note: Only the local provider is ported - which is the default. Other providers (such as google-fonts etc) can be discussed and implemented in additional, separate pull-requests.

theme.json implementation

Following the comment by @jorgefilipecosta on #35591 (comment), the theme.json implementation was placed under settings.typography.fontFamilies.[].fontFace:

{
	"settings": {
		"typography": {
			"fontFamilies": [
				{
					"fontFamily": "\"Source Serif Pro\", serif",
					"name": "Source Serif Pro",
					"slug": "source-serif-pro",
					"fontFace": [
						{
							"fontFamily": "Source Serif Pro",
							"fontWeight": "200 900",
							"fontStyle": "normal",
							"fontStretch": "normal",
							"src": [ "file:./assets/fonts/SourceSerif4Variable-Roman.ttf.woff2" ],
						},
						{
							"fontFamily": "Source Serif Pro",
							"fontWeight": "200 900",
							"fontStyle": "italic",
							"fontStretch": "normal",
							"src": [ "file:./assets/fonts/SourceSerif4Variable-Italic.ttf.woff2" ],
						}
					]
				}
			]
		}
	}
}

Any webfonts registered via theme.json or PHP will appear in the font-family pickers.

To test registering webfonts via PHP and confirm they are correctly added to the editor you can use the snippet below:

wp_register_webfonts(
	array(
		array(
			'font-family'  => 'Source Serif Pro',
			'font-weight'  => '200 900',
			'font-style'   => 'normal',
			'font-stretch' => 'normal',
			'src'          => array( 'file:./assets/fonts/SourceSerif4Variable-Roman.ttf.woff2' ),
		),
		array(
			'font-family'  => 'Source Serif Pro',
			'font-weight'  => '200 900',
			'font-style'   => 'italic',
			'font-stretch' => 'normal',
			'src'          => array( 'file:./assets/fonts/SourceSerif4Variable-Italic.ttf.woff2' ),
		),
	)
);

How has this been tested?

  • Lots of automated tests in the PR
  • Registered webfonts both via the theme.json file and PHP using the snippets above, then confirmed the following:
    • Webfonts get added to the font-family selectors in the editor
    • Selected webfonts are properly assigned and rendered in the editor
    • Selected webfonts are properly assigned and rendered on the frontend
    • CSS vars and styles are properly added both in the editor and the frontend

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@aristath aristath force-pushed the add/webfonts branch 2 times, most recently from ba075b3 to 4f02f12 Compare November 18, 2021 12:48
@aristath aristath force-pushed the add/webfonts branch 6 times, most recently from c0d08ad to 23ee03e Compare November 22, 2021 13:26
@aristath aristath changed the title WIP - webfonts API Implementing a Webfonts API Nov 24, 2021
@aristath aristath marked this pull request as ready for review November 24, 2021 07:20
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 The global styles API is not ready yet to add filters. If the goal is to register webfonts that are present in the theme theme.json, can we do that directly in the resolver?

@aristath
Copy link
Member Author

Thank you for the review @oandregal!

👋 The global styles API is not ready yet to add filters. If the goal is to register webfonts that are present in the theme theme.json, can we do that directly in the resolver?

Done 👍

@azaozz
Copy link
Contributor

azaozz commented Nov 29, 2021

Posting this here and will post at WordPress/wordpress-develop#1736 as well (having 2 PRs for the same thing is hard/confusing...).

Been looking at this implementation for a while. It seems that it can be made a lot better by removing the "providers" abstraction. Few reasons:

  • It will remove several possible (theoretical) edge cases that are a concern: registering the same font from multiple providers, different plugins registering the same "provider" (conflicting settings?), etc.

  • Seems that the "providers" abstraction can only be used to add support for the Google webfonts API/CDN. Seems the Adobe API/CDN cannot be made to work with this implementation as it expects the user to manage all features from the Adobe website (doesn't pass font settings in the URL like for the Google's CDN).

  • All webfonts available from the Google's API/CDN seem to also be available for downloading/including from https://fontsource.org/ and their repo at https://github.com/fontsource/fontsource as NPM packages.

Advantages of using local webfonts (from Fontsource):

  • Self-hosting brings significant performance gains as loading fonts from hosted services, such as Google Fonts, lead to an extra (render blocking) network request. To provide perspective, for simple websites it has been seen to double visual load times. Benchmarks can be found here and here.

  • Fonts remain version locked. Google often pushes updates to their fonts without notice, which may interfere with your live production projects. Manage your fonts like any other NPM dependency.

  • Commit to privacy. Google does track the usage of their fonts and for those who are extremely privacy concerned, self-hosting is an alternative.

  • Your fonts load offline. On top of benefiting PWAs, often there may be situations, like working in an airplane or train, leaving you stranded without access to your online hosted fonts. Have the ability to keep working under any circumstance.

  • Support for fonts outside the Google Font ecosystem. This repository is constantly evolving with other Open Source fonts. Feel free to contribute!

In these terms my recommendations are:

  1. Add support only for local fonts for now. If WordPress decides to include support for the Google CDN later on, the implementation will have to consider web privacy laws and restrictions and be tied with an eventual User Consent API, etc.
  2. Because of the above the "providers" abstraction can be removed as it is not needed.
  3. The "WP webfont settings validation" functionality (a.k.a. "schema" in the patch) should be moved out of the low-level API and into a separate function. The API should only check parameter types (to prevent fatal errors as it seems this may be broken in new versions of PHP). However regex is not needed to check/set function param types, that is redundant there.

@azaozz
Copy link
Contributor

azaozz commented Nov 29, 2021

Just FYI: still thinking this will benefit from being a feature plugin, just like any new WP feature that needs more testing. Having it merged with the Gutenberg's PHP code may help a little with testing it from Gutenberg, but can prevent testing from outside of Gutenberg...

In any case, if this API is simplified significantly (as recommended above), perhaps it would be okay to bypass the standard WP development process, and any errors and omissions can be fixed later.

@oandregal oandregal dismissed their stale review November 30, 2021 10:33

changes requested were applied

@azaozz
Copy link
Contributor

azaozz commented Dec 6, 2021

@aristath Hi, would you be able to make the changes as suggested above (remove the "providers" abstraction and all associated code, hard-code local webfonts, and move param type validation to the actual function WP_Webfonts_Registry::register()) before the holidays?

@aristath aristath mentioned this pull request Dec 6, 2021
7 tasks
@aristath
Copy link
Member Author

aristath commented Dec 6, 2021

It will remove several possible (theoretical) edge cases that are a concern: registering the same font from multiple providers, different plugins registering the same "provider" (conflicting settings?), etc.

That is no different than what happens in other WP APIs. The same script can be registered from multiple sources/CDNs, it can be registered from multiple plugins and so on. Same thing applies to styles.

Seems that the "providers" abstraction can only be used to add support for the Google webfonts API/CDN. Seems the Adobe API/CDN cannot be made to work with this implementation as it expects the user to manage all features from the Adobe website (doesn't pass font settings in the URL like for the Google's CDN).

Actually, the Adobe API can be used just fine, as it supported the Typekit API: https://fonts.adobe.com/docs/api. There's even some ready-to-use PHP libraries.
Removing the providers implementation would significantly limit the extensibility of the API and make it harder to extend in the future. We're not only building something we need right now, we need to look ahead and pave the way for the future.

Advantages of using local webfonts (from Fontsource):

Self-hosting brings significant performance gains as loading fonts from hosted services, such as Google Fonts, lead to an extra (render blocking) network request. To provide perspective, for simple websites it has been seen to double visual load times. Benchmarks can be found here and here.

Fonts remain version locked. Google often pushes updates to their fonts without notice, which may interfere with your live production projects. Manage your fonts like any other NPM dependency.

Commit to privacy. Google does track the usage of their fonts and for those who are extremely privacy concerned, self-hosting is an alternative.

Your fonts load offline. On top of benefiting PWAs, often there may be situations, like working in an airplane or train, leaving you stranded without access to your online hosted fonts. Have the ability to keep working under any circumstance.

Support for fonts outside the Google Font ecosystem. This repository is constantly evolving with other Open Source fonts. Feel free to contribute!

I agree 100% with all those points. But that doesn't mean we need to remove the providers abstraction, we can just do better and download remove webfonts to the server, and serve them locally from there.
In fact, I agree so much with the above that this was part of the initial implementation and was then removed due to complexity concerns. It's still visible in the code on WordPress/wordpress-develop#1736 (comment), and we already have that same implementation as a package that themes can use on https://github.com/WPTT/webfont-loader.
Using local webfonts does not exclude interacting with 3rd-party sites on the server-side, and bringing back that implementation is an easy task should we choose to do it.

Add support only for local fonts for now. If WordPress decides to include support for the Google CDN later on, the implementation will have to consider web privacy laws and restrictions and be tied with an eventual User Consent API, etc.

The Google fonts class was removed from this PR 2 weeks ago 👍

Because of the above the "providers" abstraction can be removed as it is not needed.

Removing the providers abstraction would make it impossible to add & extend the implementation in the future to support other providers like Adobe, Google and others, either in plugins or in core. Instead of removing it, maybe we could implement them properly, enforcing locally-hosted webfonts to improve performance & privacy? This way we'd be setting a good example, and we'd see a significant performance & privacy improvement in the WP ecosystem as themes & plugins that currently use Google-fonts, Adobe-fonts and whatnot will start to adopt the API.

The "WP webfont settings validation" functionality (a.k.a. "schema" in the patch) should be moved out of the low-level API and into a separate function. The API should only check parameter types (to prevent fatal errors as it seems this may be broken in new versions of PHP). However regex is not needed to check/set function param types, that is redundant there.

I submitted an alternative, simplified version of the API in #37140.
The functionality is identical, but the diff is a lot smaller. It's not better or worse, it's just a different implementation. If you prefer going that route we can do that.


EDIT:
After giving this some more thought, I thought that If we want to enforce locally-hosted webfonts - even on 3rd-party APIs, then after a webfont gets downloaded it's basically a "local" webfont, at which point it can use this API directly, passing the correct arguments for the locally-hosted version of the webfonts.
With that in mind, I removed the providers implementation from #37140 and added 2 filters in its place to facilitate extending the API for use by 3rd-party providers (1 filter when registering the arguments of a webfont, and a 2nd filter when generating the styles). That should be sufficient for most implementation and I'll be creating a Google-fonts class to verify that this can still work (see commit dd7f2fb).
However... that did not work as expected, so I reverted it. The problem is that 3rd-party APIs optimize delivery and performance by using multiple files for each variation of a webfont, with separate unicode-ranges - and that can't be accomplished unless there is a separate provider implementation. A single webfont does not equal a single @font-face.
Leaving the "provider" implementation will ensure the API can be properly extended and used, otherwise, it's just inadequate to handle anything beyond hardcoded webfonts and therefore not future-proof.

@azaozz
Copy link
Contributor

azaozz commented Dec 7, 2021

@aristath Thanks for the tests and alternate PRs! Really appreciate your work on this :)

It will remove several possible (theoretical) edge cases

That is no different than what happens in other WP APIs.

Correct, but why have more edge cases when can have less or none? :)

Actually, the Adobe API can be used just fine, as it supported the Typekit API.

Hmmm, seems that API is not intended for serving fonts as a CDN:

"The Typekit API gives you programmatic access to the functionality of Typekit over a RESTful HTTP based API. It allows you to programmatically create, modify and publish kits, and fetch metadata about all the fonts in the Typekit library."

It seems intended for an Adobe user to access their Adobe account and change their (fonts) settings. However don't think that is intended to be happening on every page load of the user's website. In any case, seems best if this functionality (loading webfonts from third-party CDNs) is implemented later, ideally after WP has a User Consent API in place.

Removing the providers implementation would significantly limit the extensibility of the API
Not so sure about that. Perhaps we can try to figure out what fails when using a filter, or perhaps the code needed for the Google API can be hard-coded (when the time comes).

Still thinking that for now it will be best to support local fonts only which removes the need for the providers abstraction. That includes all fonts available from the Google CDN. They can be downloaded from https://fontsource.org/ or included as NPM packages (which is very handy for a lot of developers).

This means that "out of the box" WordPress will support all webfonts that are available form Google plus some more open source fonts that are available only from Fontsource, plus all fonts that are purchased/downloaded from one of the fonts foundries. Unfortunately it seems support for the Adobe webfonts would have to wait for now as it seems impossible to download the webfonts you have purchased (I think I've read somewhere that it is possible to download them, but cannot find it now).

Looking again at the list of advantages for using local webfonts, in practice the providers abstraction will only be needed to let plugins to add support for the Adobe webfonts CDN (if at all possible). It seems the best practice would be to use local copies of the webfonts that are available from the Google CDN.

If at some point WP decides to support external CDNs that abstraction (or the filters replacing it) can be added to this API as an update. As I mentioned before this should ideally happen after there is a User Consent API in place so accessing the external webfonts CDN(s) can be made to require user consent.

@azaozz
Copy link
Contributor

azaozz commented Dec 7, 2021

we can just do better and download remove webfonts to the server, and serve them locally from there.

Yeah, thought about that too but not sure it will be possible for Adobe fonts. Can try to figure out if their licenses would allow it. Also don't think it is needed for Google fonts. A (much) better option would be to include these fronts as NPM dependencies or download them directly from https://github.com/fontsource/fontsource and add them in the theme/plugin.

@aristath
Copy link
Member Author

aristath commented Dec 8, 2021

A (much) better option would be to include these fronts as NPM dependencies or download them directly from https://github.com/fontsource/fontsource and add them in the theme/plugin.

I agree that would be better. However, that assumes that the theme/plugin has a single font they want to use - or at most a couple of them. That is not a viable option in the wild because themes allow users to make their own decisions.
A theme can allow users to pick one of 20 fonts, in which case they would have to include all font-files for all available fonts. That in turn would make the theme size grow exponentially just for the sake of including font-files... and it would mean that themes will not be uploadable to the w.org repo.
By downloading locally only the files that the user actually needs, we can keep the theme size to a minimum, reduce wasted server disk space and provide a better experience to end-users.
Limiting the API won't make a difference to developers building one-off sites or agencies building client sites, but it will hurt mom-and-pop sites where the user builds their own site, picks a theme, and expects things to work with the typography they want to use.

@azaozz
Copy link
Contributor

azaozz commented Dec 8, 2021

A theme can allow users to pick one of 20 fonts, in which case they would have to include all font-files for all available fonts.

Yes, you're right, that's a lot of extra files.

A theme can allow users to pick one of 20 fonts, in which case they would have to include all font-files for all available fonts.

Right. This is a good optimization. It seems to have limitations that may need a "deeper look" (mostly around font licenses, etc. when using proprietary fonts for example from Adobe which might need to be excluded at first). Perhaps this can be the next step for this API. Another idea there would be to (auto) download the font files from the GitHub repo, not from the Google CDN. Yet another idea would be to let users upload fonts the same way they upload other media files (with or without using the WP uploader and/or the media library).

There is another optimization/enhancement that needs addressing (perhaps at a later time too). It concerns both this API and any UI for selecting fonts or font settings. Generally the "typography" fonts are the big, multi-file fonts that support all locales. However many decorative fonts support only a limited number of locales. So both this API and the UI will need to be made "locale aware", and themes and plugins that add fonts should "know" how to "do it properly" for all locales.

For now I'm thinking a possible nice enhancement may be to include few of the "big", most commonly used open source fonts in WordPress. Quite a few people probably remember that wp-admin used to use Open Sans several years ago. Eventually it was switched to use the system fonts for better speed and because of user privacy concerns. But perhaps it's time to add few fonts and switch to using one of then in wp-admin again? :)

@jeyip
Copy link
Contributor

jeyip commented Jan 28, 2022

Hi @aristath! I was only able to skim this PR and will take a closer look tomorrow, but I'm wondering if there are any blockers for continuing work on these code changes? The only sticking point I saw after reading some of the conversations dealt with the addition / removal of the provider abstraction.

cc @zaguiini

@aristath
Copy link
Member Author

Hey there @jeyip!
There is an alternative PR on #37140 that simplifies the providers abstraction and a lot of the code here. That PR is not better or worse, it's just a different code structure.
The only blocker right now is that after 5.9, we have no way to hook in global-styles (see comment on #37140 (review))
The webfonts API itself is fine and works great, but we currently can't hook in global-styles, and as a result of that, if a webfont is registered using the wp_register_webfonts function instead of using theme.json, the additional webfonts are not visible in the font-family selector in the editor.

@aristath
Copy link
Member Author

aristath commented Mar 1, 2022

Closing this one, as #37140 was merged

1 similar comment
@aristath
Copy link
Member Author

aristath commented Mar 1, 2022

Closing this one, as #37140 was merged

@aristath aristath closed this Mar 1, 2022
@aristath aristath closed this Mar 1, 2022
@aristath aristath deleted the add/webfonts branch March 1, 2022 06:18
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

Successfully merging this pull request may close these issues.

5 participants