Skip to content
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

Font Family control component causing fatal error when rendering a block #59778

Open
gregWierzba opened this issue Mar 12, 2024 · 6 comments · May be fixed by #59846
Open

Font Family control component causing fatal error when rendering a block #59778

gregWierzba opened this issue Mar 12, 2024 · 6 comments · May be fixed by #59846
Labels
[Feature] Typography Font and typography-related issues and PRs Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended

Comments

@gregWierzba
Copy link

Description

I found an issue in Wodpress Font Family control component.

The problem is that is we don't pass default value of the fontFamilies attribute, the Font Family control gets preset values from the WP's internal getSettings hook, and the the value of typography.fontFamilies returned from this hook looks like :

{theme: [{font_family_1_attrs}, {font_family_2_attrs}, ...]}
but, when you look in the source code here, you can see that there's a bug, they're not accessing the .theme from inside the blockLevelFontFamilies object and that's what's causing the issue.

That causes fatal error in rendering block, when On Block settings panel, you click + on typography:
Zrzut ekranu 2024-03-12 o 12 11 10

fontFamilies = blockLevelFontFamilies;

Step-by-step reproduction instructions

Can be reproduced with recent version of WPML

  1. create a post
  2. add language switcher block
  3. On Block settings panel, click + on typography

Screenshots, screen recording, code snippet

Now its:

if ( ! fontFamilies ) {
	fontFamilies = blockLevelFontFamilies;
}

Should be

if ( ! fontFamilies ) {
	fontFamilies = blockLevelFontFamilies.theme; 
}

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

No

@gregWierzba gregWierzba added the [Type] Bug An existing feature does not function as intended label Mar 12, 2024
@annezazu annezazu added the Needs Technical Feedback Needs testing from a developer perspective. label Mar 12, 2024
@annezazu annezazu changed the title issue in Wodpress Font Family control (on wp 6.5) Font Family control component causing fatal error when rendering a block Mar 12, 2024
@annezazu
Copy link
Contributor

@matiasbenedetto is this something you can take a look at?

@matiasbenedetto
Copy link
Contributor

matiasbenedetto commented Mar 12, 2024

Hi @gregWierzba, thanks for submitting this ticket.
I couldn't reproduce this issue just by using the Gutenberg plugin.

Now its:
if ( ! fontFamilies ) {
fontFamilies = blockLevelFontFamilies;
}

Should be

if ( ! fontFamilies ) {
fontFamilies = blockLevelFontFamilies.theme;
}

This change doesn't seem right because font families can live in .custom, too.

I'm not sure about which translation plugin you are using to test. Could you please link it?
Can this problem be plugin-specific?
Could you add a screencast featuring the issue?
Could you add to the ticket description the version of WordPress and Gutenberg you are running?

@matiasbenedetto matiasbenedetto added the [Feature] Typography Font and typography-related issues and PRs label Mar 12, 2024
@zeopix
Copy link

zeopix commented Mar 13, 2024

Hello @matiasbenedetto thanks for checking this,

Here the code that is triggering this in our plugin:

          <FontFamilyControl
          value={...}
          onChange={...}
          />

We do not specify fontFamilies attribute at all, because we just want to offer the theme presets (and now the custom fonts).

It crashes in WP 6.5-beta2.

In WP 6.4, the variable blockLevelFontFamilies value was:

  {
      "fontFamily": "\"Inter\", sans-serif",
      "name": "Inter",
      "slug": "body",
      ...
  }

In WP 6.5-beta2:

{
    "theme": [
        {
            "fontFamily": "\"Inter\", sans-serif",
            "name": "Inter",
            "slug": "body",
            ...
        },
       ...
    ],
   "custom"...
}

So, a block that uses FontFamily without specifying any value, will not work.

This can be seen in the WPML Language Switcher Block.

I've done some tests in my environment, adding some custom fonts. Replacing the lines that @gregWierzba mentioned, by the following:

	if ( ! fontFamilies ) {
		const { theme, custom } = blockLevelFontFamilies;
		fontFamilies = theme !== undefined ? theme : [];
		if ( custom !== undefined ) {
			fontFamilies = [ ...fontFamilies, ...custom ];
		}
	}

Works well for me, the block does not crash and uses the theme fonts plus custom ones as fallback.

Does this make sense to you?

EDIT:
added PR.

@zeopix zeopix linked a pull request Mar 13, 2024 that will close this issue
@Molovicmilos
Copy link

Hi @gregWierzba , @zeopix and others,

This is my first comment in this repository, so I'll keep it brief.

I frequently use the FontFamilyControl in my custom plugin as follows:

import { __experimentalFontFamilyControl as FontFamilyControl } from "@wordpress/block-editor";

This worked perfectly with Gutenberg version 17.7, but the component broke starting from version 17.8.0. Since then, I haven't been able to update the plugin.

Here’s one potential solution:

const custom = useSettings("typography.fontFamilies")[0]?.custom ?? [];
const theme = useSettings("typography.fontFamilies")[0]?.theme ?? [];
const fontFamilies = [...custom, ...theme];

However, I'm concerned that future WordPress core updates might break my plugin again.

I believe the best approach would be to merge fontFamilies in the font-family/index.js similarly to how it’s done in the typography-panel.js file.

This approach ensures that the fontFamilies will be an array that can be mapped over - LN29.

@Molovicmilos
Copy link

Regarding the above comment maybe I should mention the #59846.

@t-hamano
Copy link
Contributor

I determined that this issue was caused by #58951. As I remember, this PR caused the font family to merge all origins.

I believe the best approach would be to merge fontFamilies in the font-family/index.js similarly to how it’s done in the typography-panel.js file.

I also think this approach is correct.

@t-hamano t-hamano linked a pull request May 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants