-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: expose enqueueing method instead of directly enqueueing fonts on registration #39327
Conversation
@aristath As Luis and I were refactoring the Webfonts resemble scripts and styles, and we can't see why we wouldn't use it. Could you elaborate? |
The initial implementation of the Webfonts API was actually using a class extending It's exactly the same for global-styles... Conceptually, global-styles should extend Both global-styles and webfonts are standalone APIs that use styles, but don't need to extend some other API. |
Thanks, @aristath, and that makes a lot of sense! One question, though:
Do you think it's true? One might want to include font B only if font A has been registered... That might happen. Also, fonts definitely have dependents so I wonder if the |
🤔 I can't think of a scenario where we wouldn't want to load font B if font A has not been loaded... Or a scenario where something would depend on whether a webfont has been loaded or not. Can you please elaborate? A practical example might help me understand! The way I see it, users should be able to get the content and continue being able to use the page as if everything is fine, even if a webfont for some reason doesn't load. Worst-case scenario, they will see the content with a system-font and not the designer's font of choice... But that's not necessarily a bad thing: If a webfont doesn't load it will be because for example they are on an extremely slow connection, or force their browser to ignore all custom font-families and use their own due to a11y. |
Maybe I went too far with it 😅 This could be achieved in the future if needed, anyways! Thanks, @aristath! Any observations about this PR? We thought about making the API look more WP-ish by adding |
ceaf7e9
to
6bcd1b0
Compare
74b8d4d
to
da0cfe6
Compare
da0cfe6
to
8a53646
Compare
8a53646
to
953d6e9
Compare
Question: Scripts & styles have enqueue and register methods, because they have dependencies. So the concept is that we register everything, and then enqueue things as needed based on the dependencies tree. |
Hey @aristath, thanks for the messages!
The idea is that developers provide their own hashing mechanism so they can understand how creating an ID works -- since they're creating their own. I do believe that adding the So, now that we're separating registration from enqueueing, I do think it makes sense for the developers to come with their own IDs, so that they can register and enqueue the fonts as they need instead of relying on an abstract mechanism.
As we discussed previously, yes, there are no dependencies between fonts. The reason we decided to go that route is solely based on API similarity: we are registering resources and then enqueueing them as needed. I do not see a reason to treat webfonts differently: we are registering so the users can pick them up in the editor, and enqueueing them as they're picked. The API is vastly similar to what WP developers are already used to: |
be66425
to
7b4900a
Compare
@aristath Thanks for the prompt replies. As we continued working on this PR, we were wondering about a few more things: Why we do parse the font-face
|
@carolinan @creativecoder @spacedmonkey @TimothyBJacobs We'd love your input on this PR -- it stands to solve potential performance problems users may have in the future and has significant ramifications on how the webfonts API will evolve. |
f763e10
to
e497a0c
Compare
We're now placing the logic to transform Apart from being simple separation of concerns, there is an implication: a webfont enqueueing might happen after its theme.json registration. Referencing #39399's use case, we want to enqueue the webfonts that are used in the content, and that's pretty late in the process -- almost before rendering. Because of that, the URL transformation wouldn't get applied. So it's been moved to the provider. The reason why it's done here and not in the other PR is that I don't want to deliver this PR in a broken state where enqueued webfonts are not shown in the front-end because of the wrong URL. |
e497a0c
to
5ab6da7
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.
This approach makes sense, both because 1) it tracks with the existing WP mental model of scripts and styles having separate register
and enqueue
functionality, and 2) it promotes better performance by not trying to render fonts that are not in use.
I've left some smaller feedback on a couple of items as well to get this into a mergeable state.
public function register_font( $font ) { | ||
public function register_font( $id, $font ) { | ||
if ( ! $id ) { | ||
trigger_error( __( 'An ID is necessary when registering a webfont.', 'gutenberg' ) ); |
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 that using the WP internal _doing_it_wrong
here would be better. Plugins and themes do all kinds of weird things and we generally don't want a users' site to WSOD on account of it.
_doing_it_wrong
has the virtue of throwing actual errors when WP_DEBUG
is defined, which is typical for developers, who will get the more aggressive treatment.
I see several other uses of trigger_error
in here and would replace all of them.
* <code> | ||
* wp_enqueue_webfonts( | ||
* array( | ||
* 'some-already-registered-font', // This requires the font to be registered beforehand. |
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 would tweak this slightly to comment on both the already-registered font and the new font. The latter isn't explicitly commented-on here. I would change the example to something like:
wp_enqueue_webfonts(
array(
// Pass an ID for an already-registered font:
'some-already-registered-font',
// Register and enqueue some new fonts:
array(
'id' => 'source-serif-200-900-normal',
'provider' => 'local',
'font_family' => 'Source Serif Pro',
'font_weight' => '200 900',
'font_style' => 'normal',
'src' => get_theme_file_uri( 'assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2' ),
),
array(
'id' => 'source-serif-200-900-italic',
'provider' => 'local',
'font_family' => 'Source Serif Pro',
'font_weight' => '200 900',
'font_style' => 'italic',
'src' => get_theme_file_uri( 'assets/fonts/source-serif-pro/SourceSerif4Variable-Italic.ttf.woff2' ),
),
)
);
@@ -58,7 +55,8 @@ function gutenberg_register_webfonts_from_theme_json() { | |||
} | |||
} | |||
foreach ( $webfonts as $webfont ) { | |||
wp_webfonts()->register_font( $webfont ); | |||
$id = isset( $webfont['id'] ) ? $webfont['id'] : gutenberg_generate_font_id( $webfont ); |
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.
The sole reason we're doing this ternary is that it might break themes already using the Webfont API and registering webfonts, but without specifying the font id.
The thing is, the Webfont API hasn't been made public yet, so we can do whatever we want with the signature... Including breaking changes such as this one.
"Why are we making the id
key required?"
The idea is that developers provide their own hashing mechanism so they can register IDs that works for them, since they're creating their own.
I do believe that adding the gutenberg_generate_font_id
function is confusing. The reason why it's there is that no theme specifies the ID when registering a font through theme.json so we wanted to maintain it backward compatible. That's why this function is used there.
So, now that we're separating registration from enqueueing, I do think it makes sense for the developers to come with their own IDs, so that they can register and enqueue the fonts as they need instead of relying on an abstract mechanism that they're not aware of. Explicit is better than implicit.
Yeah, that one was a bit weird... The problem was that the |
Closed in detriment of #39559 |
Part of #39332. Read #39332 (comment) to gather more context about the whole picture regarding the implementation.
What?
Currently, the webfonts API automatically enqueues all registered webfonts. In this PR, we separate webfont registration from webfont enqueuing into two different processes.
Why?
How?
We're also adding the possibility to register custom
id
s now. We're actually enforcing it when registering throughwp_register_webfont
orwp_enqueue_webfont
.Naming what you're registering and enqueueing makes the whole API more predictable. You know exactly which fonts you are registering and you can be sure that the font you want to enqueue by id is the exact same font you registered. That's the same principle defined in
wp_scripts
andwp_styles
, for example, where you must pass the$handle
.Testing Instructions
wp_register_webfont
php function (See below for examples)wp_enqueue_webfont
*For now, we're not enqueueing any font if solely picked in the editor without programmatically enqueueing, so no fonts will be loaded in the front-end, even if selected. To test this, you need to programmatically enqueue the font until #39399 gets merged.
For the examples below, the Roboto font can be downloaded from https://fonts.google.com/specimen/Roboto. Unzip the package and place the .ttf file in the
bennett/assets/fonts
theme directory.Testing a webfont loaded via
theme.json
:Using the Bennett theme as an example, we edit its
theme.json
file and replacefontFamilies
with the following:Then, we create a
functions.php
in thebennett
theme directory and add the following:Testing a webfont programmatically through PHP:
Next, to test the PHP registration using the
wp_register_webfont
function, infunctions.php
add the following:Like WordPress, we can also directly enqueue a webfont without registering it like so: