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

Add registered fonts to theme JSON AFTER the child and parent data has been merged. #47290

Merged
merged 2 commits into from
Jan 23, 2023

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Jan 19, 2023

What?

This fixes a problem noted here: Automattic/themes#6827

In short because fonts registered via Font Provider are appended to a CHILD theme's data any fonts provided by the PARENT are no longer provided.

This change appends the fonts to the theme data AFTER the parent and child data have been merged.

Testing Instructions

You will need to test with a rather specific theme scenario at the moment.

In an environment with Gutenberg plugin activated
Jetpack activated and the Google Font Provider activated
(wpcom simple sites fit this scenario)

Using this branch of Blockbase (which permist SOME but not ALL Jetpack fonts to be registered)
activate a Blockbase CHILD theme (such as Geologist).

Navigate to the Site Editor and observe the collection of fonts provided. Note that ALL of the fonts offered by Blockbase as well as ALL of the fonts provided by Jetpack are available.

This fixes the problem noted here where the original change (#6777) was rolled back.

Screenshots or screencast

image

image

@pbking pbking requested a review from spacedmonkey as a code owner January 19, 2023 16:24
@pbking pbking marked this pull request as draft January 19, 2023 16:24
@github-actions
Copy link

github-actions bot commented Jan 19, 2023

Flaky tests detected in 4e93664.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3965739031
📝 Reported issues:

@hellofromtonya hellofromtonya added [Type] Bug An existing feature does not function as intended [Feature] Webfonts labels Jan 19, 2023
@hellofromtonya hellofromtonya removed the request for review from spacedmonkey January 19, 2023 17:28
@hellofromtonya hellofromtonya added this to the Gutenberg 15.1 milestone Jan 19, 2023
@pbking pbking marked this pull request as ready for review January 23, 2023 14:40
@pbking pbking merged commit 39579ec into trunk Jan 23, 2023
@pbking pbking deleted the fix/append_registered_fonts_without_clobbering branch January 23, 2023 14:53
@mreishus
Copy link
Contributor

With this, I'm seeing sometimes all registered fonts are enqueued. For example, see the site: https://testsitemmrfont1.wordpress.com/ (using theme: twentytwentytwo). View source and see 24,000 lines of font css.

  • Jetpack registers a large number of fonts
  • static::$theme = gutenberg_add_registered_fonts_to_theme_json( static::$theme ); adds all registered fonts to the theme.
  • gutenberg_register_fonts_from_theme_json() enqueues all fonts on the theme (Link)

End result: All registered fonts are enqueued and there is no longer a distinction between registered fonts and registered+enqueued fonts.

@hellofromtonya
Copy link
Contributor

@mreishus thank you for reporting this. Can you please open a new issue please and reference this PR? Thank you

@mreishus
Copy link
Contributor

Added issue #48263. I first thought it was directly related to this PR, but now I'm not so sure. I think it's more related to the gutenberg_add_registered_fonts_to_theme_json() in particular which was added before this PR (which only moves ir around).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants