-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Font Face: to generate and print font-face styles for theme.json fonts #51770
Conversation
8800cd1
to
c56c8df
Compare
This PR is ready to be manually tested. I've added test instructions for TT3 (block theme) and TT1 (classic theme). Please test and add test reports. |
Test ReportEnv:
Test Instructions
Results
|
Flaky tests detected in 2b61cf2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5462793951
|
Pushed a codings-standards fix 👍 |
Hi team; where are we on this one? Fonts loaded by theme.json are currently not working in the Editor in WordPress 6.3 Beta 2 when metaboxes exist. I strongly believe this will need to get fixed in 6.3. See #51209 |
@ndiego I think it's almost ready. I just need to fix some unit tests. UPD: This PR is ready to be reviewed. |
76879a1
to
72d3ecf
Compare
Just tested to see if this fixes the issue where fonts registed in theme.json do not load when metaboxes are present in Gutenberg 16.1 or WordPress 6.3 Beta 2 (i.e. the Editor is iframed). Unfortunately, I am still seeing the issue. In WordPress 6.2.2, without Gutenberg active, if you enable the Yoast plugin, the fonts in TT3 will load as expected. Yoast includes a metabox and the post Editor in 6.2.2 is not iframed. Now if you update to 6.3 Beta 2, the fonts are not loaded when a metabox is present (the Editor is no longer iframed). Reverting back to 6.2.2, if you activate Gutenberg with this PR, unfortunately, the fonts are still not loaded when the metabox is present.
|
68ea19e
to
fcb16fd
Compare
5318e56
to
c6746db
Compare
Rebased on top of |
1. Adds a test case to the fonts directory. 2. Adds a dataset trait for sharing datasets amongst different test classes. This change the maintance burden of redundant datasets by centralizing in a shared trait. 3.Removes the style variation tests as these are no longer relevant to the Font Face. 4. A wee bit of renaming and reformatting.
1. Adds test for when no fonts are passed into the function, i.e. meaning it will pull from the resolver. 2. Improves resolver test to validate the full returned array structure. 3. Moves the expected fonts to the dataset trait to share it between these 2 tests. 4. A wee bit of girlscouting.
1. Adds guard clause to WP_Font_Face::generate_and_print(). 2. Adds unhappy test for it. 3. Adds similar unhappy test for wp_print_font_faces().
@anton-vlasenko I reviewed the tests. You did a great job! Thank you :) To help, I made the changes from the review in the 3 added commits with commit messages explaining the changes. What do you think? |
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 have tested this PR again, and my test report is still valid considering the recent fixes.
Additionally, I tested different combinations, such as meta boxes enabled/disabled, block/classical theme, and I couldn't find any issues (to the best of my knowledge).
The code also looks good to me.
Therefore, I approve this PR.
&& | ||
function_exists( 'current_theme_supports' ) && ! current_theme_supports( 'html5', 'style' ) | ||
) { | ||
$this->style_tag_attrs = array( 'type' => 'text/css' ); |
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 type
attribute is deprecated, as stated in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/style#deprecated_attributes.
However, I don't think this could create any issues (and the type
attribute can be removed later if absolutely necessary).
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.
Core still supports pre HTML 5 syntax. This particular code comes from WP Styles. Once Core drops < HTML 5, then this can also be removed.
Thank you everyone for your contributions :) I've merged this PR into
Remember, Font Face only loads into memory (replacing the Fonts API) when the Fonts Library is also loaded into memory. That's by design to keep sites running (using Fonts API) while providing the new typography path forward for testing with the Fonts Library. |
Removed the |
|
||
// Converts the "file:./" src placeholder into a theme font file URI. | ||
if ( ! empty( $font_face['src'] ) ) { | ||
$font_face['src'] = static::to_theme_file_uri( (array) $font_face['src'] ); |
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 know this is in Core already, but it relates to a feature being built for 6.6: relative paths to background images in theme.json styles
For consistency, and after a nudge from @creativecoder, I decided to follow the file:./
dotslash path convention to represent paths to these images.
@noisysocks then pointed out that dotslash indicates the current directory, and might imply other dot-style paths, e.g., ../
So, if web fonts can be used in theme variations, e.g., my-theme/styles/variation-with-fonts.json
, the ./
would indicate a path relative to the styles directory.
It might also imply that paths with ../../some/path.file
could also be resolved. But I'm not sure if get_theme_file_uri()
supports such things as it looks for files relative to the active or parent theme root. Maybe I'm missing something.
I was wondering if we should enforce a convention. Either:
- Enforce that all paths must be relative to the theme root, or
- Allow relative to current directory, and then in resolve them in the background with PHP trickery with realpath or something.
What do folks think?
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 see what you mean... the syntax and behavior seem mismatched: file:./
seems to imply a reference to the current directory, but really it's a placeholder for the theme root looking at how it's coded.
Updating file:./
to apply to the current directory (e.g. theme-root/styles
) for theme variations and/or adding resolution of ../
paths would make the syntax clearer. But I worry about backward compatibility (I imagine that some theme variations are already using file:./
?). And some care is needed to prevent directory traversal outside the theme directory.
So I favor keeping things simple, and continuing to use file:./
simply as a placeholder to the theme directory root. But it's not a strong opinion.
(Note that either way, we should add to and update the theme handbook docs to make it clearer).
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.
Thanks for the quick reply @creativecoder
So I favor keeping things simple, and continuing to use file:./ simply as a placeholder to the theme directory root.
👍🏻
I went with this approach for now given that folks might be using to it already.
Updating file:./ to apply to the current directory (e.g. theme-root/styles) for theme variations and/or adding resolution of ../ paths would make the syntax clearer. But I worry about backward compatibility (I imagine that some theme variations are already using file:./?).
True. Backwards compatibility is now a requirement regardless. And I considered this in #61271 and contemplated supporting file:./
(with the ./
for folks who are now used to it) and also a file:
prefix.
But I agree that it's simpler to go with what's there until folks come up with a viable alternative. Even if it's "Enforce that all paths must be relative to the theme root" - that way, we could update the docs now and make the ./
optional later without adding any complex ../
support etc. 🤷🏻
What?
Introduce Font Face processing to generate and print
@font-face
styles for fonts in theme.json merged data.It's minimalistic in what it publicly exposes to avoid BC concerns.
There's no register, deregister, or enqueue of fonts or providers.
The fonts to be processed come directly from theme.json merged data which contains the theme's defined fonts and user "activated" fonts (from the Font Library).
Why?
See #51769.
How?
It adds 2 new classes
WP_Font_Face
andWP_Font_Face_Resolver
which currently live in/lib/experimental/fonts/
folder. That folder could also house the new Fonts Library.Jobs:
WP_Font_Face
's job is to generate and print the@font-face
styles.WP_Font_Face
.If in the future support for custom providers is needed, the local provider can be removed from the
WP_Font_Face
and reintroduced as a separate class without breaking BC (backwards-compatibility).When it's time to print,
wp_print_font_faces()
handles a call to theWP_Font_Face_Resolver::get_fonts_from_theme_json()
and passes the fonts toWP_Font_Face::print()
.This also means other sources could pass fonts to
WP_Font_Face::print()
, such as plugins on a classic site.Prepared to be merged before Fonts Library
The new files will not be loaded into memory unless the Fonts Library exists. In this way, this PR can be merged into
trunk
before the Fonts Library without breaking websites that are using the Fonts API.TODO
WP_Font_Face_Resolver
.WP_Font_Face
.WP_Font_Face
.print()
to hide the internal workings (eliminate future BC concerns).Testing Instructions
a. Open
lib/load.php
.b. Scroll down to where the Font Face files are loaded.
c. Add
true ||
in theif
:a. In Site Editor using Dev Tools, check that the
<script id="wp-fonts-local">
and@font-face
styles are present in<head>
and the iframed editor.b. In a post or page, check that the
<script id="wp-fonts-local">
and@font-face
styles are present in<head>
and the iframed editor.c. On the front-end, check that the
<script id="wp-fonts-local">
and@font-face
styles are present.functions.php
, add the following code:<script id="wp-fonts-local">
and@font-face
styles are present.