Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 19 months ago

#55985 closed enhancement (fixed)

[Twenty Twelve to Twenty Seventeen]: bundle Google Fonts in each theme

Reported by: luminuu's profile luminuu Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: high
Severity: major Version:
Component: Bundled Theme Keywords: has-patch add-to-field-guide has-dev-note
Focuses: privacy Cc:

Description

Several older default themes are still using Google Fonts directly loaded from the Google servers. Here's a list of all affected themes:

  • Twenty Twelve
  • Twenty Thirteen
  • Twenty Fourteen
  • Twenty Fifteen
  • Twenty Sixteen
  • Twenty Seventeen

This issue has been talked about in the past here in Trac: #42166, #43898 as well as in the Github repository of the Twenty Twenty theme, which is the most recent one: https://github.com/WordPress/twentytwenty/issues/4

Earlier this year, a German Court has fined a website owner for implementing fonts directly from Google Fonts: https://wptavern.com/german-court-fines-website-owner-for-violating-the-gdpr-by-using-google-hosted-fonts

While in the newer default themes fonts got added as an asset, the older default themes remained untouched. This can cause issues with users not being aware of both the legal stuff and the fact that Google Fonts are directly used in default themes. Today, in the German support forums a website owner, who created a website with Twenty Seventeen, shared that he has been threatened by another person making use of the German court rule from earlier this year. Link to the support topic, all posts are in German: https://de.wordpress.org/support/topic/google-fonts-werde-hier-abgemahnt-twentyseventeen-fonts/

We (@luehrsen, @luminuu) want to raise this issue and propose to have the Google Fonts used in older default themes to be served from the theme itself, not using the Google Fonts servers. We think while it is still widely among plugins and other themes to use Google Fonts directly, the default themes of WordPress should be able to used risk-free and compliant with the GDPR.

Change History (116)

This ticket was mentioned in Slack in #themereview by poena. View the logs.


2 years ago

#2 @luehrsen
2 years ago

I would suggest analysing how the current core themes are implementing fonts and use that as a template to provide patches for the old themes.

I would not simply comment out the google request, as that would visually impact a lot of live websites.

This ticket was mentioned in PR #2814 on WordPress/wordpress-develop by Luehrsen.


2 years ago
#3

  • Keywords has-patch added

This pull request aims to remove google webfont requests from core themes and serve needed fonts locally.

Trac ticket: https://core.trac.wordpress.org/ticket/55985#ticket

#4 @luehrsen
2 years ago

I have prepared a proof of concept for Twenty Seventeen. The idea is to follow the google implementation as closely as possible and to just replace the CSS file.

The question is: Would we want to do the replacements for all core themes in one big pull request, or do we want to create one pull request per core theme?

j-hoffmann commented on PR #2814:


2 years ago
#5

The new @font-face rules use local('') in the src: declarations.

On the github repository for the Google Webfonts Helper there is an open issue about problems arising from using such declarations.
There is another open issue about the same type of declaration where I commented having another type of problem myself. I know I should have opened a separate ticket for my issue, but I realized that the project didn't seem to be mantained anymore (last code change is 5 years old), so I disited.

To avoid these problems, I would suggest to either use the "smiley" hack as mentioned here or drop the local('') part altogether.

Luehrsen commented on PR #2814:


2 years ago
#6

To avoid these problems, I would suggest to either use the "smiley" hack as mentioned here or drop the local('') part altogether.

I think dropping local is the way to go.

This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.


2 years ago

#8 @SergeyBiryukov
2 years ago

  • Component changed from Themes to Bundled Theme

#9 follow-up: @jffng
2 years ago

Thanks for tackling this.

Would we want to do the replacements for all core themes in one big pull request, or do we want to create one pull request per core theme?

Personally I think one PR per core theme would make it easier for folks to pitch in, test, and verify the fonts are loading as expected. All the commits can reference this single ticket though for continuity.

#10 in reply to: ↑ 9 @paapst
2 years ago

Would we want to do the replacements for all core themes in one big pull request, or do we want to create one pull request per core theme?

For review and testing purposes I would suggest to go for one PR per core theme.

cbirdsong commented on PR #2814:


2 years ago
#11

I see this PR includes the font files in the repo, but installing them via NPM is also an option: https://fontsource.org/

Luehrsen commented on PR #2814:


2 years ago
#12

I see this PR includes the font files in the repo, but installing them via NPM is also an option: https://fontsource.org/

This is how I would do it today. But I think the overhead in terms of packages and build tools is a little too much for the scope of what we want to achieve here. I don't think introducing a new package and build process for a legacy polyfill is wise here.

We can always go back and optimise if we wish to do that.

This ticket was mentioned in PR #2823 on WordPress/wordpress-develop by Luehrsen.


2 years ago
#13

This pull request aims to remove google webfont requests from core themes and serve needed fonts locally.

Trac ticket: https://core.trac.wordpress.org/ticket/55985#ticket

#14 @webcommsat
2 years ago

Just adding a [link]https://wordpress.slack.com/archives/C02RQBWTW/p1655325272832719 in this ticket to the discussion in Dev Chat on June 15, 2022 so any items from it can be consolidated on the ticket itself. Thanks for raising it for Dev Chat and congratulations on your first ticket @luminuu

2ndkauboy commented on PR #2814:


2 years ago
#15

I think dropping local is the way to go.

Why not using the real local name local('Libre Franklin')? On my Manjaro laptop I have the Google Fonts package installed and load pretty much any Goole Web Font locally. Some operating systems have other local fonts installed by default.

j-hoffmann commented on PR #2814:


2 years ago
#16

Why not using the real local name local('Libre Franklin')? On my Manjaro laptop I have the Google Fonts package installed and load pretty much any Goole Web Font locally. Some operating systems have other local fonts installed by default.

Apparently there were cases with colliding font names. Originally Google Fonts used to add the local("Font Name") delarations but eventually removed them except for specific cases.

Another possible reason is that there is no control over which version of the font is loaded from the user's computer. The user could have a version with a limited character set or even incomplete variations installed.

Luehrsen commented on PR #2814:


2 years ago
#17

Based on a discussion with @2ndkauboy I've rewritten the implementation and added the unicode-range selector. (https://github.com/WordPress/wordpress-develop/pull/2823#discussion_r900925587)

The downside is, that the font.css is really complex now and we have a lot of file. But the upside is, that character subsets get automatically loaded when needed.

https://user-images.githubusercontent.com/1105871/174487580-8919f358-1bb4-49bd-a02e-2c3ac01e8e2d.mov

2ndkauboy commented on PR #2814:


2 years ago
#18

I've rewritten the implementation and added the unicode-range selector.

That seems like a great solution!

#19 follow-up: @MatthiasReinholz
2 years ago

Thank you @luminuu and @luehrsen for raising this!

I have a small remark regarding the font to be bundled in Twenty Twelve.

The WordPress Theme directory maintains a list of licenses compatible with GPL. The docs say that any bundled font is required to be licensed under one of the licenses mentioned here: https://make.wordpress.org/themes/handbook/review/resources/#gpl-compatible-font-licenses

Not sure if this requirement is also important in this context?

I came across the potential issue of bundling the font Open Sans with Twenty Twelve.
According to Google Fonts, this font is licensed under the Apache License 2.0 which is not part of the allowed list of licenses. (see https://fonts.google.com/specimen/Open+Sans?query=open+sans#license)

I checked all the other bundled fonts in all other mentioned themes and they're ok as released under the SIL Open Font License.

#20 in reply to: ↑ 19 ; follow-up: @paapst
2 years ago

Replying to MatthiasReinholz:

I came across the potential issue of bundling the font Open Sans with Twenty Twelve.
According to Google Fonts, this font is licensed under the Apache License 2.0 which is not part of the allowed list of licenses. (see https://fonts.google.com/specimen/Open+Sans?query=open+sans#license)

I checked all the other bundled fonts in all other mentioned themes and they're ok as released under the SIL Open Font License.

Luckily that issue has already been solved. In 2020 the Open Sans font was also licensed under the SIL Open Font License. See: https://github.com/googlefonts/opensans/blob/main/OFL.txt

#21 in reply to: ↑ 20 @MatthiasReinholz
2 years ago

Replying to paapst:

Replying to MatthiasReinholz:

I came across the potential issue of bundling the font Open Sans with Twenty Twelve.
According to Google Fonts, this font is licensed under the Apache License 2.0 which is not part of the allowed list of licenses. (see https://fonts.google.com/specimen/Open+Sans?query=open+sans#license)

I checked all the other bundled fonts in all other mentioned themes and they're ok as released under the SIL Open Font License.

Luckily that issue has already been solved. In 2020 the Open Sans font was also licensed under the SIL Open Font License. See: https://github.com/googlefonts/opensans/blob/main/OFL.txt

Perfect, thank you!

#22 @luehrsen
2 years ago

For reference, I use this code to trigger a load of the different subsets:

<!-- wp:paragraph -->
<p>Latin Extended<br>Ą, Ć, Ę, Ł, Ń, Ó, Ś, Ź, Ż</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Vietnamese<br>Ă, Â, Đ, Ê, Ô, Ơ, Ư</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Cyrillic<br>№, Ѐ, Ж</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Cyrillic Ext<br>₴, ◌ⷠ</p>
<!-- /wp:paragraph -->

2ndkauboy commented on PR #2814:


2 years ago
#23

I believe locally hosted fonts are the best way forward, but this is really interesting:

https://twitter.com/dimensionmedia/status/1538912055381463041

Luehrsen commented on PR #2814:


2 years ago
#24

IMHO not without asking for consent first.

Am 21.06.2022 um 16:01 schrieb Bernhard Kau *@*.*>:


I believe locally hosted fonts are the best way forward, but this is really interesting:

https://twitter.com/dimensionmedia/status/1538912055381463041


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.

2ndkauboy commented on PR #2814:


2 years ago
#25

IMHO not without asking for consent first.

I think in terms of GDPR compliance it is OK without consent, as no personal data is sent to any non-EU entity.

But still I think for the default themes we should really go with local fonts.

#26 @sabernhardt
2 years ago

  • Milestone changed from Awaiting Review to 6.1
  • Version trunk deleted

This ticket was mentioned in PR #2856 on WordPress/wordpress-develop by Luehrsen.


2 years ago
#27

This pull request aims to remove google webfont requests from core themes and serve needed fonts locally.

Trac ticket: [](https://core.trac.wordpress.org/ticket/55985#ticket)

This ticket was mentioned in PR #2878 on WordPress/wordpress-develop by Luehrsen.


2 years ago
#28

This pull request aims to remove google webfont requests from core themes and serve needed fonts locally.

Trac ticket: https://core.trac.wordpress.org/ticket/55985#ticket

This ticket was mentioned in PR #2889 on WordPress/wordpress-develop by Luehrsen.


2 years ago
#29

This pull request aims to remove google webfont requests from core themes and serve needed fonts locally.

Trac ticket: https://core.trac.wordpress.org/ticket/55985#ticket

This ticket was mentioned in Slack in #themereview by kafleg. View the logs.


2 years ago

This ticket was mentioned in PR #2920 on WordPress/wordpress-develop by Luehrsen.


2 years ago
#31

This pull request aims to remove google webfont requests from core themes and serve needed fonts locally.

Trac ticket: https://core.trac.wordpress.org/ticket/55985#ticket

#32 follow-up: @luehrsen
2 years ago

We're getting noticed, that users in Germany are still getting these 100€ E-Mails for using Google Fonts in themes.

I would therefore love to have some more eyes on this ticket as this directly affects the core WordPress audience in Germany.

#33 @sabernhardt
2 years ago

  • Priority changed from normal to high

This ticket was mentioned in Slack in #core-privacy by paapst. View the logs.


2 years ago

#35 in reply to: ↑ 32 ; follow-up: @paapst
2 years ago

Replying to luehrsen:

We're getting noticed, that users in Germany are still getting these 100€ E-Mails for using Google Fonts in themes.

I would therefore love to have some more eyes on this ticket as this directly affects the core WordPress audience in Germany.

At this moment this is a 6.1 ticket. I think that means that until at least the 25th of October nothing will change. Beta 1 testing will start at the end of September. Until then, German users can change their theme or use one of the other solutions mentioned here: https://complianz.io/self-hosting-google-fonts-for-wordpress/

#36 in reply to: ↑ 35 @hellofromTonya
2 years ago

Replying to paapst:

Replying to luehrsen:

We're getting noticed, that users in Germany are still getting these 100€ E-Mails for using Google Fonts in themes.

I would therefore love to have some more eyes on this ticket as this directly affects the core WordPress audience in Germany.

At this moment this is a 6.1 ticket. I think that means that until at least the 25th of October nothing will change. Beta 1 testing will start at the end of September. Until then, German users can change their theme or use one of the other solutions mentioned here: https://complianz.io/self-hosting-google-fonts-for-wordpress/

A suggestion: explore releasing the updated version of each theme separately from WordPress 6.1 release.

When each theme is ready, release it to wp.org's theme repo. Users can then update to get locally hosted fonts ahead of when WP 6.1 is released.

#37 @sabernhardt
2 years ago

No commits have been made to these themes since the 6.0 version bumps, so creating new theme versions for this specific change could be good when they are ready. Using locally hosted fonts is already recommended, but we need to fix our own themes before we can make this a requirement for others.

Reviewing the patches, I found a few problems plus potential improvements:

  1. URL references need to use get_template_directory_uri() instead of get_stylesheet_directory_uri() to work properly with child themes.
  2. When the theme adds the font stylesheet within the add_editor_style() array, the block editor prints stylesheet contents into an inline script. This makes the editor look for the relative-URL font files within the wp-admin directory (404 errors) before correctly finding them in the theme directory from the enqueued stylesheet. We may need to create a function like twentytwelve_mce_css() in the other five themes.
  3. As noted earlier, the empty local('') proposed for Twenty Seventeen has a reported problem, yet I think local('Libre Franklin') and local('Open Sans') could be worth including (with any name variations). I tried it with the Open Sans stylesheet in Twenty Twelve.
  4. Creating an additional function for the fonts URL seems unnecessarily complex. See the Twenty Thirteen example for a simpler possibility with multiple fonts.
  5. Several license files mention the TrueType format (TTF), though these stylesheets have WOFF2 and WOFF.
  6. The font URLs could use the theme version's date instead of null when enqueued.
  7. Font files might not be updated frequently in these themes, but could adding a query string like ?ver=21 be better than -v21- in the filename?
  8. Would removing the resource hint filter from each theme and deprecating the related functions be better than changing the functions?
  9. I would like to consider adding a function_exists check, similar to the ones wrapped around twentyfifteen_fonts_url and twentysixteen_fonts_url, in the other themes.

#38 @luehrsen
2 years ago

Just to quickly answer on some of the points raised by @sabernhardt:

  1. The question is if we want to force the network resource to load. From personal experience I can say that there are so many different versions of fonts available that it can lead to slight differences. So it's a bit about priorities: Do we want to consider local files, even if they could be from a different font version? Or do we want to ensure consistency and always load our defined resources.
  1. Yes, on the surface this might be a simpler solution, but with the tradeoff of massive duplication of stylesheets. For consistency (also across the themes), readability and based on past implementations I would approach this with a strict one font per css file rule. The new functions help that, as we keep the old functions consistent in terms of return types, if child themes rely on them.
  1. As the Google Fonts repository was one of the main sources for the font files, it is also one of the main sources for the attribution for the licenses. See https://fonts.google.com/attribution
  1. I opted to not change the filename of the font for better maintainability and consistency across projects.
  1. I personally would not deprecate the function, as there might be child themes dependent on the existence of the function. It is certainly cleaner, but with some of them being 10 years in the field I can't imagine what will break inadvertently.

#39 @sabernhardt
2 years ago

Perhaps we should not start to specify locally installed fonts for these themes (using local()). We have received a few complaints about the font weight mismatch of "Source Serif Pro" in Twenty Twenty-Two (#56332).

By 'deprecating' I meant leaving the resource hint function itself unchanged, probably in the same place, but removing the call to the function and adding a 'deprecated' note to the docblock.

Also, I tried adding the font stylesheet to Classic Editor with an array from twentyfourteen_fonts_urls(), so the stylesheet could be removed from add_editor_style().

Last edited 2 years ago by sabernhardt (previous) (diff)

This ticket was mentioned in Slack in #core-privacy by paapst. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-privacy by brendanmcn. View the logs.


2 years ago

#42 @sabernhardt
2 years ago

#46170 was marked as a duplicate.

#43 @sabernhardt
2 years ago

  • Summary changed from [Older Twenty-* Themes] Replace directly loading fonts from Google Fonts with including them in each theme to [Twenty Twelve to Twenty Seventeen]: bundle Google Fonts in each theme

Props from #46170: @garrett-eclipse, @kjellr, @ocean90, @SergeyBiryukov, @westonruter

#44 @sabernhardt
2 years ago

The block editor had trouble with add_editor_style() because full URLs do not get the base directory in the inline script. It required a relative reference. Then removing the parent/child theme directory should make creating new mce_css functions unnecessary.

Maintaining 3 stylesheets for Twenty Thirteen—and 7 stylesheets each for Twenty Fifteen and Twenty Sixteen—is a lot of duplication, but I think combining them is important. If anyone else dequeues the font stylesheet in their current child theme (or a plugin), the theme would need to keep a single stylesheet with the same ID to continue dequeuing the font(s).

I started creating a collection of child themes to test each theme in different scenarios:

  • no font change
  • using a custom Google stylesheet
  • supplying a new self-hosted font stylesheet in the child theme
  • dequeuing fonts
  • replacing the function output with an empty string when the theme checks if function_exists (Twenty Fifteen and Twenty Sixteen)

#45 @luehrsen
2 years ago

Hey @sabernhardt, thank you for taking care of this issue!

I think I understand the problem (or one of them). Let me try to rephrase in my words:

Whenever a child theme dequeues one of the main font stylesheets (e.g. twentyfifteen-fonts), with my current implementation we ignore that and inadvertently dump new font stylesheets into the theme.

That is obviously an issue.

What would you think about declaring the 'new' stylesheets as dependencies of the main stylesheet? So of the main stylesheet is dequeued, the subsets will not get loaded, all while avoiding duplication.

So something like:


$dependencies = array();
foreach ( twentyfifteen_fonts_urls() as $font_slug => $font_url ) {
        wp_register_style( 'twentyfifteen-font-' . $font_slug, $font_url, array(), null );
        $dependencies[] = 'twentyfifteen-font-' . $font_slug;
}

// Register the style 'twentyfifteen-fonts' for backwards compatibility.
wp_register_style( 'twentyfifteen-fonts', false, $dependencies );
wp_enqueue_style( 'twentyfifteen-fonts' );

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#48 @sabernhardt
2 years ago

  • Type changed from enhancement to task (blessed)

#49 @JeffPaul
2 years ago

@sabernhardt @luehrsen it appears from the conversation you're having that perhaps there's a refresh needed here. I'd love to see this get into a sooner 6.1 Beta release, so please share if there's some additional help needed or if it's just needing time to get things moved along and ready for commit?

#50 @luehrsen
2 years ago

All PRs use get_template_directory_uri now to avoid child theme conflicts.

@JeffPaul Absolutely. But as this is a massive change and my time is limited I would love to have some guidance on the implementation, especially on #comment:45

Last edited 2 years ago by luehrsen (previous) (diff)

#51 @davidbaumwald
2 years ago

  • Milestone changed from 6.1 to 6.2
  • Type changed from task (blessed) to enhancement

After chatting with @JeffPaul, 6.1 Beta 2 releasing today, and this ticket still needing a final direction and patch, this ticket has missed the window for 6.1.

Given the current momentum, I'll move this to 6.2, hoping it lands earlier in the cycle. Also reverting this back to an Enhancement since it's not an officially blessed Task for 6.2 as yet.

#52 @luehrsen
2 years ago

While I understand the issue, this is nontheless sad to see. This is stilly a serious issue in Germany (and other GDPR territories), as users with active Google Fonts are currently getting targeted by people exploiting the law.

https://www-heise-de.translate.goog/news/DSGVO-Abmahnwelle-wegen-Google-Fonts-7206364.html?_x_tr_sl=de&_x_tr_tl=en&_x_tr_hl=de&_x_tr_pto=wapp

If there is anything I can do, I would love to invest more time to land this ASAP.

#53 @JeffPaul
2 years ago

@ianbelanger any guidance on how @luehrsen should proceed on this (see https://core.trac.wordpress.org/ticket/55985#comment:45)?

This ticket was mentioned in Slack in #core by marybaum. View the logs.


2 years ago

#55 @desrosj
2 years ago

I mentioned this in Slack, but wanted to also detail it here in this ticket.

While updates for bundled themes are usually pushed out in coordination with major and minor releases of WordPress, this is not the only time we can push updates to these themes.

The reason the updates are usually coordinated is that the themes are usually updated to be compatible with new versions of WordPress, so releasing at the same time makes a lot of sense. Also, the number of contributors that focus on the tickets within the Bundled Themes component is usually very low unless these compatibility issues are being addressed.

I’m currently the only component maintainer (and I wouldn’t necessarily consider myself a theming person) because the component is a bit unique in that it needs at least one maintainer to ensure the themes are released and managed appropriately. This is not meant to slight any effort that has taken place here. I'm just trying to detail why it's often difficult for default theme tickets to move forward. This is especially true of the older ones. Insert sales pitch to become a bundled themes component maintainer here. New maintainers are always welcome.

Recently, we have released bundled theme updates separately. Twenty Twenty-One and Twenty Nineteen were updated independently of Core in 2020, and Twenty Ten, Eleven, & Twelve were released mid cycle in 2021 (see #53777). But this comes down to contributor willingness, availability, and ultimately need. In these two cases, there was either a number of bugs or a few more severe bugs making a release necessary.

As long as the new versions of the themes do not depend on unmerged/unreleased changes in WordPress Core, these themes can be updated between 6.1 and 6.2. We can also look at coordinating with the 6.1.x minor releases too, if that seems more appropriate.

I know it is disappointing that these changes will not be ready for 6.1, but it does not necessarily mean another 3-5 month gap until this can be released.

#56 @sabernhardt
2 years ago

#41071 was marked as a duplicate.

#57 follow-up: @bedas
2 years ago

Perhaps the Google FAQ Should be considered before A) condemning someone to pay fines as a court and B) investing work in something that, if I understand the FAQ correctly, is not even happening:
https://developers.google.com/fonts/faq#what_does_using_the_google_fonts_api_mean_for_the_privacy_of_my_users

To my understanding, Google does _not_ log any IP address when using their fonts. Thus on what this German court even based their decision? Hearsay? Where is the proof this data was actually logged by google? I find it weird that the court would fine the small webmaster for 100 Euro when they could go after google and fine them for probably hundreds of millions, if this would be really happening. And probably, as per Google's own statements, it is not happening.

What do I miss here?

PS: Of course it is great to self store the fonts - always a good idea. But it seems to me that the reason is not a real concern?

#58 in reply to: ↑ 57 ; follow-up: @paapst
2 years ago

Replying to bedas:

What do I miss here?

Google changed their FAQ a few months ago. It used to state that they stored the IP addresses.

Also last part of the court ruling (translated by deepl from german to english)
"It must also be taken into account that the IP address was indisputably transmitted to a Google server in the USA, whereby an adequate level of data protection is not guaranteed there (see ECJ, Judgment of 16.7.2020 - C-311/18 (Facebook Ireland and Schrems), NJW 2020, 2613)."

#59 @poena
22 months ago

I think that declaring the 'new' stylesheets as dependencies of the main stylesheet as suggested in comment:45 would be a good next step.

#60 in reply to: ↑ 58 ; follow-up: @luminuu
22 months ago

@luehrsen Do you want to give this a try for one of the PRs and implement what you suggested in comment:45? Just to get this discussion going again, and maybe bring attention to more people on this.

cc @sabernhardt @desrosj

#61 in reply to: ↑ 60 @luehrsen
21 months ago

Replying to luminuu:

@luehrsen Do you want to give this a try for one of the PRs and implement what you suggested in comment:45? Just to get this discussion going again, and maybe bring attention to more people on this.

cc @sabernhardt @desrosj

I have started implementing this for twentytwelve, see https://github.com/WordPress/wordpress-develop/pull/2920

This ticket was mentioned in PR #3826 on WordPress/wordpress-develop by @luehrsen.


21 months ago
#62

As discussed here, loading the local font file can lean to unexpected compatibility issues due to version or locale mismatches of the font.

Vendors like Google have dropped using local in their font delivery.

That is why I would propose dropping the local rule altogether.

Trac ticket: https://core.trac.wordpress.org/ticket/57430#ticket

This ticket was mentioned in Slack in #core-editor by luehrsen. View the logs.


21 months ago

#64 @costdev
21 months ago

  • Keywords dev-feedback added

Hi all, PR 3826 looks good to me. What else is needed to move this ticket forward?

@desrosj based on comment:55, noting the priority/severity of this ticket, and with 6.2 Beta 1 approaching on February 7th, would you like me to propose this ticket to the release squad be a Task (blessed) for the 6.2 cycle?

#65 @sabernhardt
21 months ago

@costdev PR 3826 is for a separate ticket, #57430. If the Fonts API is not ready soon enough, that PR might be a temporary fix.

#66 @costdev
21 months ago

Ah, thanks for clarifying @sabernhardt!

#67 @poena
21 months ago

PR 2920 tests well for me. The correct font files and font-open-sans.css are loading in the block editor and the front. Tested in multiple languages.
I left two comments (nits) on the PR, about the docs.

Last edited 21 months ago by poena (previous) (diff)

This ticket was mentioned in Slack in #core by luminuu. View the logs.


21 months ago

#69 @davidbaumwald
21 months ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #core-test by luminuu. View the logs.


21 months ago

@azaozz commented on PR #2920:


21 months ago
#71

Perhaps this has been asked and answered somewhere else, but is it possible to keep the "font subsets" to optimize loading of font files similarly to how they are loaded from the Google's CDN?

Looking at the code, this part that is now removed:

		if ( 'cyrillic' === $subset ) {
			$subsets .= ',cyrillic,cyrillic-ext';
		} elseif ( 'greek' === $subset ) {
			$subsets .= ',greek,greek-ext';
		} elseif ( 'vietnamese' === $subset ) {
			$subsets .= ',vietnamese';

Seems that loading /open-sans/font-open-sans.css will always load all Open Sans font files, Cyrillic, Greek, Herbew, Latin, Vietnamese, etc. which won't be needed on all sites. Ideally the CSS should be split in few parts/subsets and only the needed font files will be loaded by the browsers.

@sabernhardt commented on PR #2920:


21 months ago
#72

@azaozz The Google font stylesheet already includes all subsets even if the URL specifies only latin and latin-ext.
https://fonts.googleapis.com/css?family=Open+Sans%3A400italic%2C700italic%2C400%2C700&subset=latin%2Clatin-ext&display=fallback

@azaozz commented on PR #2920:


21 months ago
#73

The Google font stylesheet already includes all subsets even if...

Yes, what I am asking is if it is possible to include only the needed subset(s). There is no point in the browser downloading all 64 font files on every front-end page load. This is a considerable slow down and a large amount of unneeded files, wasted bandwidth, etc.

Looking at https://fontsource.org/fonts/open-sans and the repo at https://github.com/fontsource/fontsource/tree/main/fonts/google/open-sans, there are ready-made CSS files that can probably be adjusted and used. For example: https://github.com/fontsource/fontsource/blob/main/fonts/google/open-sans/400.css (the .woff font files for the whole Open Sans can be downloaded from the first link).

@luehrsen commented on PR #2920:


21 months ago
#74

There is no point in the browser downloading all 64 font files on every front-end page load.

Very rarely will the user agent download all fonts. That is why the unicode-range tag describes the characters defined in each file, so that the browser/user-agent can decide weather that font file is needed or not. Same with the weight descriptor.

See: https://w3c.github.io/csswg-drafts/css-fonts/#font-prop-desc

For a font family defined with several @font-face rules, user agents can either download all faces in the family or use these descriptors to selectively download font faces that match actual styles used in document.

Also: https://caniuse.com/font-unicode-range

#75 @sabernhardt
20 months ago

Testing PR 2920 with wp_dequeue_style in some child themes, the front end and block editor properly removed the Open Sans stylesheet from the queue. (The dequeue function itself did not affect Classic Editor.) Edge case: I managed to break the Classic Editor by trying to remove the font with the mce_css filter, though my code already could fail with the current theme if the language turns off the font.

I do not consider that a blocker against using the dependencies method; however, I am still convinced that enqueuing only one stylesheet with the existing handle should be faster with multiple fonts and have lower potential for breaking sites.

Internet Explorer is one old browser that does not select font files based on characters in the unicode-range. Fontsource gives WOFF files with all character sets in each variant's file to reduce the number of files. I will create six pull requests today with these fonts and updated WOFF2 files, plus other changes.

This ticket was mentioned in PR #3992 on WordPress/wordpress-develop by @sabernhardt.


20 months ago
#76

Maintains existing font stylesheet handle in Twenty Twelve.
For Trac ticket 55985

This ticket was mentioned in PR #3993 on WordPress/wordpress-develop by @sabernhardt.


20 months ago
#77

Maintains existing font stylesheet handles in Twenty Thirteen, enqueuing only one font stylesheet.
For Trac ticket 55985

This ticket was mentioned in PR #3994 on WordPress/wordpress-develop by @sabernhardt.


20 months ago
#78

Maintains existing font stylesheet handle in Twenty Fourteen.
For Trac ticket 55985

This ticket was mentioned in PR #3995 on WordPress/wordpress-develop by @sabernhardt.


20 months ago
#79

Maintains existing font stylesheet handles in Twenty Fifteen, enqueuing only one font stylesheet.
For Trac ticket 55985

This ticket was mentioned in PR #3996 on WordPress/wordpress-develop by @sabernhardt.


20 months ago
#80

Maintains existing font stylesheet handles in Twenty Sixteen, enqueuing only one font stylesheet.
For Trac ticket 55985

This ticket was mentioned in PR #3997 on WordPress/wordpress-develop by @sabernhardt.


20 months ago
#81

Maintains existing font stylesheet handle in Twenty Seventeen.
For Trac ticket 55985

#82 @sabernhardt
20 months ago

Notes for pull requests 3992 to 3997:

  1. To avoid errors in the block editor with the more recent five themes, the theme directory is removed from the function output in add_editor_style. Making the stylesheet relative gives a baseURL for the relative font URLs in the stylesheet so the editor does not try loading them from the wp-admin directory. If anyone has replaced the theme setup function from Twenty Fourteen, Twenty Fifteen or Twenty Sixteen, that child theme (likely) would need updating.
  2. License information at the top of LICENSE.txt files is similar to what is in Twenty Twenty-Two. The README files also include copyright and licensing information, consistent with other themes.
  3. These font filenames would not change with any future updates, though the PRs include version numbers for the font files and the stylesheets. If instead we prefer to leave the stylesheet version as null and remove the query strings, visitors probably would not mind reusing the cached version of either the stylesheet or font files (if/when we update them). Without the query strings, updating fonts could be a little simpler, too.
  4. The resource hint function still can be used with a child theme or plugin, but site owners would need to opt-in to adding it for their custom Google stylesheet.
  5. I have appreciated the ability to override the font function output in Twenty Fifteen and Twenty Sixteen, so I added a function_exists check to the other four themes. This way, we can recommend that for custom URLs in any of these themes instead of dequeue functions or something else.
Last edited 20 months ago by sabernhardt (previous) (diff)

#83 @poena
20 months ago

I have tested each of the pull requests with the theme test data which has pages with English and Greek content.

  • The fonts are loading correctly without any apparent visual changes.
  • The correct subsets are loading.
  • Code review, to the best of my ability: no issues.

This ticket was mentioned in Slack in #core-themes by poena. View the logs.


20 months ago

#85 @poena
20 months ago

  • Keywords commit added

#86 @sabernhardt
20 months ago

When I applied the .diff files from the PRs with TortoiseSVN, the font files were not added. These could need edits to their SVN properties, which may be easier in a follow-up commit.

In case someone wants to test languages that turn off one or more fonts, I made a spreadsheet of font settings (yes, using Google Sheets).

Also, @jorbin asked about the WordPress package size increase. The beta and RC releases nightly builds would be more than 6 MB larger with these new font files and stylesheets.

Theme Fonts Fonts Directory Size
Twenty Twelve Open Sans (v34)
400italic, 700italic, 400, 700
635 KB
Twenty Thirteen Source Sans Pro (v21)
300, 400, 700, 300italic, 400italic, 700italic;
Bitter (v32)
400, 700
914 KB
Twenty Fourteen Lato (v23)
300, 400, 700, 900, 300italic, 400italic, 700italic
429 KB
Twenty Fifteen Noto Sans (v27)
400italic, 700italic, 400, 700;
Noto Serif (v21)
400italic, 700italic, 400, 700;
Inconsolata (v31)
400, 700
2.85 MB
Twenty Sixteen Merriweather (v30)
400, 700, 900, 400italic, 700italic, 900italic;
Montserrat (v25)
400, 700;
Inconsolata (v31)
400
1.22 MB
Twenty Seventeen Libre Franklin (v13)
300, 300i, 400, 400i, 600, 600i, 800, 800i
489 KB
Last edited 20 months ago by sabernhardt (previous) (diff)

#87 follow-up: @aristath
20 months ago

The patches/PRs attached above seem to be working fine on my end.
I do have a couple of notes however:
Perhaps on older themes we could remove the fonts altogether? Themes before 2019 were trying to accommodate older browsers and systems where the default system fonts were lacking (to put it nicely). This however is no longer the case, so maybe we can simply remove webfonts for these old themes now that system fonts are decent on all systems that WP supports?

#88 in reply to: ↑ 87 @luehrsen
20 months ago

Replying to aristath:

Perhaps on older themes we could remove the fonts altogether? Themes before 2019 were trying to accommodate older browsers and systems where the default system fonts were lacking (to put it nicely). This however is no longer the case, so maybe we can simply remove webfonts for these old themes now that system fonts are decent on all systems that WP supports?

While it is true that modern systems support a lot more fonts, the fonts selected in the themes are not installed locally by default on most (any?) systems, so removing the font files would result in a change of visuals for these themes and all child themes.

One of the base approaches of this issue is to avoid exactly that.

#89 @audrasjb
20 months ago

  • Keywords needs-testing removed

I agree with @luehrsen, it doesn't sound realistic to me to introduce so much visual changes to existing websites.

#90 @audrasjb
20 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Self assigning for review/commit.

#91 @audrasjb
20 months ago

Let's commit these changesets one by one.

I tested the changes on Twenty Seventeen: PR2814 is good to go on my side.

#92 @audrasjb
20 months ago

In 55266:

Twenty Seventeen: Bundle Google Fonts locally.

This changeset bundles the Google Fonts used by Twenty Seventeen locally in the theme folder, instead of loading them from Google servers. Existing font stylesheet handles are maintained for backward compatibilily.

Props luminuu, audrasjb, jhoffmann, jffng, paapst, cbirdsong, webcommsat, kau-boy, MatthiasReinholz, sabernhardt, hellofromTonya, JeffPaul, davidbaumwald, desrosj, bedas, poena, costdev, azaozz, aristath.
See #55985.

#94 @audrasjb
20 months ago

Twenty Sixteen changeset looks good to go, too.

#95 @audrasjb
20 months ago

In 55267:

Twenty Sixteen: Bundle Google Fonts locally.

This changeset bundles the Google Fonts used by Twenty Sixteen locally in the theme folder, instead of loading them from Google servers. Existing font stylesheet handles are maintained for backward compatibilily.

Props garrett-eclipse, kjellr, ocean90, SergeyBiryukov, westonruter, luminuu, audrasjb, jhoffmann, jffng, paapst, cbirdsong, webcommsat, kau-boy, MatthiasReinholz, sabernhardt, hellofromTonya, JeffPaul, davidbaumwald, desrosj, bedas, poena, costdev, mukesh27, azaozz, aristath.
See #55985.

#97 @audrasjb
20 months ago

In 55268:

Twenty Fifteen: Bundle Google Fonts locally.

This changeset bundles the Google Fonts used by Twenty Fifteen locally in the theme folder, instead of loading them from Google servers. Existing font stylesheet handles are maintained for backward compatibilily.

Props garrett-eclipse, kjellr, ocean90, SergeyBiryukov, westonruter, luminuu, audrasjb, jhoffmann, jffng, paapst, cbirdsong, webcommsat, kau-boy, MatthiasReinholz, sabernhardt, hellofromTonya, JeffPaul, davidbaumwald, desrosj, bedas, poena, costdev, mukesh27, azaozz, aristath.
See #55985.

#99 @audrasjb
20 months ago

In 55270:

Twenty Fourteen: Bundle Google Fonts locally.

This changeset bundles the Google Fonts used by Twenty Fourteen locally in the theme folder, instead of loading them from Google servers. Existing font stylesheet handles are maintained for backward compatibilily.

Props garrett-eclipse, kjellr, ocean90, SergeyBiryukov, westonruter, luminuu, audrasjb, jhoffmann, jffng, paapst, cbirdsong, webcommsat, kau-boy, MatthiasReinholz, sabernhardt, hellofromTonya, JeffPaul, davidbaumwald, desrosj, bedas, poena, costdev, mukesh27, azaozz, aristath.
See #55985.

#101 @audrasjb
20 months ago

In 55274:

Twenty Thirteen: Bundle Google Fonts locally.

This changeset bundles the Google Fonts used by Twenty Thirteen locally in the theme folder, instead of loading them from Google servers. Existing font stylesheet handles are maintained for backward compatibilily.

Props garrett-eclipse, kjellr, ocean90, SergeyBiryukov, westonruter, luminuu, audrasjb, jhoffmann, jffng, paapst, cbirdsong, webcommsat, kau-boy, MatthiasReinholz, sabernhardt, hellofromTonya, JeffPaul, davidbaumwald, desrosj, bedas, poena, costdev, mukesh27, azaozz, aristath.
See #55985.

#103 @audrasjb
20 months ago

In 55277:

Twenty Twelve: Bundle Google Fonts locally.

This changeset bundles the Google Fonts used by Twenty Twelve locally in the theme folder, instead of loading them from Google servers. Existing font stylesheet handles are maintained for backward compatibilily.

Props garrett-eclipse, kjellr, ocean90, SergeyBiryukov, westonruter, luminuu, audrasjb, jhoffmann, jffng, paapst, cbirdsong, webcommsat, kau-boy, MatthiasReinholz, sabernhardt, hellofromTonya, JeffPaul, davidbaumwald, desrosj, bedas, poena, costdev, mukesh27, azaozz, aristath.
See #55985.

This ticket was mentioned in Slack in #core by costdev. View the logs.


20 months ago

#105 @costdev
20 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

@hellofromTonya commented on PR #3826:


20 months ago
#107

Committed via https://core.trac.wordpress.org/changeset/55314. Thanks for your contributions!

#108 @milana_cap
20 months ago

  • Keywords add-to-field-guide added

#109 @sabernhardt
20 months ago

  • Keywords needs-dev-note added; dev-feedback commit removed

My SVN acknowledged a modified font file in trunk, following the commit, so that seems fine.

I've started writing a dev note for child theme and plugin authors/users (post draft/Google Docs draft).

Also, #57807 is a potential follow-up ticket to keep subset translations.

#110 @milana_cap
20 months ago

@sabernhardt thank you so much for writing the Dev note. I see it's not done yet but I reviewed what's in there so far and it's looking good. Let me know when it's completed. Thanks

#111 @audrasjb
20 months ago

The post looks good to me, but personaly, I would just rephrase a bit the post title to use something like "Google Fonts are now included locally in bundled themes" because I think "Google Fonts addresses" doesn't convey the main information :)

#112 @milana_cap
20 months ago

@sabernhardt, is the dev note ready for the final review? Do you want to convert the code block into syntaxhighlight one?

#113 @sabernhardt
20 months ago

I updated the dev note title to "Google Fonts are included locally in bundled themes" (removing "now" because the updates will not be available officially for another three weeks).

The post is not yet ready for final review. I intend to complete the custom font set section tomorrow (Central U.S.) so reviewers could check it all on Monday (their time).

I prefer the Code block for the HTML snippet in the first section because the markup wraps to the next line, and I probably would rather keep the same block for the PHP examples, too. The PHP is temporarily in both block types to compare the output and styling.

#114 @sabernhardt
19 months ago

I preferred the SyntaxHighlighter for CSS examples, so the PHP examples are consistent with them.

The post draft and Google Doc had a few discrepancies, but now the post content should be the same in both places. The two Classic Editor considerations do not include full examples of how to use the mce_css filter or setting a version only on the front end, which could distract from working with situations I expect are more common. The Google Sheet has those at the very end of the document, and I might add them in a comment—on this ticket.

Last edited 19 months ago by sabernhardt (previous) (diff)

#115 @sabernhardt
19 months ago

Custom font stylesheet: fixing potential Classic Editor issues (if you cannot avoid them)

  1. The child theme’s stylesheet and directory should not match a file in its parent theme, or else the editor would fetch both stylesheets. For example, you could name yours '/fonts/montserrat-all.css' to avoid adding Twenty Sixteen’s '/fonts/montserrat.css'. If you already have a stylesheet with the same filename and directory, you could remove the extra file in the editor with a filter:
    <?php
    function wpdocs_remove_parent_font_from_classic_editor( $mce_css ) {
    	$font_url = str_replace(
    		get_stylesheet_directory_uri(),
    		get_template_directory_uri(),
    		twentysixteen_fonts_url()
    	);
    	$mce_css  = str_replace(
    		',' . $font_url . ',',
    		',',
    		$mce_css
    	);
    
    	return $mce_css;
    }
    add_filter( 'mce_css', 'wpdocs_remove_parent_font_from_classic_editor', 11 );
    
  1. If a query string such as a version number is important, the child theme function could add the variable only on the front end so that the Classic Editor still adds the fonts (without the query string):
    <?php
    function twentysixteen_fonts_url() {
    	$fonts_url = get_stylesheet_directory_uri() . '/fonts.css';
    	if ( ! is_admin() ) {
    		$fonts_url = add_query_arg( 'ver', '1.1', $fonts_url );
    	}
    	return $fonts_url;
    }
    
Note: See TracTickets for help on using tickets.