Skip to content

#55985 - Remove Google fonts from Twenty Twelve #2920

Open
Luehrsen wants to merge 15 commits intoWordPress:trunkfrom
Luehrsen:update/google-font-twenty-twelve
Open

#55985 - Remove Google fonts from Twenty Twelve #2920
Luehrsen wants to merge 15 commits intoWordPress:trunkfrom
Luehrsen:update/google-font-twenty-twelve

Conversation

@Luehrsen
Copy link
Copy Markdown

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 Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>

return $urls;
}
add_filter( 'wp_resource_hints', 'twentytwelve_resource_hints', 10, 2 );
Copy link
Copy Markdown

@sabernhardt sabernhardt Feb 1, 2023

Choose a reason for hiding this comment

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

The resource hint filter should not return an empty string for everyone run without making changes for anyone. We could simply remove the add_filter line and add a deprecated comment in the documentation (under since). Then anyone who still has a custom Google URL could add this pre-defined filter in a child theme or plugin.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've updated the PR to remove the whole filter function. I'm unsure about the deprecation note, can you maybe take a look at it, @sabernhardt?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

My point was to leave the twentytwelve_resource_hints function intact. The version number in that example PR is outdated, but I only added one line to the documentation and removed the one line that calls the function.

Child themes can still use Google URLs, especially if their font selection is different, and some of those sites' owners may want to continue employing that filter.

Copy link
Copy Markdown
Contributor

@carolinan carolinan Feb 2, 2023

Choose a reason for hiding this comment

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

Should we just comment out the line with add_filter?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Commenting out the line might be better. That would show what was removed (and could be restored) even without explaining it in the DocBlock.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've just adapted the changes from your PR to this PR. I hope I got it all.

@azaozz
Copy link
Copy Markdown
Contributor

azaozz commented Feb 1, 2023

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
Copy link
Copy Markdown

@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
Copy link
Copy Markdown
Contributor

azaozz commented Feb 1, 2023

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 seems like 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 .ttf and .woff font files for the whole Open Sans can be downloaded from the first link).

@Luehrsen
Copy link
Copy Markdown
Author

Luehrsen commented Feb 2, 2023

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

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