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

Webfonts: scan content for font families and enqueue them (Attempt #2) #39593

Conversation

zaguiini
Copy link
Member

@zaguiini zaguiini commented Mar 18, 2022

What?

There are two areas of logic that we update in this PR that could cause performance concerns.

  1. When we parse all frontend rendered blocks ( link )
  2. When we parse the global styles object for data ( link )

To investigate the impact of our code changes, we conducted simple performance tests as described below.

Performance tests

We conducted the tests in the chrome browser. We emptied the web cache and reloaded the page 5 times. We then took the average of the 5 results to reach our final numbers. We're happy to provide times for each individual trial if folks are interested.

  • With block and global styles parsing logic: 859ms
  • Only block parsing logic: 843ms
  • Only global styles parsing logic: 834ms
  • With no parsing logic: 832ms

The performance impact appears to be acceptable for the changes we're making. Curious if other have thoughts though.

Why?

How?

Testing Instructions

Screenshots or screencast

@aristath aristath self-requested a review March 21, 2022 10:50
@jeyip jeyip changed the title Webfonts: scan content for font families and enqueue them Webfonts: scan content for font families and enqueue them (2nd attempt) Mar 21, 2022
@jeyip jeyip changed the title Webfonts: scan content for font families and enqueue them (2nd attempt) Webfonts: scan content for font families and enqueue them (Attempt #2) Mar 21, 2022
@zaguiini zaguiini force-pushed the try/register-and-enqueue-webfonts-by-family branch from feb1de1 to 99521d4 Compare March 23, 2022 19:22
@zaguiini zaguiini force-pushed the try/register-and-enqueue-webfonts-by-family branch 2 times, most recently from f7a2b9d to 9263fce Compare March 28, 2022 20:44
@zaguiini zaguiini force-pushed the try/scan-content-for-webfonts-by-family branch 2 times, most recently from cacc0de to d9b07eb Compare March 29, 2022 22:54
Copy link
Member Author

@zaguiini zaguiini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, running this yields the following notices:

Notice: enqueue_webfont was called incorrectly. The "body-font" font family is not registered. Please see Debugging in WordPress for more information.

Notice: enqueue_webfont was called incorrectly. The "monospace" font family is not registered. Please see Debugging in WordPress for more information.

Notice: enqueue_webfont was called incorrectly. The "heading-font" font family is not registered. Please see Debugging in WordPress for more information.

Notice: The "roboto" font family is already enqueued.

The first three are caused because fontFamily is defined in blocks or global styles, but they're not a custom webfont registered through API:

  • body-font and heading-font comes from the (I believe) legacy Google Fonts provider
  • monospace is literally the built-in monospaced font from the browser

While the last warning we can easily drop -- that information might not be useful at all since it's already enqueued, so why bother? --, I'm unsure about how to proceed with the others.

How could we differentiate between fonts we want to be registered through the Webfonts API and fonts that are not part of it?

Should we include an extra property in theme.json that shows that this font should be handled by the Webfonts API? This is not the greatest developer experience because you'll need to think about the Webfonts API and ideally it should just get out of your way...

Should we ignore all notices when scanning for fonts? This is potentially harmful because people might not know that the fonts they're trying to use are not registered, but to be able to pick from the editor, they need to register them beforehand, so maybe this problem doesn't even exist. However, once it's picked and registered, it's stored in the database. If their plugin or theme does not register the webfont again, it is definitely worth it showing the notice because the font is not there anymore.

I'm all open to suggestions here. cc @hellofromtonya @creativecoder @aristath

* We are already enqueueing all registered fonts by default when loading the block editor,
* so we only need to scan for webfonts when browsing as a guest.
*/
if ( ! is_admin() ) {
Copy link
Member Author

@zaguiini zaguiini Mar 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish we did not have to worry about this. Maybe we can make enqueue_webfont a noop when opening the page as admin?

But still, all the code that looks for webfonts in blocks/global styles would run, and that's what I'd like to avoid. Is there a better way? Are we okay with exposing this implementation detail? Are there better options?

@zaguiini zaguiini force-pushed the try/register-and-enqueue-webfonts-by-family branch from 10454fe to 4b65aee Compare March 29, 2022 23:16
@zaguiini zaguiini force-pushed the try/scan-content-for-webfonts-by-family branch from d9b07eb to 0f3b94d Compare March 29, 2022 23:18
@mtias
Copy link
Member

mtias commented Mar 29, 2022

I'm not sure I follow the implementation rationality here. Why would we attempt to parse blocks to determine if a font is being used? Not only it adds a slight performance penalty but on block themes we should rely on theme.json to specify which fonts from all available ones to enqueue, without any content parsing; on non-block themes we cannot reason exclusively through block usage because a font might be used directly in a template outside content blocks, so it's kind of mute there.

@zaguiini
Copy link
Member Author

Closing as we're following another direction. @mtias decided we should look for fonts in theme.json and enqueue them from there, with the block-scanning optimization coming later in the process.

@zaguiini zaguiini closed this Mar 31, 2022
@zaguiini zaguiini deleted the try/scan-content-for-webfonts-by-family branch March 31, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants