-
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
Add a client-side font face resolver in the editor #65019
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: +435 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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 prepared a PR for TT5 to test, which moves all the font family definitions to their respective style variation / typography presets, and it is working as expected. I haven't reviewed the code yet.
Kapture.2024-09-05.at.16.49.54.mp4
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 also working well for me 🎉 I've left some code formatting-related comments.
packages/block-editor/src/components/editor-fonts-resolver/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/editor-fonts-resolver/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/editor-fonts-resolver/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/editor-fonts-resolver/utils.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/editor-fonts-resolver/utils.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/editor-fonts-resolver/utils.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Sarah Norris <1645628+mikachan@users.noreply.github.com>
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 latest changes are looking good to me, nice work 👏
I've never edited any code related to the block canvas before; I'm wondering if we should get some more 👀 on this before bringing it in.
@@ -43,6 +44,7 @@ export function ExperimentalBlockCanvas( { | |||
__unstableContentRef={ localRef } | |||
style={ { height, display: 'flex' } } | |||
> | |||
<EditorFontsResolver /> |
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.
@youknowriad or @ellatrix since you've edited this file before, could you please sense-check how the fonts resolver is being added here?
This seems to work well in the editor content area and loads up everything as expected. The two areas where I'm still seeing fallback fonts from the stack are under the Typesets panel: And in the Styles sidebar (both the theme and typography style variation sections): Basically, what the user sees as options is not matching what they're getting in the editor content area. |
}, [] ); | ||
|
||
// Get the fonts from merged theme.json settings.fontFamilies. | ||
const [ fontFamilies = [] ] = useGlobalSetting( '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.
I know it's hard to figure that out on your own but If I'm not wrong useGlobalSetting
doesn't work consistently in all editors. In fact, we should probably avoid using it in the "block-editor" package entirely because it relies on the presence of a provider that may or may not exist (it doesn't exist in the post editor for instance).
@aaronrobertshaw added a recently a new block editor setting that allows us to access global styles in all editors if I'm not wrong and we should probably be using that.
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 ping 👍
a new block editor setting that allows us to access global styles in all editors
This was specifically related to the styles data from Global Styles as the settings values were already included within the block editor settings.
The Global Styles settings and styles weren't merged as there wasn't consensus on what the shape of those settings should be while maintaining BC. That led to using a symbol as a private key for the Global Styles style data.
Here's the PR adding the style data to the block editor settings: #61556.
Without looking into the code I'm not sure whether the Global Styles settings under __experimentalFeatures
in the editor settings are kept in sync with unsaved Global Styles changes in the site editor. It could be possible that aspect needs addressing or perhaps in the short term this could fallback to the editor settings if the Global Styles context isn't available (in post editor).
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.
Good point, I mistakenly thought this is about the styles.
This PR reminds of the "styles" prop that we pass to the block editor to render the current theme styles. I could suggest a new "type" within the same "styles" prop to load fonts (like we do for svg for instance) but first. I would like to understand more.
What fonts are we trying to resolve here. I'm guessing the fonts that are used in the theme styles right? In which case, I feel like "resolving them" or "rendering them" should be done in the EditorStyles component. It's a component that is already used to "resolve or render" SVGs and CSS, it's already rendered in both iframe and non-iframe, so I'm starting to think that it's probably the right place for this change? (Introduce a new "type" of style and render it within that component)
Please correct me if my argumentation or abstraction seem too far off
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'm not sure how to access global styles settings in a component like EditorStyles without using useGlobalSetting
. For this particular implementation I'm interested in getting the 'client side latest version' of settings.typography.fontFamilies
.
as the settings values were already included within the block editor settings
@aaronrobertshaw could you show me how is this done? I haven't found how to access the fontFamilies path mentioned before.
What fonts are we trying to resolve here. I'm guessing the fonts that are used in the theme styles right?
Yes. This PR was motivated by that, but implementing this would make us able to remove the manual loading of fonts when a font is installed using the font library, too, so we could simplify a little bit that code.
I feel like "resolving them" or "rendering them" should be done in the EditorStyles component.
@youknowriad I'm not sure about that, EditorStyles gets styles and not settings. At least, as it is today, that component is not getting the data needed to implement the desired functionality.
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'm not sure about that, EditorStyles gets styles and not settings. At least, as it is today, that component is not getting the data needed to implement the desired functionality.
EditorStyles renders the CSS variables for the color presets for instance, renders the SVG filters used by Duotone. Isn't it the same for fonts here? We're resolving the fonts that are used in the CSS.
It is possible that I'm misunderstanding the issue that this PR solves though and if it's the case please clarify? I'm assuming that resolving the fonts is done in the frontend somehow right? How is it done today?
I'm not sure how to access global styles settings in a component like EditorStyles without using useGlobalSetting. For this particular implementation I'm interested in getting the 'client side latest version' of settings.typography.fontFamilies.
The styles prop received by the EditorStyles
component can contain things like that
const styles = [
{
css: `some css`,
},
{
__unstableType: 'svgs'
assets: `some sig element`
},
{
__unstableType: 'presets'
assets: `css with CSS variables generated from the presets`
},
];
This represents all what's necessary for the "styles" to be rendered properly and to work properly.
Would it be out of the question to consider "fonts" at the same level as these and just add
{
__unstableType: 'fonts'
fonts: `whatever is needed to resolve the fonts`
},
Again, it's possible that I'm wrong
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.
Btw, right now that "styles" prop come from the block editor settings array that is passed from the backend to the frontend during the initialization of the editor. So custom CSS files are resolved there for instance and added to that array of styles
In the site editor, since we have the ability to update global styles, that "styles" setting (the one that comes from backend to server) is also updated on the fly as we make changes to the global styles Here
styles: [ ...nonGlobalStyles, ...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.
could you show me how is this done? I haven't found how to access the fontFamilies path mentioned before.
In the block-editor
store, under settings.__experimentalFeatures.typography.fontFamilies
, you should find the data you're chasing in the post editor.
One example of accessing theme.json settings via the block editor store can be found in the layout block support. Another example (that might change soon) is the block style variations block support which prefers the GlobalStyleContext data, if available, falling back to the block editor store's data.
Hope that helps a little 🤞
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 insights.
In this commit I removed the use useGlobalSetting
and replaced it by the use data from block-editor store and move the call to the font faces resolved inside the EditorStyles component: c6a32f9
} | ||
|
||
if ( addTo === 'iframe' || addTo === 'all' ) { | ||
const iframeDocument = document.querySelector( |
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.
Did we consider moving this logic elsewhere. I feel we should avoid using DOM APIs like that, it's really not clear which "editor-canvas" this is targeting. I can see a world where there are two editors iframes on the same page.
If fonts need to be loaded in the iframe, can we instead move the logic to load fonts into the Iframe
component instead?
We can probably use a memoized selector to fetch the fonts from the global styles config we have in the global styles config key of the block editor settings and load the fonts when they change. I believe we may have logic that get close to this for "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.
If fonts need to be loaded in the iframe, can we instead move the logic to load fonts into the Iframe component instead?
Fonts should be added in both the main document and the iframe's document. If we move that logic to the iframe document, could we add those to the main one with the same code? Should we repeat the font loading in both places? What happen when the editor is not rendering the iframe? (shouldIframe
is false in a few places across the codebase)
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 in most places for block themes (the ones that supports fonts), we're using the iframe. There are still some case where we are not though: In the post editor, and only if v<3 blocks are registered or no metaboxes and only in Core (Gutenberg plugin forces iframe for the post editor I think in block themes).
We are working on removing these edge cases but yeah in the meantime we can probably have a reusable hook or something and I think BlockCanvas
is where the switching happens between iframe or not.
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.
In this commit I removed this hardcoded references to the document and 'iframe' references. Now it's using a ref
to reference the document following the useDarkThemeBodyClassName
example: 5a9e2d0
if ( ! node ) { | ||
return; | ||
} | ||
function useDarkThemeBodyClassName( scope, ref ) { |
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.
Why did you refactor this from a hook that returns a ref to a hook that receives a ref as an argument?
Returning a ref allow supporting conditional rendering, you can adapt the hook when the "node" changes, it's not something you can do in hooks that receive refs.
Receiving refs in hooks is a pattern we should avoid (See recent discussion here #64943 (comment) )
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.
Why did you refactor this from a hook that returns a ref to a hook that receives a ref as an argument?
Because it's hard to understand the pattern (at least for me 😆 ), so I thought it wasn't something 100% intentional. Thanks for providing context. Now I see the reasons why it is like that. I reverted the changes to that hook and refactored the new useEditorFontsResolver to follow that pattern: 463fa95
…esolver follwing useDarkThemeBodyClassName pattern
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 tests well for me in the Editor; I can see the correct font faces being loaded and applied when switching style variations.
The change to how the fonts resolver is now added to EditorStyles
looks good and seems more aligned with the other style element used here.
I also believe the feedback in this comment from @youknowriad has been addressed, although it'd be great to see if Riad has any further feedback here.
Otherwise, I think this is looking ready to bring in 👏
@justintadlock, could you provide the theme you are testing? |
currentTheme: | ||
// Disable Reason: Using 'core' as string to avoid circular dependency importing from @wordpress/core-data. | ||
// eslint-disable-next-line @wordpress/data-no-store-string-literals | ||
select( 'core' )?.getCurrentTheme(), |
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 not allowed in the block-editor package. We shouldn't add an eslint exception. Why do we need the theme object here?
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 get the base url of the theme's fonts.
Example: the theme.json data has something like file:/assets/fonts/one-font.woff2
.
We need to know how to resolve that theme relative font URL in the browser.
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.
We need to find an alternative solution. The block editor is a generic package independent of WordPress and can't make REST API calls.
The alternative I suggested initially is to "resolve" the files in the server and add a new _unstableType
for the "styles" object received by the editor.
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 "resolve" the files in the server
Do you mean when the theme.json
data is read from the file / database?
add a new _unstableType for the "styles" object received by the editor.
Sorry but I don't understand this.
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.
Do you mean when the theme.json data is read from the file / database?
Yes, basically in this place, in the code base. We read global styles and enqueue "styles" setting in the block editor. We could add a new "style" in that array with a specific "_unstableType" => "font"
that we treat in a custom way in the EditorStyles
component.
I know that function is in WordPress Core but in Gutenberg we have a hook to override the output of this function block_editor_settings_all
filter I think.
I know that we used this approach in the past for "svg filters" which are also "special styles" that are not just CSS. @ajlende would know more but I think we moved a little bit from it (not entirely sure why though)
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'm trying to follow the pattern of this PR using the _links array. It seems to work with the regular theme.json, but I'm unable to access the links for the style variations. How I can pass the variation _settings to the editor settings?
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'm trying to understand the use case. What do we need to do with the resolved font paths? Preload the fonts? Could you provide some testing steps and expectations for style variations?
Just to confirm, if I run await wp.data.resolveSelect( 'core' ).__experimentalGetCurrentThemeGlobalStylesVariations()
in the console I see that the variations' fonts are resolved in the _links
object.
How I can pass the variation _settings to the editor settings?
Aren't the links already available in the settings?
It's private, so, in the useEditorFontsResolver useSelect
you'd have to import the private symbol key (import { globalStylesLinksDataKey } from '../../store/private-keys';
) and access it like settings[ globalStylesLinksDataKey ]
.
I just logged the settings out in that hook and I can see the font _links
object, and when I change variations the links update.
I'm not sure that helps, maybe it leads you in the right direction?
Just to clarify and contrast what I was working on with background images, the resolved background image paths are used to build the correct CSS. It's done at this point in the global styles output generation:
gutenberg/packages/block-editor/src/components/global-styles/use-global-styles-output.js
Lines 308 to 314 in ee98b6e
export function getStylesDeclarations( | |
blockStyles = {}, | |
selector = '', | |
useRootPaddingAlign, | |
tree = {}, | |
disableRootPadding = false | |
) { |
tree
contains the embedded _links
object and path resolution is done "on the fly" when outputting 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.
What do we need to do with the resolved font paths? Preload the fonts? Could you provide some testing steps and expectations for style variations?
This is the issue we are trying to fix: #59965
(load the fonts in the main frame and in the editor iframes when the fonts are defined in variations)
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.
In this commit I added the JS consumption of the _links. It seems to work for the main editor but not for the editors loaded to display the style variations.
The commit includes a console.log to display the font families and the _links.
gutenberg/packages/block-editor/src/components/use-editor-fonts-resolver/index.js
Lines 22 to 34 in 1461989
const { _links = [], fontFamilies = [] } = useSelect( ( select ) => { | |
const { getSettings } = select( editorStore ); | |
const _settings = getSettings(); | |
return { | |
_links: _settings[ globalStylesLinksDataKey ]?.[ 'wp:theme-file' ], | |
fontFamilies: | |
_settings?.__experimentalFeatures?.typography?.fontFamilies, | |
}; | |
}, [] ); | |
// eslint-disable-next-line no-console | |
console.log( 'fontFamilies', fontFamilies, 'links', _links ); | |
In the screencast you can see how the editors instances created to display the style variations doesn´t get the right list of font families (the ones from the style variation) and the _links array is empty.
Screencast.from.27-09-24.14.04.29.webm
Is there any way to get the right list of font families and _links in the editors used to feature a style variation?
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.
So it looks like here you need all the variations' fonts at once for the sidebar. Is that right?
If you look at where the key value is set in the settings, you'll notice that the _links
passed to settings contains only the resolved links for the merged global styles that are currently loaded in the editor.
That's why the resolved paths for style variations only work once you click on them, because it's it that point they're loaded into the editor.
So you'll probably need to iterate over the output of await wp.data.resolveSelect( 'core' ).__experimentalGetCurrentThemeGlobalStylesVariations()
as hinted at above since they contain all the variations their resolved paths, and somehow preload the fonts for each variation.
However you can't use these core selectors in the block editor package, that is, in useEditorFontsResolver()
.
Rather, you might like to tackle from the edit-site package, where the style variations are rendered.
For example, export useEditorFontsResolver()
as a private method and have it accept _links
and fontFamilies
as dependencies. I don't know if that will work, but it's the first thing I'd try without having extra context.
I wasn't testing with any one specific theme. What I shared above could be seen with any theme that includes web fonts in a style variation. I saw the same with Twenty Twenty-Four. Do the changes since my last comment address loading the fonts in the iframes in the two sidebar panels? I can certainly test again. |
The latest PR is behaving much better than it did when I initially tested. There's still an issue with the initial state in the sidebar panels, but they are updated as you attempt to preview. Here's a quick video that walks through what's currently happening: pr-65019-type-001.mp4I zipped a copy of my theme, which has a lot more variety with typography than TT4: x3p0-ideas.zip |
Flaky tests detected in 8ced871. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10960108376
|
* @param | ||
* @return array An array of resolved paths. | ||
*/ | ||
private static function get_resolved_fonts_theme_uris( $theme_json_data ) { |
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 this could be part of get_resolved_theme_uris
- it's not that much code, and it could reuse $placeholder
etc.
Example diff
diff --git a/lib/class-wp-theme-json-resolver-gutenberg.php b/lib/class-wp-theme-json-resolver-gutenberg.php
index 9c64e09b7e..c128a3c6ea 100644
--- a/lib/class-wp-theme-json-resolver-gutenberg.php
+++ b/lib/class-wp-theme-json-resolver-gutenberg.php
@@ -838,30 +838,30 @@ class WP_Theme_JSON_Resolver_Gutenberg {
*
* @since 6.6.0
*
- * @param
+ * @param
* @return array An array of resolved paths.
*/
private static function get_resolved_fonts_theme_uris( $theme_json_data ) {
$resolved_theme_uris = array();
- if ( !empty( $theme_json_data['settings']['typography']['fontFamilies'] ) ) {
+ if ( ! empty( $theme_json_data['settings']['typography']['fontFamilies'] ) ) {
$font_families = ( $theme_json_data['settings']['typography']['fontFamilies']['theme'] ?? array() )
+ ( $theme_json_data['settings']['typography']['fontFamilies']['custom'] ?? array() )
+ ( $theme_json_data['settings']['typography']['fontFamilies']['default'] ?? array() );
foreach ( $font_families as $font_family ) {
- if ( !empty( $font_family['fontFace'] ) ) {
+ if ( ! empty( $font_family['fontFace'] ) ) {
foreach ( $font_family['fontFace'] as $font_face ) {
- if ( !empty( $font_face['src'] ) ) {
- $sources = is_string( $font_face['src'] )
+ if ( ! empty( $font_face['src'] ) ) {
+ $sources = is_string( $font_face['src'] )
? array( $font_face['src'] )
: $font_face['src'];
foreach ( $sources as $source ) {
if ( str_starts_with( $source, 'file:' ) ) {
$resolved_theme_uris[] = array(
- 'name' => $source,
- 'href' => sanitize_url( get_theme_file_uri( str_replace( 'file:./', '', $source ) ) ),
+ 'name' => $source,
+ 'href' => sanitize_url( get_theme_file_uri( str_replace( 'file:./', '', $source ) ) ),
'target' => "typography.fontFamilies.{$font_family['slug']}.fontFace.src",
);
}
@@ -896,12 +896,6 @@ class WP_Theme_JSON_Resolver_Gutenberg {
$theme_json_data = $theme_json->get_raw_data();
- // Add font URIs.
- $resolved_theme_uris = array_merge(
- $resolved_theme_uris,
- static::get_resolved_fonts_theme_uris( $theme_json_data )
- );
-
// Using the same file convention when registering web fonts. See: WP_Font_Face_Resolver:: to_theme_file_uri.
$placeholder = 'file:./';
@@ -952,6 +946,31 @@ class WP_Theme_JSON_Resolver_Gutenberg {
}
}
+ // Add font URIs.
+ if ( ! empty( $theme_json_data['settings']['typography']['fontFamilies'] ) ) {
+ $font_families = array_merge(
+ $theme_json_data['settings']['typography']['fontFamilies']['theme'] ?? array(),
+ $theme_json_data['settings']['typography']['fontFamilies']['custom'] ?? array(),
+ $theme_json_data['settings']['typography']['fontFamilies']['default'] ?? array()
+ );
+ foreach ( $font_families as $font_family ) {
+ if ( ! empty( $font_family['fontFace'] ) ) {
+ foreach ( $font_family['fontFace'] as $font_face ) {
+ $sources = (array) ( $font_face['src'] ?? array() );
+ foreach ( $sources as $source ) {
+ if ( is_string( $source ) && strpos( $source, $placeholder ) === 0 ) {
+ $resolved_theme_uris[] = array(
+ 'name' => $source,
+ 'href' => sanitize_url( get_theme_file_uri( substr( $source, strlen( $placeholder ) ) ) ),
+ 'target' => "typography.fontFamilies.{$font_family['slug']}.fontFace.src",
+ );
+ }
+ }
+ }
+ }
+ }
+ }
+
return $resolved_theme_uris;
}
if ( !empty( $theme_json_data['settings']['typography']['fontFamilies'] ) ) { | ||
|
||
$font_families = ( $theme_json_data['settings']['typography']['fontFamilies']['theme'] ?? array() ) | ||
+ ( $theme_json_data['settings']['typography']['fontFamilies']['custom'] ?? array() ) |
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.
Was the union operator +
intentional? I think array_merge()
is more readable and will reindex the new array, avoid potential lost data.
See: https://stitcher.io/blog/array-merge-vs+
Unit test coverage would help test that.
currentTheme: | ||
select( editorStore ).getSettings()?.__experimentalFeatures | ||
?.currentTheme, | ||
fontFamilies: | ||
select( editorStore ).getSettings()?.__experimentalFeatures | ||
?.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.
Opportunity to call getSettings()
once, e.g.,
const { __experimentalFeatures } =
select( blockEditorStore ).getSettings();
return {
currentTheme: __experimentalFeatures?.currentTheme,
fontFamilies: __experimentalFeatures?.typography?.fontFamilies,
};
currentTheme: | ||
// Disable Reason: Using 'core' as string to avoid circular dependency importing from @wordpress/core-data. | ||
// eslint-disable-next-line @wordpress/data-no-store-string-literals | ||
select( 'core' )?.getCurrentTheme(), |
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'm trying to understand the use case. What do we need to do with the resolved font paths? Preload the fonts? Could you provide some testing steps and expectations for style variations?
Just to confirm, if I run await wp.data.resolveSelect( 'core' ).__experimentalGetCurrentThemeGlobalStylesVariations()
in the console I see that the variations' fonts are resolved in the _links
object.
How I can pass the variation _settings to the editor settings?
Aren't the links already available in the settings?
It's private, so, in the useEditorFontsResolver useSelect
you'd have to import the private symbol key (import { globalStylesLinksDataKey } from '../../store/private-keys';
) and access it like settings[ globalStylesLinksDataKey ]
.
I just logged the settings out in that hook and I can see the font _links
object, and when I change variations the links update.
I'm not sure that helps, maybe it leads you in the right direction?
Just to clarify and contrast what I was working on with background images, the resolved background image paths are used to build the correct CSS. It's done at this point in the global styles output generation:
gutenberg/packages/block-editor/src/components/global-styles/use-global-styles-output.js
Lines 308 to 314 in ee98b6e
export function getStylesDeclarations( | |
blockStyles = {}, | |
selector = '', | |
useRootPaddingAlign, | |
tree = {}, | |
disableRootPadding = false | |
) { |
tree
contains the embedded _links
object and path resolution is done "on the fly" when outputting CSS.
* Internal dependencies | ||
*/ | ||
import { getDisplaySrcFromFontFace, loadFontFaceInBrowser } from './utils'; | ||
import { store as editorStore } from '../../store'; |
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.
Let's use blockEditorStore
to be specific and differentiate from the editor package's store.
import { store as editorStore } from '../../store'; | |
import { store as blockEditorStore } from '../../store'; |
return; | ||
} | ||
|
||
const src = resolveThemeFontFaceSrc( fontFace.src, _links ); |
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.
const src = resolveThemeFontFaceSrc( fontFace.src, _links ); | |
const src = getThemeFontFaceSrc( fontFace.src, _links ); |
Nit: for code clarity purposes we should probably avoid using "resolve" inside an async
function. I thought this was async.
// Firefox needs the font name to be wrapped in double quotes meanwhile other browsers don't. | ||
if ( window.navigator.userAgent.toLowerCase().includes( 'firefox' ) ) { | ||
output = `"${ output }"`; | ||
} |
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.
What happens if we just let all browsers have the wrapped version?
if ( output.includes( ',' ) ) { | ||
output = output | ||
.split( ',' ) | ||
// Finds the first item that is not an empty string. | ||
.find( ( item ) => item.trim() !== '' ) | ||
.trim(); | ||
} |
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.
Can we describe why we need to do this? I can see what but at a casual glance I'm not sure as to why.
What?
Add a client-side font face resolver in the editor.
Why?
To load font faces dynamically in the browser, for example, when they change as a result of applying a theme style variation.
Fixes: #59965
How?
The editor font face resolver loads the font faces of the global styles. It keeps track of the already loaded assets to avoid loding them twice.
Testing Instructions
Try to load a style variation and check that the fonts referenced in those style variations load as expected. See the screencast below as example.
Screenshots or screencast
2024-09-05.13-05-42.mp4
Follow up:
Make the editor fonts resolver something exposed out of the package so it can be accessed by other parts of the code, for example the font library. This will also allow us to have a global registry of already loaded font assets in the client. (Currently there are 2 one is used by the font library (part of the
edit-site
package and one by the site editor (part of theblock-editor
package). Having a unique list of already loaded assets would save us from making some repeated requests. This could help to reduce the partial code duplication of some of the utils functions used here too.