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 families: data from theme.json is not properly registered #50576

Closed
oandregal opened this issue May 12, 2023 · 6 comments
Closed

Font families: data from theme.json is not properly registered #50576

oandregal opened this issue May 12, 2023 · 6 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@oandregal
Copy link
Member

oandregal commented May 12, 2023

The mechanism for registering font families client-side depends on every font family being registered at the theme level. It should be able to register font families from any origin: default, theme or custom (user), like any other preset does.

Test with data coming from the default origin

  • Use the latest Gutenberg and WordPress with the TwentyTwentyThree theme.
  • Delete the "DM Sans" and "IBM plex mono" from the theme.json of TT3.
  • Copy them into the theme.json bundled with Gutenberg at lib/theme.json.

The expected result was that:

  • The data store core/edit-site has all font families defined by the different origins
    • root.settings.__experimentalFeatures.typography.fontFamilies.default holds the "DM Sans" and "IBM plex mono".
    • root.settings.__experimentalFeatures.typography.fontFamilies.theme holds the rest: "Inter", "System font", "Source Serif pro".
  • All fonts are registered properly at the UI level.
    • Go to the global styles sidebar and open the typography panel for one of the top-level elements. For example, "Text".

What happened instead was that:

  • Data store
    • root.settings.__experimentalFeatures.typography.fontFamilies.default holds the "DM Sans" and "IBM plex mono". This is correct.
    • root.settings.__experimentalFeatures.typography.fontFamilies.theme holded all the fonts, the ones defined by default and the ones defined by the theme. This is incorrect. It should not have the ones defined by default
  • All fonts are registered properly at the UI level. The fonts defined via default don't show the proper labels ("DM Sans" and "Inter Plex mono") but the slug "dm-sans" and "ibm-plex-mono").
Gravacao.de.ecra.a.partir.de.12-05-2023.12.16.38.webm

Test with data coming from the custom origin

  • Use the latest Gutenberg and WordPress with the TwentyTwentyThree theme.
  • Delete the "System font" from the theme.json of TT3.
  • Save into the database the following theme.json for user data (it defines the system font):
{
	"version": 2,
	"isGlobalStylesUserThemeJSON": true,
	"settings": {
	    "typography": {
			"fontFamilies": [
				{
					"fontFamily": "-apple-system,BlinkMacSystemFont,\"Segoe UI\",Roboto,Oxygen-Sans,Ubuntu,Cantarell,\"Helvetica Neue\",sans-serif",
					"name": "System Font",
					"slug": "system-font"
				}
			]
		}
	}
}

The expected result was that:

  • The data store core/edit-site has all font families defined by the different origins
    • root.settings.__experimentalFeatures.typography.fontFamilies.theme holds all the fonts but "System font".
    • root.settings.__experimentalFeatures.typography.fontFamilies.custom holds the "System Font".
  • All fonts are registered properly at the UI level.
    • Go to the global styles sidebar and open the typography panel for one of the top-level elements. For example, "Text".

What happened instead was that:

  • Only the "System Font" was shown in the font family UI.
Gravacao.de.ecra.a.partir.de.12-05-2023.12.31.05.webm

Environment info

I've tested this with Gutenberg trunk and latest WordPress trunk.

@oandregal oandregal added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Feature] Fonts API labels May 12, 2023
@oandregal
Copy link
Member Author

root.settings.__experimentalFeatures.typography.fontFamilies.theme holded all the fonts, the ones defined by default and the ones defined by the theme. This is incorrect. It should not have the ones defined by default

This is because the gutenberg_add_registered_fonts_to_theme_json at https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-theme-json-resolver-gutenberg.php#L278 In that step we're adding the font families defined by default to the list of font families defined by the theme.

I don't know what's the intention of that code or the way forward yet, but thought I'd share what I'm learning as early as possible, to gather thoughts from others.

@hellofromtonya
Copy link
Contributor

Copy them into the theme.json bundled with Gutenberg at lib/theme.json.

I do not foresee WordPress Core bundling local fonts, i.e. providing the local font files within WordPress Core. Rather, this is the job of the theme to define the typography and then extenders via plugins for further customization.

@oandregal Can you provide more context as to why or when local font files would be bundled within WordPress Core?

@oandregal
Copy link
Member Author

oandregal commented May 15, 2023

Can you provide more context as to why or when local font files would be bundled within WordPress Core?

It's fine that we decide not to. My point is that there are issues in how it works now: data defined at the default level is duplicated and is not properly rendered.

We should enforce what's expected. I'd suggest make all origins work the same, as we could change our mind, or people could use a filter to add them to the default origin.
Alternatively, if we don't want to support that, we should make sure that list is always empty.

@hellofromtonya
Copy link
Contributor

hellofromtonya commented May 15, 2023

Test with data coming from the custom origin
Use the latest Gutenberg and WordPress with the TwentyTwentyThree theme.
Delete the "System font" from the theme.json of TT3.
Save into the database the following theme.json for user data (it defines the system font):

I'm confused by the testing steps of removing "System font". This font is not registered with the Fonts API. Why? It does not have a fontFace defined:

				{
					"fontFamily": "-apple-system,BlinkMacSystemFont,\"Segoe UI\",Roboto,Oxygen-Sans,Ubuntu,Cantarell,\"Helvetica Neue\",sans-serif",
					"name": "System Font",
					"slug": "system-font"
				},

Therefore, the Fonts API has no @font-face to generate for it as no properties or src are provided.

But I am curious of the results you had and why these are happening. I'll do some testing to see where this font is stored.

@oandregal
Copy link
Member Author

I learned a few more things about the font library goals. This is how we aim it to work #40363 (comment)

The takeaway for this issue is that the font library and the theme.json models are at odds. The font library currently passes data to the client using the theme.json data for lack of a better alternative, but it should develop its own data flow.


A longer explanation is this. The "origins" is a key concept of theme.json data, and how any preset works. Take colors, for example:

  • the ones defined by core via its theme.json end up being stored under settings.color.palette.default.
  • the ones defined by the theme vias its theme.json end up being stored under settings.color.palette.theme.
  • the ones defined by the user via the global styles sidebar end up being stored under settings.color.palette.custom.

While we only surface this "origins" to the UI for the color preset type, the underlying data flow/storage is the same for every other preset type.

Compare how font families work now;

  • fonts defined by core via its theme.json end up stored both in settings.typography.fontFamily.default and settings.typography.fontFamily.theme origins, and they aren't properly rendered in the UI.
  • fonts defined by the theme via its theme.json end up stored in the settings.typography.fontFamily.theme and they work as expected in the UI.
  • font defined via the PHP API to be used by classic themes or plugins end up stored in settings.typography.fontFamily.theme and they work as expected in the UI.
  • fonts defined by the user via the global styles sidebar end up stored in settings.typography.fontFamily.custom and make the UI ignore any other origin.

If we expected the theme.json model and the font library to work together, all origins should work, classic themes would have an API to add their fonts to the theme origin, and plugins would be able to hook into any for different needs. Instead, we want the font library to behave more like the media library.

@oandregal
Copy link
Member Author

I'm closing this as the next steps would be to develop the font library data flow and stop using theme.json data. This needs to be tracked in other places and this will no longer be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

2 participants