gutenberg icon indicating copy to clipboard operation
gutenberg copied to clipboard

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

Open arthur791004 opened this issue 1 year ago • 6 comments

What?

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

Why?

Referring to https://github.com/WordPress/gutenberg/issues/58764#issuecomment-1947778291, 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:

https://github.com/WordPress/gutenberg/assets/13596067/11114225-a017-407c-b75b-874ce78257cc

arthur791004 avatar Feb 16 '24 07:02 arthur791004

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 <[email protected]>
Co-authored-by: okmttdhr <[email protected]>
Co-authored-by: matiasbenedetto <[email protected]>

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

github-actions[bot] avatar Feb 16 '24 07:02 github-actions[bot]

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! :heart:

View changed files
:grey_question: lib/compat/wordpress-6.4/fonts/font-face/class-wp-font-face-resolver.php

github-actions[bot] avatar Feb 16 '24 07:02 github-actions[bot]

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 avatar Feb 22 '24 16:02 matiasbenedetto

There's an alternative approach implementing what i mentioned here: https://github.com/WordPress/gutenberg/pull/59119#pullrequestreview-1896329058 I'd appreciate if you can take a look.

matiasbenedetto avatar Feb 22 '24 20:02 matiasbenedetto

Reviewed https://github.com/WordPress/wordpress-develop/pull/6161#pullrequestreview-1897356295 and it looks good 🙂

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

arthur791004 avatar Feb 23 '24 04:02 arthur791004

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.

matiasbenedetto avatar Feb 23 '24 13:02 matiasbenedetto

Okay, thanks!

arthur791004 avatar Feb 26 '24 04:02 arthur791004

Closing it in favor of https://github.com/WordPress/wordpress-develop/pull/6161 and https://github.com/WordPress/gutenberg/pull/59321

arthur791004 avatar Feb 26 '24 04:02 arthur791004