Load variation font faces with theme relative urls using backend provided full urls (_links) #65019
Conversation
matiasbenedetto
added
[Type] Enhancement
|
Size Change: +261 B (+0.01%) 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. |
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
matiasbenedetto
requested review from
justintadlock,
carolinan and
richtabor
Co-authored-by: Sarah Norris <1645628+mikachan@users.noreply.github.com>
|
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.
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.
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.
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.
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.
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.
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
There was a problem hiding this comment.
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.
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
packages/block-editor/src/components/editor-fonts-resolver/utils.js
Outdated
Show resolved
Hide resolved
…_Resolver_Gutenberg::get_resolved_theme_uris
There was a problem hiding this comment.
Thanks for all the work here.
I tweaked the attached theme slightly so that the previews would use the primary font as well.
Kapture.2024-10-11.at.12.31.44.mp4
It's working well from my perspective - though some testing from folks with more experience in the font library would be great.
Happy to test out the backport PR too.
I cheekily added a bit of PHP unit test coverage.
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Show resolved
Hide resolved
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
There was a problem hiding this comment.
I used your Theme for testing (thanks so much for taking the time to make testing easier) and the fonts loaded on demand as I switched between the Style Variations.
The one thing I noticed is that the fonts are requested again, even if they have previously been loaded.
In the screencapture below you'll see the requests to fonts going out again the second time round.
I don't think it represents a huge problem though, as even on slow network, caching seems to kick in and the fonts are displayed immediately despite the unwanted network request.
Screen.Capture.on.2024-10-15.at.11-48-08.mp4
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Show resolved
Hide resolved
|
@kevin940726 @ndiego @colorful-tones Shall we pursue this for 6.7? If so then we'll need a backport PR as per the failing check below. |
For visibility, there's an alleged, but yet unconfirmed performance regression in the way Theme JSON resolver is resolving theme JSON relative paths. Hopefully it will be confirmed, and even better, addressed this week if it is indeed found to be correct. WordPress/performance#1572 (comment) If it is real, I think adding more checks might exacerbate things. Edit: we could test this in the Core backport PR for this change anyway. |
Based on the progress here I would say we can keep it on the radar for 6.7, but not a solid 'yes'. 😃 |
@getdave thanks for testing! Screencast.from.15-10-24.14.33.47.webm |
|
Trac ticket created: https://core.trac.wordpress.org/ticket/62231 |
Update: @aaronrobertshaw and I spent a big chunk of the day testing and couldn't confirm the regression, so I think we're good to go. Moreover, the CI performance tests are happy as far as I can tell. I think the file resolution probably needs to be cached at some point. I have it on my list to look at. Thank you! |
Haha so I did. Good catch. I did think it was strange! |
|
I submitted a simpler alternative to this PR to fix the problem directly in core: WordPress/wordpress-develop#7581 |
|
I'm sorry I have not found the time to test this. But I don't want you to feel like you are forced to come up with another option (7581) only because of lack of reviews. |
@matiasbenedetto I noticed this. Is there a reason why you decided to raise directly in Core? Is is a more suitable place for the fix? Does adding to Gutenberg just cause more overhead? Is it a problem of reviews? If you're lacking reviews and this then I'm happy to flag for the release leads to take a look at. |
@carolinan @getdave, thanks for your consideration! I submitted WordPress/wordpress-develop#7581 because I think it's a simpler approach, easier to debug and extend, and, for that reason, better. It's not about reviews. |
|
Sorry, I came to this late. Are we still aiming this for 6.7? Should we focus on landing WordPress/wordpress-develop#7581 and close this? Sorry if I'm misunderstanding something! 🙇 |
andrewserong
removed
the
Backport to WP 6.7 Beta/RC
It looks like 7581 has been committed (WordPress/wordpress-develop#7581 (comment)), so I've removed the Backport label for 6.7 and moved this PR to punted to 6.8 in the project board, as my understanding is that this PR is no longer required for 6.7. Feel free to update if that's not correct @matiasbenedetto! I wasn't sure if you still wanted to work on this PR, so have otherwise left it open for now. |
|
Thanks everyone for the feedback. |
What?
Use backend provided full urls to load font assets with theme relative urls.
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?
Uses the links provided by the global styles rest API to load the font assets using the full URLs instead of the theme-relative ones that don't work on the client.
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.
I created a theme to test this quickly and visually:
font-variants.zip
Before:
Screencast.from.09-10-24.13.25.05.webm
After:
Screencast.from.09-10-24.13.23.40.webm
Screenshots or screencast
Testing with TT4. It is the same as with the custom theme I created but the typographies are not so different visually so the changes are harder to see.
2024-09-05.13-05-42.mp4