-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add/webfonts loading #1573
Add/webfonts loading #1573
Conversation
940c1f5
to
598cff3
Compare
5853496
to
1f6f2f6
Compare
Note: As discussed on https://wordpress.slack.com/archives/C02QB2JS7/p1631135661385600 we should be able to register a local webfont from a |
3629fbd
to
ff72d4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some additional feedback on the fonts provider implementation. I'm still leaning towards it making more sense to register a fonts provider outside of the wp_register_webfont
/ wp_enqueue_webfont
functions and then in those calls only referencing a registered provider. Breaking out the registration may especially make sense given my other point below, that provider classes should also be able to "orchestrate" multiple fonts together.
$remote_url = $this->build_api_url(); | ||
$transient_name = 'google_fonts_' . md5( $remote_url ); | ||
$css = get_site_transient( $transient_name ); | ||
|
||
// Get remote response and cache the CSS if it hasn't been cached already. | ||
if ( ! $css ) { | ||
// Get the remote URL contents. | ||
$response = wp_remote_get( | ||
$remote_url, | ||
array( | ||
// Use a modern user-agent, to get woff2 files. | ||
'user-agent' => 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0', | ||
) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to fetch the CSS and include it inline for API-based web fonts? Would that be a notable performance benefit over using the recommended link
tag, given the extra code and maintenance cost?
I'm particularly wary about expecting and modifying certain code to be in the CSS returned from the URL, which is happening below. We don't control that CSS, so I think it'd be safer to only load the URL somehow and not do any tweaks on it.
* @param array $params The webfont's parameters. | ||
* @return void | ||
*/ | ||
public function set_params( $params ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the WP_Fonts_Provider
should tie in at a higher level than per individual font. Right now, it is limited to returning CSS per font, but the reality for some font providers is that they can cover multiple fonts in one, which e.g. would result in fewer requests needed if let's say a site enqueues 5 different Google fonts. Currently this results in 5 requests, but it could be combined into 1.
I think it would be more appropriate for a fonts provider class to be able to process multiple font declarations in combination if the respective API supports it (every fonts provider could have a flag like supports_multiple
or something). For example, instead of get_css()
returning data based on the $params
for a single font, it should be able to return data based on the $params
for multiple fonts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that there should be a way to collect like fonts and then make one external request. This is something @aristath and I have been talking about here. And it can be included in Phase 1 of this API.
return; | ||
} | ||
|
||
return wp_enqueue_style( "webfont-$handle" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using WP_Styles
here under the hood is an elegant solution, but at the same time it may not be as versatile as we need, particularly related to my other point where it would be good to be able to combine the requests to font APIs for multiple fonts into one.
So maybe we need a WP_Webfonts
class (that could probably extend WP_Styles
) after all 🤔
* @access public | ||
* @since 5.9.0 | ||
* @param array $params The webfont's parameters. | ||
* @return void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit-pick, but WP core standards don't expect a @return void
, in that case the tag should be omitted.
ff72d4b
to
a06fe06
Compare
My first impressions are:
I propose we start with the end in mind (i.e. The discussion is captured here in Ari's fork. Feedback is welcomed and encouraged. If consensus, then the next version of this API (for Phase 1) can be built rather quickly for review. |
Which part you mean is too complex? I'm envisioning the API to be the same as what's proposed in #4. Registering a webfont provider isn't something that a theme author would typically be expected to do. I think core should provide the common webfont providers (e.g. local, Google, Adobe) out of the box, so registration would only be necessary when using a less popular or entirely custom provider (let's say if a company had their own fonts CDN). Maybe that has been unclear in my comment before, but registering a webfont provider wouldn't be required as long as you use one of the common ones that WP core would provide. My proposed approach would facilitate the same API that is currently descibed in #4, where when registering a webfont you simply need to specify a
Could you clarify what is "Phase 1"? If we want to implement certain parts later, we should still ensure the architecture we decide on now will allow for that. Specifically about webfont provider registration, I agree we could also build the registration part later, which would mean that in the beginning you would really only be able to use the webfont providers that WP core implements out of the box. |
I agree. Core should have built-in providers that it registers. Theme authors would then specify which provider via its associated ID such as For custom things, a custom provider can be registered as you suggested previously.
With the new proposal being discussed, it aligns better to the future need while reducing code and complexity for theme authors.
I agree. That's the vision of the proposal. The theme defines all of the fonts it'll use and for each font, it associates a slug or ID for the provider. Core will expose built-in providers. Custom providers can be built and registered.
|
The currently proposed functions
That would never be necessary. When calling the function, no provider needs to be instantiated, only the identifier for a core default provider would be passed. Registration of a provider wouldn't be necessary either unless it's a custom one.
As far as I've seen based on the latest API iteration (also see https://core.trac.wordpress.org/ticket/46370#comment:40), I don't think the differentiation between |
That's a great question. I had thoughts here where we're initially discussing the new proposal. Overview of Phase 1:
This includes:
What's not included:
I agree. What gets built now should be a stepping stone towards the future. |
Imagine one entry into the Webfonts API that serves plugins, block themes with a Let's think about by starting with the end in mind, i.e. The configuration of webfonts will part of the This could then lead to one API function for registration that handles registering webfonts from multi-sources including themes with a Let's see that in action. This PR requires the theme or plugin to register / enqueue like this: // Passing a URL as the src.
wp_enqueue_webfont( 'roboto', 'https://fonts.googleapis.com/css2?family=Roboto:ital@0;1&display=swap' );
wp_enqueue_webfont(
'my-font-normal-400',
'',
array(
'font-family' => 'My Font',
'src' => array(
get_template_directory_uri() . '/fonts/font.woff2',
get_template_directory_uri() . '/fonts/font.woff',
),
'provider' => new WP_Fonts_Provider_Local(),
)
); or like this: wp_enqueue_webfont(
'roboto-400',
'',
array(
'fontFamily' => 'Roboto',
'fontStyle' => 'normal',
'fontWeight' => '400',
'provider' => new WP_Fonts_Provider_Google(),
)
);
wp_enqueue_webfont(
'roboto-italic-400',
'',
array(
'fontFamily' => 'Roboto',
'fontStyle' => 'italic',
'fontWeight' => '400',
'provider' => new WP_Fonts_Provider_Google(),
)
);
wp_enqueue_webfont(
'my-font-normal-400',
'',
array(
'font-family' => 'My Font',
'src' => array(
get_template_directory_uri() . '/fonts/font.woff2',
get_template_directory_uri() . '/fonts/font.woff',
),
'provider' => new WP_Fonts_Provider_Local(),
)
); With the new proposal, it can simplified to this: wp_register_webfont(
array(
'roboto-normal-400' => array(
'fontFamily' => 'Roboto',
'fontStyle' => 'normal',
'fontWeight' => '400',
'provider' => 'google',
),
'roboto-italic-400' => array(
'fontFamily' => 'Roboto',
'fontStyle' => 'italic',
'fontWeight' => '400',
'provider' => 'google',
),
'my-font-normal-400' => array(
'fontFamily' => 'My Font',
'fontDisplay' => 'swap',
'fontStyle' => 'normal',
'fontWeight' => '400',
'src' => array(
get_template_directory_uri() . '/fonts/font.woff2',
get_template_directory_uri() . '/fonts/font.woff',
),
'provider' => 'local',
),
)
); |
Notice in the last example how the array follows the |
To my above question, is that still relevant given the latest iteration supports font providers? I think it'd be more appropriate to register e.g. Google fonts by registering a font like in the other examples and specifying I agree with pretty much everything you're saying otherwise. Regarding your last example though ( |
I agree with you. I think we want to standardize on the params structure and not an optional Why? It could make collecting the like fonts a little bit easier rather than parsing separate URLs that have been registered by a theme and/or plugin.
Why better? A couple of things come to my mind:
|
+1 to both of that. I'd be leaning towards having a higher-level |
+1 Agreed! I'll start a new branch on Ari's fork with this new approach and optimized architecture. Why? To retain this PoC PRs history and implementation. Also plan to include a suite of unit/integration tests. Once ready, performance benchmarks will be needed too. |
Closing this PR as it has been replaced by #1736 |
This PR adds functions for a webfonts API and is a first iteration covering the basics, something we can build upon in future patches to add more features and expand on what we do here.
wp_register_webfont
wp_deregister_webfont
wp_enqueue_webfont
wp_dequeue_webfont
wp_webfont_is
wp_webfont_add_data
The syntax of all these functions is identical to their
style
counterparts, sowp_register_webfont
is the same aswp_register_style
and so on. The only difference is the use of$params
in lieu of$deps
for practical reasons (see below)Notes:
webfont-
prefix$deps
argument was replaced wirth$params
. These params can be used to register a webfont from local files, and auto-generates the CSS for@font-face
.In addition to the above functions, this PR also adds a
wp_webfont_generate_styles
function. This accepts an array of params, and returns the generated CSS for the webfoot.Example 1: enqueuing a webfont from google-fonts
Example 2: Getting the styles for a google-font and attaching them to an existing stylesheet:
Example 3: enqueueing a webfont from local files
Registering a webfont from an API with CSS files
Most APIs provide CSS for their webfonts. Registering a webfont in this manner is easy as we can simply call
wp_enqueue_webfont( $handle, $url );
. No extra args are required, this is the simplest scenario.Registering a webfont from local files
To register a webfont from local files, we can use the
$params
arg. This is formatted as an array and accepts all valid CSS props of@font-face
as its array keys. Any extra args are ignored. The list of valid descriptors was taken from MDN.Using a
font-family
is mandatory, and skipping that results in no CSS getting generated.The
src
If we only want to define a single file for the webfont, then we can add it as a string (
'src' => $url
).If we have multiple files for the webfont (different formats to support older browsers), then we can use an array (
'src' => [ $url1, $url2 ]
). In this case, the URLs get internally reordered for browser support (woff2, woff, ttf, eot, otf). SVG for webfonts is not supported because they have been deprecated (see caniuse.com/svg-fonts), so if provided it gets removed (like any other invalid type).Note: The src can also accept data-urls.
Variable fonts
The
font-variation-settings
property accepts either a string (normal
), or, an array of key/value pairs (e.g.,["wght" => 637, "wdth" => 100]
), and returns a string of these values (e.g.,wght 637, wdth 100
).This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.