Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Load variation font faces with theme relative urls using backend provided full urls (_links) #65019
Load variation font faces with theme relative urls using backend provided full urls (_links) #65019
Changes from 12 commits
6219a6d
1e8ef55
65f3754
77f7638
0bed01d
48bfe65
65d147a
c6a32f9
5a9e2d0
27b5e64
463fa95
9bd3378
5cecd18
c262896
8ced871
ee98b6e
1461989
8172039
7cb4c01
20a00e3
f2ad8e7
5a3b172
db89bf6
32868cd
efd9562
905f0d8
e85076a
a1cdf2c
2250a0c
59ce5a4
d94e2f2
607740a
b69ae26
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do you mean when the
theme.json
data is read from the file / database?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.
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 theEditorStyles
component.https://github.com/WordPress/wordpress-develop/blob/bf09fe506620678fb82c3b872309edda0ed8ce61/src/wp-includes/block-editor.php#L497-L553
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.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 likesettings[ 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
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.
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
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
andfontFamilies
as dependencies. I don't know if that will work, but it's the first thing I'd try without having extra context.