-
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
Webfonts: Add internal-only theme.json webfonts handler - WP 6.0 stopgap #40493
Conversation
c6d5bd7
to
f109cb7
Compare
|
||
/* | ||
* DO NOT MERGE THIS CHANGE. | ||
* Don't load the Webfonts API. | ||
require __DIR__ . '/experimental/register-webfonts-from-theme-json.php'; | ||
require __DIR__ . '/experimental/class-wp-webfonts.php'; | ||
require __DIR__ . '/experimental/class-wp-webfonts-provider.php'; | ||
require __DIR__ . '/experimental/class-wp-webfonts-provider-local.php'; | ||
require __DIR__ . '/experimental/webfonts.php'; | ||
*/ |
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 is a temporary workaround for testers to test this PR. Once the handler is backported to Core, then (in a separate PR), remove_action( 'plugins_loaded', '_wp_theme_json_webfonts_handler' );
can be added to turn off the stopgate handler in Core and use the Webfonts API.
@peterwilsoncc @desrosj This PR is using the concepts we talked about. What do you think? @kjellr @jffng I tested it using the TT2 alternative style variations PR. Can you please test it too and share feedback? |
@hellofromtonya I'm getting a fatal error attempting to test this:
My config is:
I tried excluding |
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.
Currently unable to test due to internet troubles, so in the meantime, here's some suggestions for docblocks, polyfill usage and additional validation.
I'll get testing when I'm back on a stable connection.
I resolved this by applying this diff, I think it's correct to limit everything to the stopgap only. diff --git a/lib/experimental/class-wp-theme-json-resolver-gutenberg.php b/lib/experimental/class-wp-theme-json-resolver-gutenberg.php
index 960ea659e8..119489e665 100644
--- a/lib/experimental/class-wp-theme-json-resolver-gutenberg.php
+++ b/lib/experimental/class-wp-theme-json-resolver-gutenberg.php
@@ -36,14 +36,14 @@ class WP_Theme_JSON_Resolver_Gutenberg extends WP_Theme_JSON_Resolver_6_0 {
if ( null === static::$theme ) {
$theme_json_data = static::read_json_file( static::get_file_path_from_theme( 'theme.json' ) );
$theme_json_data = static::translate( $theme_json_data, wp_get_theme()->get( 'TextDomain' ) );
- $theme_json_data = gutenberg_add_registered_webfonts_to_theme_json( $theme_json_data );
+ // $theme_json_data = gutenberg_add_registered_webfonts_to_theme_json( $theme_json_data );
static::$theme = new WP_Theme_JSON_Gutenberg( $theme_json_data );
if ( wp_get_theme()->parent() ) {
// Get parent theme.json.
$parent_theme_json_data = static::read_json_file( static::get_file_path_from_theme( 'theme.json', true ) );
$parent_theme_json_data = static::translate( $parent_theme_json_data, wp_get_theme()->parent()->get( 'TextDomain' ) );
- $parent_theme_json_data = gutenberg_add_registered_webfonts_to_theme_json( $parent_theme_json_data );
+ // $parent_theme_json_data = gutenberg_add_registered_webfonts_to_theme_json( $parent_theme_json_data );
$parent_theme = new WP_Theme_JSON_Gutenberg( $parent_theme_json_data );
// Merge the child theme.json into the parent theme.json. |
Tested using the instructions in the PR's description. Results:
Will continue testing. For others testing by making the manual changes to If so, move:
to
|
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've added a few notes inline.
I tried forcing my way in to using the API from WP CLI (ie, not in theme.json) and wasn't able to find a reasonable way to do so.
Testing with the TT2 branch of WordPress-Develop shows what I'd expect in the source code.
Co-authored-by: Colin Stewart <[email protected]>
Co-authored-by: Colin Stewart <[email protected]>
In reviewing with @costdev, an enterprising developer who really wants to gain access could use reflection to get access to within the container's class. This means using a class isn't truly non-accessible. I've switched the design to use anonymous functions (closures) that are stored to variables within the container's scope (as originally suggested by @peterwilsoncc). |
@peterwilsoncc Thanks for spotting the fatal error. I had commented those 2 lines out in my testing environment but didn't include them in the PR. Those 2 lines are now commented out in fb00786. |
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 gave this test using TT2's style variations.
The fonts are loading for each variation on the front-end. However in the site editor, only the fonts for the currently saved style variation load:
Kapture.2022-04-21.at.11.12.14.mp4
This issue was at one point fixed here but I'm not sure if that is still relevant to this PR's implementation.
87c5ca6
to
8295f15
Compare
Whoops, sorry I missed the variable name changes. Fixed now in 8295f15. |
Getting the following fatal:
|
Hmm, that new method has been merged into Core (in I'll add a temporary check to load either Core's version or |
I got the same error and resolved it by changing the call from Edit: Ah, just saw the commit you linked to Tonya 😛 |
That did the trick, thank you! All variations and their respective fonts working in the site editor and front-end for me. |
I've been testing with the design change to use anonymous functions as variables within the container. This appears to fully prevent direct interaction with the API. In other words, I can't find a way to use the API in a backwards-incompatible way. LGTM 👍 |
Hi all, do we have an ETA on when this will be merged? |
@ndiego before Beta 3's (April 26th) release party. Here's the backport PR in WP Core WordPress/wordpress-develop#2622. |
Adds `_wp_theme_json_webfonts_handler()` for handling `fontFace` declarations in a theme's `theme.json` file to generate the `@font-face` styles for both the editor and front-end. Design notes: * It is not a public API, but rather an internal, Core-only handler. * It is a stopgap implementation that will be replaced when the public Webfonts API is introduced in Core. * The code design is intentional, albeit funky, with the purpose of avoiding backwards-compatibility issues when the public Webfonts API is introduced in Core. * It hides the inter-workings. * Does not exposing API ins and outs for external consumption. * Only works for `theme.json`. * Does not provide registration or enqueuing access for plugins. For more context on the decision to include this stopgap and the Webfonts API, see: * Core's PR 40493 WordPress/gutenberg#40493 * Gutenberg's tracking issue 40472 WordPress/gutenberg#40472 Props aristath, hellofromTonya, peterwilsoncc, costdev, jffng, zieladam, gziolo, bph, jonoaldersonwp, desrosj. See #55567, #46370. git-svn-id: https://develop.svn.wordpress.org/trunk@53282 602fd350-edb4-49c9-b593-d223f7449a82
Adds `_wp_theme_json_webfonts_handler()` for handling `fontFace` declarations in a theme's `theme.json` file to generate the `@font-face` styles for both the editor and front-end. Design notes: * It is not a public API, but rather an internal, Core-only handler. * It is a stopgap implementation that will be replaced when the public Webfonts API is introduced in Core. * The code design is intentional, albeit funky, with the purpose of avoiding backwards-compatibility issues when the public Webfonts API is introduced in Core. * It hides the inter-workings. * Does not exposing API ins and outs for external consumption. * Only works for `theme.json`. * Does not provide registration or enqueuing access for plugins. For more context on the decision to include this stopgap and the Webfonts API, see: * Core's PR 40493 WordPress/gutenberg#40493 * Gutenberg's tracking issue 40472 WordPress/gutenberg#40472 Props aristath, hellofromTonya, peterwilsoncc, costdev, jffng, zieladam, gziolo, bph, jonoaldersonwp, desrosj. See #55567, #46370. Built from https://develop.svn.wordpress.org/trunk@53282 git-svn-id: http://core.svn.wordpress.org/trunk@52871 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Adds `_wp_theme_json_webfonts_handler()` for handling `fontFace` declarations in a theme's `theme.json` file to generate the `@font-face` styles for both the editor and front-end. Design notes: * It is not a public API, but rather an internal, Core-only handler. * It is a stopgap implementation that will be replaced when the public Webfonts API is introduced in Core. * The code design is intentional, albeit funky, with the purpose of avoiding backwards-compatibility issues when the public Webfonts API is introduced in Core. * It hides the inter-workings. * Does not exposing API ins and outs for external consumption. * Only works for `theme.json`. * Does not provide registration or enqueuing access for plugins. For more context on the decision to include this stopgap and the Webfonts API, see: * Core's PR 40493 WordPress/gutenberg#40493 * Gutenberg's tracking issue 40472 WordPress/gutenberg#40472 Props aristath, hellofromTonya, peterwilsoncc, costdev, jffng, zieladam, gziolo, bph, jonoaldersonwp, desrosj. See #55567, #46370. Built from https://develop.svn.wordpress.org/trunk@53282 git-svn-id: https://core.svn.wordpress.org/trunk@52871 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Closing this PR. Why?
Thank you everyone for contributing! |
} | ||
|
||
// Bail out early if there are no settings for webfonts. | ||
if ( empty( $settings['typography'] ) || empty( $settings['typography']['fontFamilies'] ) ) { |
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.
Font families can be defined at the top-level, as in:
{
"settings": {
"typography": {
"fontFamilies": []
}
}
}
But also can be defined per block, as in:
{
"settings": {
"typography": {
"fontFamilies": [] // Fonts for all.
},
"blocks": {
"some/block": {
"typography": {
"fontFamilies": [] // A block can define its own.
}
}
}
}
}
Is this problematic for webfonts? How can a webfont be defined for a single block?
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.
Is this problematic for webfonts? How can a webfont be defined for a single block?
@oandregal Top-level only in this first version.
I remember a discussion thread (not sure where it happened) where @mtias indicated not having block-level web font selection. Also see #40362.
Part of #40472.
Props to @aristath who pulled together the key elements of this PR.
What?
Using the Webfonts API from PR #37140, this PR adds a
theme.json
webfonts handler (albeit funky design) to:theme.json
webfonts defined infontFace
._wp_theme_json_webfonts_handler()
.This is a stopgap approach.
Why?
See the issue for the reasons.
How?
It uses the Webfonts API from PR #37140 but smushes the code together into
$fn = function() {}
closures that are nested inside of a global function. This global function is the container that prevents access to the handler, thus ensuring the mechanisms are hidden and not accessible for extenders to interact with the handler.Testing Instructions
Set up instructions:
Using Twenty Twenty-Two theme, either:
functions.php
file, comment out lines 58 and 81 (the lines that are add inline styles using the font face styles).fontFamilies
in itstheme.json
fileHow to Test
How to Test on the front-end
<head>
element to view all of the nodes within the head.<style>
element that has an ID ofwp-webfonts-inline-css
.Expected Results:
The
<head>
should have one instance of the following HTML style element:This element should have
@font-face
declarations to match what was defined in thetheme.json
file.url
should no longer have thefile:./
placeholder in it.How to Test in the back-end
<head>
element to view all of the nodes within the head.<style>
element that has an ID ofwp-block-library-inline-css
. Open that element (see Expected Results for the CSS).Expected Results:
The
<head>
should have one instance of the following HTML style element:This element should have
@font-face
declarations to match what was defined in thetheme.json
file.url
should no longer have thefile:./
placeholder in it.Design Considerations
To enable testing, this PR comments out loading the Webfonts API files in the
lib/load.php
file. In doing so, this makes the PHPUnit tests fail as those files are missing.A mechanism is needed in Gutenberg to continue developing, testing, and maturing the Webfonts API.
_wp_theme_json_webfonts_handler()
into'wp_plugins'
action event, giving Gutenberg's plugin the opportunity to remove that callback and load its files.'wp_plugins'
action event. Doing this would eliminate deprecating the function once the Webfonts API is merged into Core.