-
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] Change architecture to use Core's Dependencies API #43492
Conversation
f91d4bb
to
7e3e88d
Compare
35a2b12
to
9fe815f
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 is a big PR... 😅
I didn't manage to do a full review today, but posted some comments and will continue with the review and testing tomorrow 👍
$css = sprintf( | ||
"<style id='wp-webfonts-local'%s>\n%s\n</style>\n", | ||
$this->type_attr, | ||
$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.
Should this <style>
element be compiled here? Conceptually it would make more sense to me to get the actual CSS here - just like before, and then compile the <style>
element in the WP_Webfonts_Provider::print_styles
method
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 thought about that. But each Provider will have its own HTML tag needs. For example, a font foundry provider may choose to build a preconnect <link>
instead of dynamically generating the @font-face
styles.
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.
Hmmm, for resource hints like preconnect
, providers should be able to use wp_resource_hints
Though in practice, I expect all 3rd-party providers will extend the Local provider and download font-files to be hosted locally because of all the court decisions etc lately regarding font-services & GDPR
If we print the <style>
element here, then perhaps we should rename the function? If it prints stuff, then the_style
would make more sense than get_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.
Maybe lean towards being verbose. 🤔
get_css()
renamed to get_font_face_css()
. Return only the '@font-face{' . $this->build_font_face_css( $webfont ) . '}';
in get_font_face_css()
. Then provide WP_Webfonts_Provider_Local::get_style_css
, which returns the final <style>
.
Otherwise, leave it alone and rename get_css
to get_styles
.
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.
To keep the provider's get_css()
the same, dec6a3e and d4c01b3 do the following:
- Removes the
<style>
element fromWP_Webfonts_Provider_Local::get_css()
. - Adds
WP_Webfonts_Provider::get_style_element()
which gets the<style>
element. WP_Webfonts_Provider::print_styles()
then wraps the CSS fromget_css()
with the<style>
element.
If a Provider wants something different from <style>
or its implementation, their Provider can overload the WP_Webfonts_Provider::get_style_element()
to return what their needs require.
@aristath 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.
Though in practice, I expect all 3rd-party providers will extend the Local provider and download font-files to be hosted locally because of all the court decisions etc lately regarding font-services & GDPR
While extenders have that option, some will extend from the abstract class. For example, Jetpack extends WP_Webfonts_Provider
for its Google Font Provider https://github.com/Automattic/jetpack/blob/b313bd253afdf919adf7a71a866d0ab85b3a7bda/projects/packages/google-fonts-provider/src/class-google-fonts-provider.php#L18.
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.
Makes sense 👍
private static function search_for_font_family( array $haystack ) { | ||
if ( array_key_exists( 'fontFamily', $haystack ) ) { | ||
$key = 'fontFamily'; | ||
} elseif ( array_key_exists( 'font-family', $haystack ) ) { | ||
$key = 'font-family'; | ||
} else { | ||
trigger_error( 'Font family not found.' ); | ||
return null; | ||
} | ||
|
||
if ( static::is_defined( $haystack[ $key ] ) ) { | ||
return $haystack[ $key ]; | ||
} | ||
|
||
trigger_error( 'Font family not defined in the variation.' ); | ||
return null; | ||
} |
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.
Could we not rewrite this a bit to make it simpler/easier?
private static function search_for_font_family( array $haystack ) { | |
if ( array_key_exists( 'fontFamily', $haystack ) ) { | |
$key = 'fontFamily'; | |
} elseif ( array_key_exists( 'font-family', $haystack ) ) { | |
$key = 'font-family'; | |
} else { | |
trigger_error( 'Font family not found.' ); | |
return null; | |
} | |
if ( static::is_defined( $haystack[ $key ] ) ) { | |
return $haystack[ $key ]; | |
} | |
trigger_error( 'Font family not defined in the variation.' ); | |
return null; | |
} | |
private static function search_for_font_family( array $haystack ) { | |
if ( array_key_exists( 'fontFamily', $haystack ) && static::is_defined( $haystack['fontFamily'] ) ) { | |
return $haystack['fontFamily']; | |
} | |
if ( array_key_exists( 'font-family', $haystack ) && static::is_defined( $haystack['font-family'] ) ) { | |
return $haystack['font-family']; | |
} | |
trigger_error( 'Font family not found.' ); | |
return null; | |
} |
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.
Hmm then it repeats the static::is_defined()
. But I do agree that the PHP notices need work.
- The first one is meant to inform the developer that the property is not defined in the variation.
- The second one is meant to inform the developer that the font-family's value has to be a non-empty string.
2 different notifications. Both need work 🤣
Hey @aristath do you think a developer needs this level of detail?
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 2 notices here may be overkill... 😅
We can definitely simplify it and use a single notification if undefined or empty.
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 say leave it as is. I see both notices being useful.
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.
How about this? Leave it as is and then get feedback from developers if the extra informational notice is helpful or not. If not, then combine into 1 notice. Thoughts?
Benchmark: Average Processing Time
Ran on 17 Aug 2002Commits used:
Results:
Ran on 1 Sep 2002Commits used: Results:
|
I plan on doing some testing for this on Friday. |
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.
Most of my stuff was nit picking. Overall, this looks like a great improvement. I ❤️ the test coverage.
$css = sprintf( | ||
"<style id='wp-webfonts-local'%s>\n%s\n</style>\n", | ||
$this->type_attr, | ||
$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.
Maybe lean towards being verbose. 🤔
get_css()
renamed to get_font_face_css()
. Return only the '@font-face{' . $this->build_font_face_css( $webfont ) . '}';
in get_font_face_css()
. Then provide WP_Webfonts_Provider_Local::get_style_css
, which returns the final <style>
.
Otherwise, leave it alone and rename get_css
to get_styles
.
private static function search_for_font_family( array $haystack ) { | ||
if ( array_key_exists( 'fontFamily', $haystack ) ) { | ||
$key = 'fontFamily'; | ||
} elseif ( array_key_exists( 'font-family', $haystack ) ) { | ||
$key = 'font-family'; | ||
} else { | ||
trigger_error( 'Font family not found.' ); | ||
return null; | ||
} | ||
|
||
if ( static::is_defined( $haystack[ $key ] ) ) { | ||
return $haystack[ $key ]; | ||
} | ||
|
||
trigger_error( 'Font family not defined in the variation.' ); | ||
return null; | ||
} |
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 say leave it as is. I see both notices being useful.
@hellofromtonya can we update the Testing Instructions to point to Twenty Twenty-Two, please? I believe that is what we're all testing against right now to see if there are any regressions. Although, I'm sure it would be ideal to get testing against other themes that may already be leveraging some of the Webfonts APIs. This is some monumental work here and really solid stuff. 👏 |
6dd3d88
to
e3f4d4e
Compare
Rebased the PR and ported a change from trunk 👍 |
In working through the changes needed to get the API working with Global Styles, I think the new architectural design needs more work:
I have ideas for each of the above. Will work on today and then push new commits for testing and discussing. TODO:
|
To better encapsulate the deprecated WP_Webfonts methods: * Rename the new API to `WP_Web_Fonts` * Restore the original `WP_Webfonts` class * Retain only the deprecated `WP_Webfonts` methods * Reorganize the inheritance to: WP_Dependencies > WP_Webfonts > WP_Web_Fonts
* Deprecated wp_register_webfont() in favor to use wp_register_webfont_variation() * Centralized font-family and variations in wp_register_webfonts() * Move deprecated global functions to a new deprecation file * Moved the font-family name to handle conversion into WP_Web_Fonts::add_font_family()
Why? Primary reason is to reduce the amount of code that Core will need to maintain. If not adding value, then remove it. 1. Each is a wrapper for `wp_webfonts()->method_name()`. 2. There's no extra context or meaning to providing these wrappers. 3. Each is likely not needed by the majority of extenders. For those extenders who need access to this functionality, they can use the `WP_Web_Fonts` methods instead: * Deprecated wp_get_webfont_providers() Instead use wp_webfonts()->get_providers() * Removed wp_register_font_family() Instead pass an array keyed by the font-family with an empty array to wp_register_webfonts(). For example, wp_register_webfonts( array( 'Lato' => array() ) ); * Removed wp_register_webfont_variation() Instead use wp_register_webfonts(). Why? To ensure the font-family is also registered. A variation needs to have a font-family.
Test ReportTest Env
Test Results
Using the plugin's deprecated webfonts array structureAdding
CSS added to the
|
Test ReportTests worked as expected for me 👍🏻 Consistent with @hellofromtonya's results 🎉 Environment
Actual Results
(No screenshots, as I have nothing to add to what was provided above.) |
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 HUGE improvement!
Tested the implementation and everything works as expected.
The code looks good, and I couldn't find anything wrong.
There are some things we can tweak, but they can be done in followup PRs once this is merged 👍
I see the e2e/Playwright tests are stuck here... Probably unrelated to the PR itself. I merged Update: All tests now pass. |
Thank you everyone for contributing to this architectural redesign!!! 🎉 |
Closes #41481.
See PR #40556 for the Proof of Concept (PoC).
Replaces PR #42946.
What?
Changes the Webfonts API's architecture to use WordPress Core's Dependencies API.
Why?
From the issue #41481
How?
Extending
WP_Dependencies
The new architecture extends from
WP_Dependencies
. Liberties are taken to wrangleWP_Dependencies
to fit the needs of web fonts.do_items()
processes each provider for each to print their CSS for their web fontsWP_Dependencies::add()
where an instance of_WP_Dependency()
is created and then stored in the queue_WP_Dependency::$deps
propertywp_enqueue_webfont( $font_family_handle )
Additional technical notes
WP_Webfonts_Utils
which is a static class wrapper. Why? See Webfonts API Architecture: Remove external access to internal-only functionality #41484.wp_enqueue_webfont_variations()
wp_deregister_font_family()
wp_deregister_webfont_variation()
Backwards Compatibility: Deprecations
To help extenders know where to modify their plugins/themes and to give them time to make the changes, the following list of deprecated functions, functionality, and array structures will still work (for now) and will throw a deprecation notice.
Note: These unused deprecated items will be removed before backporting to Core.
Deprecated functions:
wp_register_webfont()
wp_enqueue_webfonts()
wp_get_webfont_providers()
WP_Webfonts
:WP_Webfonts::get_font_slug()
WP_Webfonts::init()
WP_Webfonts::get_registered_webfonts()
WP_Webfonts::get_enqueued_webfonts()
WP_Webfonts::get_all_webfonts()
WP_Webfonts::register_webfont()
WP_Webfonts::enqueue_webfont()
Web font array structure
When invoking
wp_register_webfonts()
, the structure of the web font definition changed:deprecated structure
new structure
For backwards-compatibility (see note),
WP_Webfonts::migrate_deprecated_structure()
migrates the deprecated web font array structure into the new API structure. It also throws a deprecation notice to alert developers what needs to change.Where do deprecations now live?
This PR adds a new file called
webfonts-deprecations.php
, which contains the deprecated global functions.To handle the
WP_Webfonts
public methods BC and deprecations:WP_Web_Fonts
, which is the new API class.WP_Webfonts
class becomes the BC layer between as it only includes the deprecated methods.WP_Web_Fonts
extends fromWP_Webfonts
extends fromWP_Dependencies
Before Backporting to Core
The API has a BC layer in it to (a) give extenders time to upgrade their code and (b) keep sites from breaking if they are using the original API. This BC layer is temporary and should not be backported to Core.
BACKPORT NOTE
is included in the API's code where code needs to be removed before backporting.What to remove:
WP_Webfonts
file.WP_Web_Fonts
, change theextends WP_Webfonts
toextends WP_Dependencies
.BACKPORT NOTE
.Removed functions
The following methods were removed:
WP_Webfonts::generate_and_enqueue_styles()
was a registered callback for printingWP_Webfonts::generate_and_enqueue_editor_styles()
was a registered callback for printingThese were registered callbacks for printing.
TODO
- [ ] Performance enhancement: ChangeNot worth the changes for a tiny bump.$deps
array to be keyed by the variation handles. Faster O(1) lookups vs scanning the array elements.WP_Webfonts::dequeue()
.Testing Instructions
@font-face
stylings in the admin area by:wp-webfonts-local
(which is in the<head>
)<style>
element to inspect the CSSwp-webfonts-local
<style>
element to inspect the CSSwp-webfonts-local
<style>
element to inspect the CSS and verify the CSS.Testing a plugin
This gist is a plugin that uses Jetpack's Google Font Provider package, which is using the original (deprecated) API functionality and web fonts array structure. So it should continue to work and throw deprecation notices.
Instructions of how to install and test are in the plugin's
README.md
file, which you can get here.Note: This plugin has a Google Fonts Provider which means it will print
<style id="
wp-webfonts-jetpack-google-fonts">..</style>into the
`.