WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.45k stars 4.17k forks source link

Font families with names containing numbers renders presets wrongly #53695

Open matiasbenedetto opened 1 year ago

matiasbenedetto commented 1 year ago

Description

If you have a font family with a slug containing a number the presets renders presets wrongly and the font is not loaded. Example with a font called N27 and using slug n27.

In this case CSS property that is given to the elements is:

font-family: var(--wp--preset--font-family--n27);

Meanwhile the preset definition in <style id="global-styles-inline-css"> is:

--wp--preset--font-family--n-27: n27;

Pay attention to the dash in the preset definition vs the lack of it in the element font-family value: ...--n-27 vs ...--n27

Step-by-step reproduction instructions

  1. Add a font family with a number in their name to your theme.json file.

Example:

{
    "fontFace": [
        {
            "fontFamily": "n27",
            "fontStyle": "normal",
            "fontWeight": "700",
            "src": [
                "file:./assets/fonts/n27_normal_700.woff2"
            ]
        },
        {
            "fontFamily": "n27",
            "fontStyle": "normal",
            "fontWeight": "400",
            "src": [
                "file:./assets/fonts/n27_normal_400.woff2"
            ]
        },
        {
            "fontFamily": "n27",
            "fontStyle": "italic",
            "fontWeight": "400",
            "src": [
                "file:./assets/fonts/n27_italic_400.woff2"
            ]
        }
    ],
    "fontFamily": "n27",
    "slug": "n27"
}
  1. Set this font family to an element and save
  2. Load the public page displaying the element.
  3. Check that the font family is not applied.

Screenshots, screen recording, code snippet

You can download and use these theme to test. It is using the font called N27:

ih23-structure.zip

Environment info

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

Yes

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

Yes

jorgefilipecosta commented 1 year ago

Hi @matiasbenedetto, I did some debugging and it seems core it self never generates styles with font-family: var(--wp--preset--font-family--n27); The sample theme you provided is the one that one that cointains rules like "fontFamily": "var(--wp--preset--font-family--n27)", on its theme.json. I guess these need to be updated to "fontFamily": "var(--wp--preset--font-family--n-27)",?

matiasbenedetto commented 1 year ago

The sample theme you provided is the one that one that cointains rules like "fontFamily": "var(--wp--preset--font-family--n27)", on its theme.json. I guess these need to be updated to "fontFamily": "var(--wp--preset--font-family--n-27)",?

Hi, I don't think we should update the theme.json contents to match the preset because what I think is failing is the preset generation when a font family has a slug with a number. Font family slug is n27 and not n-27 so the preset should be --wp--preset--font-family--n27 and not --wp--preset--font-family--n-27.

If the font family slug is arial the preset rendered is --wp--preset--font-family--arial: Arial; and not something like--wp--preset--font-family--ari-al: Arial;

It is the preset in <style id="global-styles-inline-css"> what is not working as expected.

jorgefilipecosta commented 1 year ago

Hi @matiasbenedetto, thank you for the clarification. I managed to reproduce the issue. Essentially, in all instances where we employ --wp--preset--font-family--n-27 (kebab case version), when implementing changes in the global styles panel, we don't employ kebab case. To me, the solution seems to involve using kebab case consistently.

I concur that ideally, we should utilize this version --wp--preset--font-family--n27; the slug is n27, and it's best not to alter the slug. However, I'm uncertain whether we can currently make this change, considering that at the block level, we use the n-27 version. I suppose the simple resolution would be to adjust the global styles saving to n-27 to match other instances. But despite being a complex and somewhat risky change, we could seize the opportunity to rectify the matter correctly and switch everything to n27 (without changing the slug).

@oandregal, do you believe removing the kebab case n-27 and using n27 is a feasible step at this moment? Or would you prefer to address the specific case where n27 is used and switch eveythting to n-27?

matiasbenedetto commented 1 year ago

I'm uncertain whether we can currently make this change, considering that at the block level, we use the n-27 version. I suppose the simple resolution would be to adjust the global styles saving to n-27 to match other instances.

I'm not sure why we are treating or should treat slugs like n27 differently from slugs like arial or inter. @jorgefilipecosta could you add more context to that, please?

jorgefilipecosta commented 1 year ago

The slug is passing by a text transform called kebab case. The idea of this text transform is if a slug like FontArialStrong is passed which would not make a sense has a css variable or class we would transform it to font-arial-strong which is more common as css names. The kebab case text transform also separates number and letters with "-", so n27 becomes n-27 following the kebab case transform. The transform was used in slugs (colors, font sizes etc) for a long time even before Gutenberg went to core. That's why I'm not sure it is change we can do now.

oandregal commented 1 year ago

@matiasbenedetto this is expected behavior.

The reason is that the kebabCase function by lodash that we use in the client behaves that way. You can check it by going to that page and click on "Try on REPL". Then, add _.kebabCase('n27'); and execute. The result is n-27. Because the client-side code serializes some block supports to the post data and it is stored in the database, we cannot change how it works. For example, a font size with slug h1 would be serialized to the block HTML as .has-h-1-font-size. If we change it and it becomes .has-h1-font-size, what do we do with older content?

When the server code for theme.json was introduced later, it needed to behave the same way. This is why we added _wp_to_kebab_case in https://github.com/WordPress/gutenberg/pull/32766 and updated block supports in https://github.com/WordPress/gutenberg/pull/35751

matiasbenedetto commented 1 year ago

Thanks for the context. I'm wondering what would be a good way to solve the issue.

jorgefilipecosta commented 1 year ago

Thanks for the context. I'm wondering what would be a good way to solve the issue.

I guess the fix is change the styles we apply on global styles also to have the missing kebab case transform.

oandregal commented 1 year ago

I'm wondering what would be a good way to solve the issue.

Would you be able to expand what's the issue? I've done some testing and it works fine for me.

I used TT3 and added n27 as a font face as described in the issue. Then,

In the post editor, I set the n27 font family in a paragraph. The paragraph is attached the CSS class, as expected:

<p class="has-n-27-font-family">...</p>

I also see that the global-styles-inline-css contains the definition of the font family:

body {
    --wp--preset--font-family--n-27: n27;
}
.has-n-27-font-family {
    font-family: var(--wp--preset--font-family--n-27) !important;
}

I set the font family in the theme.json of the theme for a particular block:

{
  "core/post-author": {
    "typography": {
      "fontFamily": "var(--wp--preset--font-family--n-27)"
    }
  }
}

and it correctly generates the corresponding styles:

.wp-block-post-author{
  font-family: var(--wp--preset--font-family--n-27);
}
jorgefilipecosta commented 1 year ago

I'm wondering what would be a good way to solve the issue.

Would you be able to expand what's the issue? I've done some testing and it works fine for me.

I used TT3 and added n27 as a font face as described in the issue. Then,

In the post editor, I set the n27 font family in a paragraph. The paragraph is attached the CSS class, as expected:

<p class="has-n-27-font-family">...</p>

I also see that the global-styles-inline-css contains the definition of the font family:

body {
    --wp--preset--font-family--n-27: n27;
}
.has-n-27-font-family {
    font-family: var(--wp--preset--font-family--n-27) !important;
}

I set the font family in the theme.json of the theme for a particular block:

{
  "core/post-author": {
    "typography": {
      "fontFamily": "var(--wp--preset--font-family--n-27)"
    }
  }
}

and it correctly generates the corresponding styles:

.wp-block-post-author{
  font-family: var(--wp--preset--font-family--n-27);
}

The issue is on global styles UI if we set the n27 font family it generates styles as "fontFamily": "var(--wp--preset--font-family--n27)" instead of "fontFamily": "var(--wp--preset--font-family--n-27)". I think it is something on front end.

oandregal commented 1 year ago

The issue is on global styles UI if we set the n27 font family it generates styles as "fontFamily": "var(--wp--preset--font-family--n27)" instead of "fontFamily": "var(--wp--preset--font-family--n-27)".

Oh, I see. I can reproduce now. It happens both in the site editor and front-end. This needs to be fixed to use the expected kebab case.

fabiankaegy commented 1 year ago

This also happens for spacing variables same as typography ones. If you have a spacing preset with the slug 2xl global styles prints it as --wp--preset--spacing--2xl whilst the frontend output and the generated variable prints as --wp--preset--spacing--2-xl

justintadlock commented 3 months ago

Based on the linked discussions below, the correct property name should be --wp--preset--font-family--n-27 (this should be used everywhere):