Fix site editor broken when fontWeight is not defined or is an integer in theme.json or theme styles

[Imran92]

Fixes #64952

First got report in Automattic/wp-calypso#94033

What?

In some themes (for example, Pendant), when users selected a style and then tried changing the font, the site editor crashed.

Upon digging, I found that it began after this #61915. The PR added a very cool feature. Previously, we showed a fixed set of font weights in the editor, which was not always correct. This PR made it dynamic. But there were three small but tricky issues, all of them caused the site editor to stop working.

1. trim() error

If we look at this line, we'll notice that here ( /\s/.test( face.fontWeight.trim() ) the trim function will be called on the fontWeight variable even if it's undefined or integer, in which case it will throw an error.

Now, the question is, can fontWeight be undefined or integer? According to the theme.json schema, we can see that it can be both. Because it's not required, and can be either a string or an integer (may change in later versions I think).

As real example, the Pendant theme doesn't have a fontWeight explicitly set here.

2. Empty weight option

In this portion a few lines below, we check if fontWeight value is present, and push it in a weight list if it does. But, we get the fontWeight value by calling the formatFontWeight which returns an empty object even when fontWeight doesn't exist. So this check always passes. Resulting in listing an empty weight in the option.

3. Infinite render loop and editor crash when weight is an integer

This was the most tricky one as there was nothing helpful found in debug console, just broke the render depth after a while and crashed.

In this line, you'll notice that we're trying to fetch the nearest weight value to set to the editor, by calling the findNearestFontWeight function. But if we look inside the function, we'll see that it returns an empty string if the fontWeight value of the type string. But we've already seen in the screenshot above that it's possible to have an integer as a fontWeight value. Pendant theme has a few of those as well. So this function will not work if the fontWeight value is an integer. So it kept trying to get a value, till it caused it to crash.

gutenberg/packages/block-editor/src/components/global-styles/typography-utils.js

Lines 177 to 179 in 5a87fb0

	if ( ! newFontWeightValue || typeof newFontWeightValue !== 'string' ) {
	return '';
	}

Initially I thought about changing all the values as expected on the Pendant theme. But then decided it won't probably be the right thing to do because there can be other themes like this as well. Also, it follows the theme.json schema.

About the 'number' === typeof approach that I took, instead of calling toString() on everything is because I noticed, even though it says fontWeight can only be an integer or a string, this line indicates that fontWeight can sometimes be an object too (?). So I didn't want to risk breaking any backward compatibility.

Why?

To fix the editor.

How?

Having additional null checks and integer to string conversions.

Testing Instructions

1. Any site
2. installed Pendant theme
3. Browse to Site Editor, then Styles.
4. Select a style
5. Drilled down into Styles to Headings.
6. Select H1.
7. Select any other font
8. Editor shouldn't crash
9. Now try with different other themes as well, to make sure nothing broke.

Testing Instructions for Keyboard

Screenshots or screencast

Before

Screen.Recording.2024-08-31.at.3.01.18.AM.mov

After

Screen.Recording.2024-08-31.at.4.53.08.AM.mov

[github-actions]

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: Imran92 <imranh920@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: mikachan <mikachan@git.wordpress.org>
Co-authored-by: annezazu <annezazu@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]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Imran92! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[ramonjd]

Thank your for this PR!

It's possible that this also fixes:

• Typography: numerical font size presets break FontSizePicker #44857

Sorry, same broad topic (type coercion in typography) but different 😄

[ramonjd]

Thank you for getting this started! I can replicate the error (Maximum update depth exceeded).

And this branch as it is fixes the problem for me.

I added some comments/questions.

I also think that it would be good to add test coverage too.

I did a bit of poking around in theme.json to see if we could coerce or warn about value types that don't conform to the schema definitions (related issue #57641) but I think that's a separate issue.

cc @mikachan / @creativecoder whose font library knowledge is deeper than mine

[ramonjd]

If we're guarding against extra whitespace for space-separated values, I'm wondering if we should trim all face.fontWeight string values, e.g.,

		face.fontWeight =
			'string' === typeof face.fontWeight
				? face.fontWeight.trim()
				: face.fontWeight;
		if ( /\s/.test( face.fontWeight ) ) {

[Imran92]

Thanks so much for checking this and the suggestions @ramonjd ! I thought about writing tests for it right after adding the solution but then decided to put in the effort after we agreed on the solution's approach. As I'm very new to this area, there could be other ways to solve it which could be way better :p So wanted to make sure I'm doing it right.

If we're guarding against extra whitespace for space-separated values, I'm wondering if we should trim all face.fontWeight

This is a very nice idea. My only concern about this is that we'll probably be doing the regex test even on numeric values in the right next line.

Also, the regex test will return true if the value of fontweight variable is an object. It should probably never be an object, but this line of here makes me wonder if it sometimes is an object for some reason.

[ramonjd]

Ah yes, true. Thanks for the explanation 👍🏻

I thought about writing tests for it right after adding the solution but then decided to put in the effort after we agreed on the solution's approach.

Maybe tests will reveal some optimizations too. For now, it LGTM to me since it fixes a pretty major bug 🚀

[ramonjd]

Could we coerce all values to string at the top?

I'm thinking, if we make sure all items in the fontWeights array are string here, at the source, the toString() coercions above in packages/block-editor/src/components/global-styles/typography-utils.js might not be required.

[Imran92]

This is a very good idea @ramonjd ! I actually tried to implement it. But later I stopped, because I was afraid of adding an regression because we're modifying the value fetched from the JSON. So for now, I'm just avoiding doing it in this PR. But WDYT about creating an issue where we properly test all implications of and modify the values at the top?

Thank you!

[ramonjd]

Fair enough - probably a premature optimization. Good to have the bug fixed first 😄

[mikachan]

Thanks for working on this, @Imran92! This fixes the issues you describe locally for me when testing with Pendant.

It would be great to update the tests for these functions before bringing this in, but otherwise, this is looking great.

[Imran92]

Thank you so much for checking it and your super helpful reviews @ramonjd and @mikachan! I've updated the PR as per the suggestions and have also added tests. Can you kindly take a look again?

[mikachan]

Thanks @Imran92!

The additional tests and changes look good to me; the tests are passing for me locally, I believe they cover the new changes, and the functionality is working well still in the Editor.

I'm going to approve, but happy to wait for @ramonjd to take another look before we bring this in.

[ramonjd]

Thanks for fixing this, and for the tests.

[ramonjd]

Also, thanks for the PR @Imran92 - I hope this is the first of many 🚀