-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 API - webfonts collection implementation #1736
Conversation
ca8f410
to
18f7807
Compare
Styles for webfonts now get properly enqueued in the editor. |
src/wp-includes/webfonts-api/providers/class-wp-webfonts-local-provider.php
Outdated
Show resolved
Hide resolved
007ef64
to
1d02ed4
Compare
5ca9d8d
to
91d18ad
Compare
src/wp-includes/webfonts-api/providers/class-wp-webfonts-google-provider.php
Outdated
Show resolved
Hide resolved
Been looking at this patch for couple of days and thinking it may be going a bit too far/in a wrong direction. Few things:
|
Uh, sorry about the long comment above. I'm also a bit worried that this is far too complex, not only as "code design" but also as usage. What would be the compelling reason a theme would prefer to switch to using this API rather than continue to hard-code loading of the web fonts it needs, like it has been doing for the last five years or so? |
It's less code for themes and plugins. How so? Without this API, themes and plugins:
With this API, themes and plugins:
See this action 👀Take a look at the new TT2 theme's PR 95:
This PR is an example of how plugins and non- Note: Once this API is committed, then What are benefits for theme and plugin authors and extenders? 🚀
|
The validation code protects users and provides debug help for extenders. For performance:
What's the purpose of it? Benefits
Yes that's possible. |
This architecture is designed to:
How so? I hadn't yet detailed the architecture of what each piece does and why. Now is a good time. Let's start with the pieces that are needed and then move to have each class is delivering those pieces. The Pieces NeededFor the webfonts (
For the providers (
For the API (
For local or external fetching and CSS (the providers):
The Registry
An in-memory storage mechanism is needed for the API to collect and then organize the optimizations to be done. Instead of register and processing each individual webfont, the goal is to organize the collections for each provider in such a way that each provider can do its work in an optimum way without duplicating the sorting process. The knowledge of how to work with the webfonts or providers is contained within their respective registry. As each has an in-memory container to house the registered items, further optimizations can be done. These registries provide the mechanisms to handle the work of registering, preparing, verifying, and querying. That know-how is contained inside of each of these registries. Benefits:
The ProvidersThe provider is a processor that encapsulates the business logic of how to generate the Benefits:
The ControllerPattern: Controller The Controller is the glue. It's the manager directly the flow of work between the pieces and keeping the coupling between to a minimum. Benefits:
|
It's doing a lot more than those highly specific 40 lines of code you mentioned are frequently copied/pasted into themes and plugins. (See the details above of all that is delivers.) How much code does it add to Core? @azaozz do you still feel it's overengineered? If yes, what ideas do you have to simplify it and still deliver everything it needs to do? |
This is not the end of the implementation, it is the beginning. Before we discuss privacy concerns, loading remote files etc, we need to have a base we can build on and improve. wordpress-develop/src/wp-includes/class.wp-webfonts.php Lines 108 to 249 in 381ae55
|
Users have the freedom to use this API however they want... If the user downloads non-free webfont files, then bundles them in their theme and registers them using this API, the CSS we generate is not an issue. The issue is the font-files they used. |
2de948d
to
c99f96f
Compare
c99f96f
to
6a211c8
Compare
Testing instructions for webfonts registration added in https://core.trac.wordpress.org/ticket/46370#comment:66 |
1c02d77
to
e79b46d
Compare
A filter is provided to grant permission to do remote requests to external service providers. By default, this filter is set to `false`, meaning the API will not do remote requests or generate `@font-face` styles for external service providers. Removes the `is_external()` logic as all providers are external except for the local provider bundled in core. Removes the `is-external` parameter from the webfonts schema and validation code.
Further documents the API with examples. Includes code review fixes.
2ab1403
to
aa08030
Compare
@felixarntz Sorry for the delay in responding. Have some health problems.
Right. I believe this API, like all other larger/more complex new features in WordPress, would benefit from being a feature plugin and get tested in all sorts of environments, staging, etc. That includes testing at very large sites, multi-site/network installs, etc.
Correct, that's a similar case like with the REST API. Feature plugins can be targeted at extenders too, not just at site admins. The alternative would be to add a brand new, untested, potentially unrefined API which would "commit" WP to support it "forever" even if there are large problems with it (I don't think there are, but it is a possibility). And it is not an MVP, it adds a lot of more advanced features. Hence my recommendation to be on the cautious side, and I believe the patch was not merged following that recommendation. Also identified two "blockers" (documentation, user privacy) that were fixed in time. |
@aristath Again, sorry about the delay, have some health problems at the moment.
Specifically asked about Adobe as they have probably the largest fonts library and guessing they are the top candidate for being added by plugins that want to extend the WP webfonts API. Also their APIs work in a pretty different way. As far as I see they expect the users to have an account on their website and to manage the webfonts features from there. Here are some examples in this tutorial: https://helpx.adobe.com/fonts/using/add-fonts-website.html. As far as I see it works by providing an URL that doesn't contain any fonts information, just points to the user's account on their site. So all the fonts settings that are required by this patch are not available. As far as I see there is no way to make this work with the current patch. Perhaps if the patch was more extensible a plugin may be able to bypass most of the current requirements and enable use of Adobe fonts, but still looks like a pretty hard thing to do.
I'm actually still wondering about having the "providers" registration in the first place. Could you perhaps tell me how did you come up with that idea, to separate the font URL into two parts and what benefits that brings? At the moment, no matter from what angle I try to look at it, it seems that the "providers" abstraction is not needed/not performing a particular function. Having the fonts registered together with the providers (i.e. the providers are part of the font registration) seems more feasible. That would also reduce the current complexity quite a lot, and make the API easier to use. Another idea/concern/enhancement would be to separate the font settings that are for use by other parts of WP (like the editor) and the actual URL used to get the font(s). For example looking at how Google serves webfonts, having to "extract" the settings from the URL that is generated by the Google API, then enter them as PHP array key/value pairs, then the API would try to regenerate the original URL as given by the remote API from the settings is not the best way for this to work imho. Perhaps a better solution would be to distinguish between "functional" webfonts settings (as needed for the webfont to work) and settings intended for other parts of WP. Then a theme or plugin will be able to expose some of the settings to the editor UI, but keep others "hidden" from the users: a lot better control and a lot simpler/safer as the original third-party API URL will not be disassembled by the plugin author and then reassembled by WP. Please note: this is just an idea, not a "blocker" or anything like that :) |
I'm sorry to hear than Andrew, I hope things go well and you feel better soon ❤️
I'm sorry but I'm not going to try and fix the way the Adobe API works. If Adobe wants, they can build a class themselves - or fix their API to be more friendly/reasonable.
The benefit is consistency. All webfonts can be defined the same way - regardless of whether they are local, google-fonts, or something else. This is necessary so that webfonts can be defined in a
That's a false premise... You're missing a step here, and that step is the very 1st one: The user chooses a webfont they want to use. Users don't "extract the settings from the generated URL to then enter them as PHP array key/value pairs"... Users don't have a URL in their mind when they want to use the Open-Sans font with weights 400 & 700. They start with what they want to use which is a font-family, some font-weights etc. Instead of going to the google-fonts (or any other API) to generate a URL, they just enter what they want as key/value pairs - either in PHP or JSON in their Currently:
With the Webfonts API:
I don't see how the current, unmanaged version is any better than a more managed solution which doesn't require users to go to a 3rd-party site, search for webfonts, then get a URL and manually handle adding it everywhere.
What would be a non-functional webfont-setting exactly? The API simply uses |
Thank you!
Hehe, of course not. But not being able to use the Adobe fonts API/CDN with the WordPress webfonts API seems like big drawback. Themes and plugins should be able to do that. On the other hand looking at the other font foundries that provide webfonts, most would let you download the font and host it locally (whether is it paid or free to use). There is also https://fontsource.org/ and its repo: https://github.com/fontsource/fontsource that seem to contain the fonts that are available from Google. Then the consideration becomes: does WordPress want to facilitate usage of third party webfonts APIs/CDNs at this stage? Seems quite better to make the WP API work only with local fonts for now. This makes it quite simpler, and also resolves any concerns about user privacy issues. Also removes the necessity of integration with a future "user consent API" that may be added later. In that case not being able to extend the API to use Adobe webfonts CDNs for now seems appropriate. Frankly I'm starting to think this is the proper way forward.
Hmm, yes, I can see that but generally a webfont is defined by its URL. Third-party/remote webfonts would have absolute URLs, local fonts would probably need to have relative URLs. This is the only difference, and is really "easy to handle". Don't think the abstraction is needed there. Furthermore this abstraction seems to only be used when using the Google CDN. Considering the above thoughts (only local fonts for now) think the need for abstracting the fonts "sources" disappears completely.
Okay, this only concerns using the Google webfonts API/CDN so probably not that important for now. When you select a webfont there (with few options) you're given an URL like this: I understand that the different options that were selected have to be "known" so they can be used by the editor UI for example. My suggestion in this particular case was to separate the "working URL as given by Google" from the options that are presented to the WP UI. This is something to maybe examine more closely in the future if WordPress decides to add support for using the Google CDN. At that time a decision would have to be made about using the Adobe CDN too. |
Re-posting this here from WordPress/gutenberg#36394 (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:
Advantages of using local webfonts (from Fontsource):
In these terms my recommendations are:
|
The Webfonts API has moved to Gutenberg (see first implementation in WordPress/gutenberg#36394). Closing this PR, thought it will remain available for the ongoing work in Gutenberg. |
I'm still not convinced the Webfonts API should be "merged" with Gutenberg. It is a separate API after all. Another reason is that it still may benefit from being made into a feature plugin (no point to bypass the WP development process). On the other hand if it is significantly simpler and more straightforward, perhaps it would be okay to "keep it" in Gutenberg as that would make it a bit easier to test with the block editor. Ideally it should be a plugin though, just like any other new WP feature that needs more testing. |
This API solves the problem of inefficient font loading which is a known "performance bottleneck" that can impact the user experience.
What the API Provides
This API provides:
preconnect
linksWhat the API does
Exposes a standard way for
theme.json
parser, themes withouttheme.json
, and plugins to register their webfonts usingwp_register_webfonts()
Allows for different ways to get the webfont file: from an external font service or locally hosted within a theme or plugin
google
: handles requesting from the Google Fonts servicelocal
: handles locally hosted webfont filesValidates what is registered:
For required configuration parameters: validates the keys exist and the values are of the data type required. Why?
For optional configuration parameters: checks if key ones are valid, if not, sets a default. Why?
Orchestrates webfonts organization:
- doing a single external API roundtrip (when requesting an external font service), instead of individual fetches
- adding
preconnect
linksValidation (more on this below)
Benefits for extenders:
theme.json
, in a theme's PHP, or in a pluginBenefits for users:
Trac ticket: https://core.trac.wordpress.org/ticket/46370
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.