Font Library: Merge the font faces instead of overriding when parsing settings by arthur791004 · Pull Request #59119 · WordPress/gutenberg

arthur791004 · 2024-02-16T07:00:16Z

What?

This PR is to combine the font faces provided by ALL origins instead of overriding.

Why?

Referring to #58764 (comment), the installed font variants override the font variants provided by the theme. For example, if a theme provides a Cardo font with 400, and 500 font variants and people install a new Cardo font with a 600 font variant, then only the Cardo font with a 600 font variant will take effect. As a result, we have to merge the font faces across different origins to prevent this issue.

How?

Instead of overriding, now we check whether the new font face exists or not. If it exists, we will merge the new one into the existing font face. If not, we just add it to the array.

Testing Instructions

Make this change take effect by commenting out the core files

Run wp-env run cli bash
Open the wp-settings.php

Comment out the following lines

// require ABSPATH . WPINC . '/fonts/class-wp-font-face-resolver.php';
// require ABSPATH . WPINC . '/fonts/class-wp-font-face.php';           
// require ABSPATH . WPINC . '/fonts.php';

Open a site with TT4
Go to the Editor
Create the following paragraph:
- Cardo Normal. Set the Font Family to Cardo, and the Appearance to “Regular”
- Cardo Bold. Set the Font Family to Cardo, and the Appearance to “Bold”
- Cardo Normal Italic. Set the Font Family to Cardo, and the Appearance to “Regular Italic”
Install the Cardo font with 400 font variant from the Install Fonts tab of the Font Library Modal
Save changes and refresh the page
Make sure the rendered font of those paragraphs is Cardo-Regular

Testing Instructions for Keyboard

Screenshots or screencast

Here is the video to reproduce the issue:

font-library-installed-fonts-overrides-theme-fonts.mov

github-actions · 2024-02-16T07:00:29Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: arthur791004 <arthur791004@git.wordpress.org>
Co-authored-by: okmttdhr <okat@git.wordpress.org>
Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-02-16T07:01:33Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/compat/wordpress-6.4/fonts/font-face/class-wp-font-face-resolver.php

… settings

okmttdhr · 2024-02-16T07:56:49Z

+					foreach ( $font_faces_to_add as $font_face_to_add ) {
+						$offset = static::search_font_face( $font_face_to_add, $fonts[ $font_family_name ] );
+						if ( false !== $offset ) {
+							$fonts[ $font_family_name ][ $offset ] = array_merge( $fonts[ $font_family_name ][ $offset ], $font_face_to_add );


The use of array_merge here combines the properties of the existing font face ($fonts[ $font_family_name ][ $offset ]) with those of the new font face ($font_face_to_add). This operation prioritizes the properties from $font_face_to_add when the same keys exist in both arrays. For instance, if both arrays contain a src property, the src from $font_face_to_add might overwrite the one in the existing font face.

Should we keep the original font face when all font-family & font-style & font-weight are the same?

I thought just $fonts[ $font_family_name ][] = $font_face_to_add; does a good job of merging the different variants of the original and the installed font.
WDYT? 💭 (Or CMIIW) @arthur791004

That's a fair point. IMHO, I assume it's fine to override the src if all the font-family & font-style & font-weight are the same as the fonts here are for generating and printing font-face styles to the HTML. I don't think we have to print the same font face multiple times. Do you have any concerns about that?

I was somewhat hesitant to overwrite the built-in fonts because they might have already been set (I mean I might prefer array_merge( $font_face_to_add, $fonts[ $font_family_name ][ $offset ] ); instead of array_merge( $fonts[ $font_family_name ][ $offset ], $font_face_to_add );). However, as you mentioned,

I assume it's fine to override the src if all the font-family & font-style & font-weight are the same as the fonts here are for generating and printing font-face styles to the HTML

so it should be fine. 👍

matiasbenedetto

Thanks, @arthur791004 and @okmttdhr , for working on this!

I think this change is working mostly as expected, but it has a weak point produced by relying on one unnecessary aspect of the current implementation.

gutenberg/lib/compat/wordpress-6.4/fonts/font-face/class-wp-font-face-resolver.php

Line 79 in c915f0b

    
           $fonts[ $font_family_name ] = static::convert_font_face_properties( $definition['fontFace'], $font_family_name );

could be

$fonts[] = static::convert_font_face_properties( $definition['fontFace'], $font_family_name );

I don't see any reason why $fonts should be an associative array and not an indexed one. Is there still any reason for that? Maybe @hellofromtonya has thoughts about that.

If we convert $fonts into an indexed array, the complexity added by this PR won't be needed. So, we can solve the problem without adding extra complexity and extra logic in a part of the code that can impact the public frontend rendering performance.

matiasbenedetto · 2024-02-22T16:55:56Z

Apart from that, I think it would be useful to implement these changes in the WordPress core repo first to be able to run the unit tests for this functionality.

matiasbenedetto · 2024-02-22T20:40:59Z

There's an alternative approach implementing what i mentioned here: #59119 (review) I'd appreciate if you can take a look.

arthur791004 · 2024-02-23T04:32:57Z

Reviewed WordPress/wordpress-develop#6161 (review) and it looks good 🙂

Do we still need this PR? I'm unsure whether we have a mechanism to sync changes from Core.

matiasbenedetto · 2024-02-23T13:35:43Z

Do we still need this PR?

Nope no longer needed.

I'm unsure whether we have a mechanism to sync changes from Core.

In this case, we need to manually copy these changes from the core to the Gutenberg repo when this is merged.

arthur791004 · 2024-02-26T04:54:18Z

Okay, thanks!

arthur791004 · 2024-02-26T04:54:51Z

Closing it in favor of WordPress/wordpress-develop#6161 and #59321

arthur791004 requested a review from spacedmonkey as a code owner February 16, 2024 07:00

arthur791004 self-assigned this Feb 16, 2024

arthur791004 added the [Feature] Font Library label Feb 16, 2024

arthur791004 added the [Type] Bug An existing feature does not function as intended label Feb 16, 2024

arthur791004 mentioned this pull request Feb 16, 2024

Font Weight Conflicts Between Built-in and Installed Fonts #58764

Closed

arthur791004 removed the request for review from spacedmonkey February 16, 2024 07:20

Font Library: Merge the font faces instead of overriding when parsing…

a4dd8d6

… settings

arthur791004 force-pushed the fix/font-library-install-fonts-variant-overrides-theme-fonts branch from cbdb05d to a4dd8d6 Compare February 16, 2024 07:22

arthur791004 mentioned this pull request Feb 16, 2024

Heading Font Weight Not Applying in Adventurer Automattic/wp-calypso#84590

Closed

okmttdhr reviewed Feb 16, 2024

View reviewed changes

okmttdhr approved these changes Feb 19, 2024

View reviewed changes

matiasbenedetto requested a review from hellofromtonya February 20, 2024 14:01

matiasbenedetto reviewed Feb 22, 2024

View reviewed changes

matiasbenedetto mentioned this pull request Feb 22, 2024

Font face resolver: print font faces from font families defined in all theme.json origins WordPress/wordpress-develop#6161

Closed

arthur791004 closed this Feb 26, 2024

arthur791004 deleted the fix/font-library-install-fonts-variant-overrides-theme-fonts branch February 26, 2024 04:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font Library: Merge the font faces instead of overriding when parsing settings#59119

Font Library: Merge the font faces instead of overriding when parsing settings#59119
arthur791004 wants to merge 1 commit intotrunkfrom
fix/font-library-install-fonts-variant-overrides-theme-fonts

arthur791004 commented Feb 16, 2024

Uh oh!

github-actions Bot commented Feb 16, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Feb 16, 2024

Uh oh!

okmttdhr Feb 16, 2024 •

edited

Loading

Uh oh!

arthur791004 Feb 16, 2024

Uh oh!

okmttdhr Feb 19, 2024 •

edited

Loading

Uh oh!

matiasbenedetto left a comment •

edited

Loading

Uh oh!

matiasbenedetto commented Feb 22, 2024

Uh oh!

matiasbenedetto commented Feb 22, 2024

Uh oh!

arthur791004 commented Feb 23, 2024

Uh oh!

matiasbenedetto commented Feb 23, 2024 •

edited

Loading

Uh oh!

arthur791004 commented Feb 26, 2024

Uh oh!

arthur791004 commented Feb 26, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

arthur791004 commented Feb 16, 2024

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Uh oh!

github-actions Bot commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 16, 2024

Uh oh!

okmttdhr Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthur791004 Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

okmttdhr Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matiasbenedetto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matiasbenedetto commented Feb 22, 2024

Uh oh!

matiasbenedetto commented Feb 22, 2024

Uh oh!

arthur791004 commented Feb 23, 2024

Uh oh!

matiasbenedetto commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthur791004 commented Feb 26, 2024

Uh oh!

arthur791004 commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Feb 16, 2024 •

edited

Loading

okmttdhr Feb 16, 2024 •

edited

Loading

okmttdhr Feb 19, 2024 •

edited

Loading

matiasbenedetto left a comment •

edited

Loading

matiasbenedetto commented Feb 23, 2024 •

edited

Loading

arthur791004 commented Feb 26, 2024 •

edited

Loading