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

Bundle common font files for externalizing data: URLs #3866

Merged
merged 3 commits into from
Dec 4, 2019

Conversation

westonruter
Copy link
Member

Summary

I found that the Gutenberg demo post was causing excessive CSS errors in Twenty Twenty. Here's the CSS manifest:

The style[amp-custom] element is populated with:
   220 B: style[type=text/css]
    81 B: style
   583 B (74%): link#amp-default-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-default.css?ver=1.5.0-alpha][media=all]
  5052 B (29%): link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1574484085][media=all]
 43461 B (75%): link#twentytwenty-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentytwenty/style.css?ver=1.0][media=all]
   122 B: style
    39 B: style[media=print]
   119 B: div.wp-block-cover has-background-dim alignwide amp-wp-d28a8c9
    88 B: img.wp-smiley amp-wp-843f19c[src=https://s.w.org/images/core/emoji/12.0.0-1/72x72/1f44b.png][alt=👋][width=72][height=72]
    75 B: p.amp-wp-224b51a
Total included size: 49,840 bytes (65% of 76,234 total after tree shaking)
The following stylesheets are too large to be included in style[amp-custom]:
  2131 B (93%): style#twentytwenty-style-inline-css
  1223 B (97%): link#twentytwenty-print-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentytwenty/print.css?ver=1.0][media=print]
Total excluded size: 3,354 bytes (94% of 3,540 total after tree shaking)
Total combined size: 53,194 bytes (66% of 79,774 total after tree shaking)

It was forcing out the important style#twentytwenty-style-inline-css stylesheet.

A big reason for this is the data: URL for NonBreakingSpaceOverride, which includes this 3KB of CSS in the resulting amp-custom stylesheet:

@font-face {
	font-family: NonBreakingSpaceOverride;
	src: 
		url("data:application/font-woff2;charset=utf-8;base64,d09GMgABAAAAAAMoAA0AAAAACDQAAALTAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP0ZGVE0cGh4GYACCahEICjx3CywAATYCJANUBCAFhiEHgWwbXQfILgpsY+rQRRARwyAs6uL7pxzYhxEE+32b3aeHmifR6tklkS9hiZA0ewkqGRJE+H7/+6378ASViK/PGeavqJyOzsceKi1s3BCiQsiOdn1r/RBgIJYEgCUhbm/8/8/h4saPssnTNkkiWUBrTRtjmQSajw3Ui3pZ3LYDPD+XG2C3JA/yKAS8/rU5eNfuGqRf4eNNgV4YAlIIgxglEkWe6FYpq10+wi3g+/nUgvgPFczNrz/RsTgVm/zfbPuHZlsuQECxuyqBcQwKFBjFgKO8AqP4bAN9tFJtnM9xPcbNjeXS/x1wY/xU52f5W/X1+9cnH4YwKIaoRRAkUkj/YlAAeF/624foiIDBgBmgQBeGAyhBljUPZUm/l2dTvmpqcBDUOHdbPZWd8JsBAsGr4w8/EDn82/bUPx4eh0YNrQTBuHO2FjQEAGBwK0DeI37DpQVqdERS4gZBhpeUhWCfLFz7J99aEBgsJCHvUGAdAPp4IADDCAPCEFMGpMZ9AQpTfQtQGhLbGVBZFV8BaqNyP68oTZgHNj3M8kBPfXTTC9t90UuzYhy9ciH0grVlOcqyCytisvbsERsEYztiznR0WCrmTksJwbSNK6fd1Rvr25I9oLvctUoEbNOmXJbqgYgPXEHJ82IUsrCnpkxh23F1rfZ2zcRnJYoXtauB3VTFkFXQg3uoZYD5qE0kdjDtoDoF1h2bulGmev5HbYhbrjtohQSRI4aNOkffIcT+d3v6atpaYh3JvPoQsztCcqvaBkppDSPcQ3bw3KaCBo1f5CJWTZEgW3LjLofYg51MaVezrx8xZitYbQ9KYeoRaqQdVLwSEfrKXLK1otCWOKNdR/YwYAfon5Yk8O2MJfSD10dPGA5PIJJQMkah0ugMJiv6x4Dm7LEa8xnrRGGGLAg4sAlbsA07sAt76DOsXKO3hIjtIlpnnFrt1qW4kh6NhS83P/6HB/fl1SMAAA==") format("woff2"), 
		url("data:application/font-woff;charset=utf-8;base64,d09GRgABAAAAAAUQAA0AAAAACDQAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAABGRlRNAAAE9AAAABwAAAAchf5yU0dERUYAAATYAAAAHAAAAB4AJwAbT1MvMgAAAaAAAABJAAAAYJAcgU5jbWFwAAACIAAAAF4AAAFqUUxBZ2dhc3AAAATQAAAACAAAAAgAAAAQZ2x5ZgAAApAAAAAyAAAAPL0n8y9oZWFkAAABMAAAADAAAAA2Fi93Z2hoZWEAAAFgAAAAHQAAACQOSgWaaG10eAAAAewAAAAzAAAAVC7TAQBsb2NhAAACgAAAABAAAAAsAOQBAm1heHAAAAGAAAAAHQAAACAAWQALbmFtZQAAAsQAAAF6AAADIYvD/Adwb3N0AAAEQAAAAI4AAADsapk2o3jaY2BkYGAA4ov5mwzj+W2+MnCzXwCKMNzgCBSB0LfbQDQ7AxuI4mBgAlEAFKQIRHjaY2BkYGD3+NvCwMDBAALsDAyMDKhAFAA3+wH3AAAAeNpjYGRgYBBl4GBgYgABEMnIABJzAPMZAAVmAGUAAAB42mNgZlJhnMDAysDCKsKygYGBYRqEZtrDYMT4D8gHSmEHjgUFOQwODAqqf9g9/rYwMLB7MNUAhRlBcsxBrMlASoGBEQAj8QtyAAAAeNrjYGBkAAGmWQwMjO8gmBnIZ2NA0ExAzNjAAFYJVn0ASBsD6VAIDZb7AtELAgANIgb9AHjaY2BgYGaAYBkGRgYQSAHyGMF8FgYPIM3HwMHAxMDGoMCwQIFLQV8hXvXP//9AcRCfAcb///h/ygPW+w/vb7olBjUHCTCyMcAFGZmABBO6AogThgZgIUsXAEDcEzcAAHjaY2BgECMCyoEgACZaAed42mNgYmRgYGBnYGNgYAZSDJqMgorCgoqCjECRXwwNrCAKSP5mAAFGBiRgyAAAi/YFBQAAeNqtkc1OwkAUhU/5M25cEhcsZick0AwlBJq6MWwgJkAgYV/KAA2lJeUn+hY+gktXvpKv4dLTMqKycGHsTZNv7px7z50ZAFd4hYHjdw1Ls4EiHjVncIFnzVnc4F1zDkWjrzmPW+NNcwGlzIRKI3fJlUyrEjZQxb3mDH2fNGfRx4vmHKqG0JzHg6E0F9DOlFBGBxUI1GEzLNT4S0aLuTtsGAEUuYcQHkyg3KmIum1bNUvKlrjbbAIleqHHnS4iSudpQcySMYtdFiXlAxzSbAwfMxK6kZoHKhbjjespMTioOPZnzI+4ucCeTVyKMVKLfeAS6vSWaTinuZwzyy/Dc7vaed+6KaV0kukdPUk6yOcctZPvvxxqksq2lEW8RvHjMEO2FCl/zy6p3NEm0R9OFSafJdldc4QVeyaaObMBO0/5cCaa6d9Ggyubxire+lEojscdjoWUR1xGOy8KD8mG2ZLO2l2paDc3A39qmU2z2W5YNv5+u79e6QfGJY/hAAB42m3NywrCMBQE0DupWp/1AYI7/6DEaLQu66Mrd35BKUWKJSlFv1+rue4cGM7shgR981qSon+ZNwUJ8iDgoYU2OvDRRQ99DDDECAHGmGCKmf80hZSx/Kik/LliFbtmN6xmt+yOjdg9GztV4tROnRwX/Bsaaw51nt4Lc7tWaZYHp/MlzKx51LZs5htNri+2AAAAAQAB//8AD3jaY2BkYGDgAWIxIGZiYARCESBmAfMYAAR6AEMAAAABAAAAANXtRbgAAAAA2AhRFAAAAADYCNuG") format("woff")
}

The URLs in the @font-face here should actually have the data: URLs replaced with filesystem URL via this code:

// Convert data: URLs into regular URLs, assuming there will be a file present (e.g. woff fonts in core themes).
foreach ( $source_data_url_objects as $i => $data_url ) {
$mime_type = strtok( substr( $data_url->getURL()->getString(), 5 ), ';' );
if ( ! $mime_type ) {
continue;
}
$extension = preg_replace( ':.+/(.+-)?:', '', $mime_type );
$guessed_urls = [];
// Guess URLs based on any other font sources that are not using data: URLs (e.g. truetype fallback for inline woff2).
foreach ( $source_file_urls as $source_file_url ) {
$guessed_url = preg_replace(
':(?<=\.)\w+(\?.*)?(#.*)?$:', // Match the file extension in the URL.
$extension,
$source_file_url,
1,
$count
);
if ( 1 === $count ) {
$guessed_urls[] = $guessed_url;
}
}
/*
* Guess some font file URLs based on the font name in a fonts directory based on precedence of Twenty Nineteen.
* For example, the NonBreakingSpaceOverride woff2 font file is located at fonts/NonBreakingSpaceOverride.woff2.
*/
if ( $stylesheet_base_url && $font_basename ) {
$guessed_urls[] = $stylesheet_base_url . sprintf( 'fonts/%s.%s', $font_basename, $extension );
$guessed_urls[] = $stylesheet_base_url . sprintf( 'fonts/%s.%s', strtolower( $font_basename ), $extension );
}
// Find the font file that exists, and then replace the data: URL with the external URL for the font.
foreach ( $guessed_urls as $guessed_url ) {
$path = $this->get_validated_url_file_path( $guessed_url, [ 'woff', 'woff2', 'ttf', 'otf', 'svg' ] );
if ( ! is_wp_error( $path ) ) {
$data_url->getURL()->setString( $guessed_url );
$converted_count++;
break;
}
}
} // End foreach $source_data_url_objects.

This is what is being done in Twenty Nineteen:

@font-face {
	font-family: "NonBreakingSpaceOverride";
	src: 
		url("https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff2") format("woff2"), 
		url("https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff") format("woff");
	font-display: swap
}

The reason why this works in Twenty Nineteen but not Twenty Twenty is that it includes the woff/woff2 font files with the theme:

https://github.com/WordPress/wordpress-develop/tree/master/src/wp-content/themes/twentynineteen/fonts

This is not the case for Twenty Twenty.

In order to save on the 3KB of CSS for Twenty Twenty or any other theme that includes the NonBreakingSpaceOverride as a data: URL but without including the file in the distributed theme, we can just include the file in the AMP plugin. We can also do this for other common fonts, like Genericons.

With the changes in this PR, the CSS manifest for Twenty Twenty now becomes:

The style[amp-custom] element is populated with:
   220 B: style[type=text/css]
    81 B: style
   583 B (74%): link#amp-default-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-default.css?ver=1.5.0-alpha][media=all]
  5037 B (28%): link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1575264840][media=all]
 40761 B (74%): link#twentytwenty-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentytwenty/style.css?ver=1.0][media=all]
  2131 B (93%): style#twentytwenty-style-inline-css
   122 B: style
    39 B: style[media=print]
   119 B: div.wp-block-cover has-background-dim alignwide amp-wp-d28a8c9
    88 B: img.wp-smiley amp-wp-843f19c[src=https://s.w.org/images/core/emoji/12.0.0-1/72x72/1f44b.png][alt=👋][width=72][height=72]
    75 B: p.amp-wp-224b51a
Total included size: 49,256 bytes (64% of 76,490 total after tree shaking)

The following stylesheets are too large to be included in style[amp-custom]:
  1223 B (97%): link#twentytwenty-print-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentytwenty/print.css?ver=1.0][media=print]
Total excluded size: 1,223 bytes (97% of 1,259 total after tree shaking)

Total combined size: 50,479 bytes (64% of 77,749 total after tree shaking)

There is still an excessive CSS error, but it is for a print stylesheet which has less prioritization. More work is needed to see if we can further reduce the amount of CSS, but this is an improvement.

For the previous work done for Twenty Nineteen, see #2079.

This is part of adding theme support for Twenty Twenty (#3342).

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v1.4.2 milestone Dec 2, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Dec 2, 2019
@westonruter westonruter added this to Ready for Review in Ongoing Dec 2, 2019
@westonruter westonruter merged commit 52ebfcc into develop Dec 4, 2019
@westonruter westonruter deleted the fix/externalize-data-url-font-faces branch December 4, 2019 04:08
westonruter added a commit that referenced this pull request Dec 4, 2019
* Bundle common font files for externalizing data: URLs

* Skip guessing the specifically-bundled fonts

* Add tests for fonts with data: URLs
westonruter added a commit that referenced this pull request Dec 4, 2019
@westonruter
Copy link
Member Author

Note: I failed to bump the cache group after the changes in this PR. Fixed in #3880.

westonruter added a commit that referenced this pull request Dec 4, 2019
…meo-shortocode-support

* 'develop' of github.com:ampproject/amp-wp: (159 commits)
  Return allowed KSES tags if not an array (#3879)
  Update dependency @wordpress/date to v3.6.0 (#3739)
  Update dependency @babel/plugin-proposal-object-rest-spread to… (#3813)
  Update dependency @babel/core to v7.7.4 (#3686)
  Update dependency @wordpress/api-fetch to v3.7.0 (#3731)
  Update dependency @wordpress/keycodes to v2.7.0 (#3745)
  Bundle common font files for externalizing data: URLs (#3866)
  Update dependency css-loader to v3.2.1 (#3870)
  Update dependency core-js to v3.4.7 (#3873)
  Update dependency core-js to v3.4.5 (#3695)
  Update dependency eslint-plugin-react to v7.17.0 (#3845)
  Update dependency eslint-plugin-jest to v23.1.1 (#3702)
  Update dependency eslint to v6.7.2 (#3811)
  Update dependency autoprefixer to v9.7.3 (#3850)
  Update dependency browserslist to v4.8.0 (#3849)
  Remove second sentence from paired browsing dialog
  Add async attribute to amp-paired-browsing-client script
  Restrict paired browsing to when dev mode is enabled
  Add robots noindex/nofollow meta tag in paired browsing app
  Remove extraneous method in favor of (temporary) closure
  ...
@westonruter westonruter moved this from Ready for Review to Ready for QA in Ongoing Dec 4, 2019
westonruter added a commit that referenced this pull request Dec 5, 2019
westonruter added a commit that referenced this pull request Dec 5, 2019
westonruter added a commit that referenced this pull request Dec 6, 2019
…id-markup-reason

* 'develop' of github.com:ampproject/amp-wp: (143 commits)
  Update dependency @wordpress/block-editor to v3.3.0 (#3691)
  Update dependency @wordpress/editor to v9.8.0 (#3693)
  Update dependency @wordpress/compose to v3.8.0 (#3736)
  Use comment as array key for data set to show when failure happens
  Remove unused AMP_YouTube_Embed_Handler::sanitize_v_arg method after 7a97571
  Bump stylesheet cache group after #3866 (#3880)
  Delete AMP_YouTube_Embed_Handler::shortcode() and oembed()
  Delete AMP_Twitter_Embed_Handler::oembed()
  Prevent wrapping plugin names in code tags
  Update dependency @wordpress/blocks to v6.8.0 (#3734)
  Update dependency @wordpress/core-data to v2.8.0 (#3737)
  Update dependency @wordpress/edit-post to v3.9.0 (#3692)
  Update dependency @wordpress/components to v8.4.0 (#3735)
  Update dependency @wordpress/element to v2.9.0 (#3741)
  Align @param descriptions in test_video_override
  Replace a call to ->shortcode() with the logic from shortcode()
  Refactor AMP_Vimeo_Embed_Handler::shortcode() into video_override()
  Deprecate AMP_YouTube_Embed_Handler::shortcode()
  Restore AMP_YouTube_Embed_Handler::video_override()
  Improve theme inline CSS checks
  ...
westonruter added a commit that referenced this pull request Dec 19, 2019
* tag '1.4.2': (25 commits)
  Bump 1.4.2
  Bump 'tested up to' to 5.3.2
  Catch unfiltered requests when testing Crowsignal embed (#3956)
  Bump version 1.4.2-RC1
  Bump 'Tested up to' to 5.3.1
  Remove test case for paired browsing since broken in WP4.9 and would not pass in AMP 1.4 (#3928)
  Further refine is_exclusively_dependent and add tests (#3928)
  Only include hoverintent-js in dev mode if exclusive dependency of admin-bar (#3928)
  Include admin-bar script deps in dev mode (e.g. hoverintent-js) (#3928)
  Fix tests after security fix in WP 5.3.1 (r46896) (#3926)
  Add E2E tests to allow_failures
  Convert `theme_features` variable into `get_theme_config` function
  Only apply smooth scroll fix on 'Twenty Twenty' <= 1.0.0
  Re-add smooth scrolling fix on WP < 5.3.1
  No need to apply smooth scrolling fix for Twenty Twenty theme
  Bump stylesheet cache group after #3866 (#3880)
  Return allowed KSES tags if not an array (#3879)
  Bundle common font files for externalizing data: URLs (#3866)
  Pull the built `block-libray` package from Gutenberg SVN if it does not exists (#3847)
  Optimize is_amp_allowed_attribute checking for reference points (#3815)
  ...
@csossi
Copy link

csossi commented Dec 29, 2019

Verified in QA

@csossi csossi moved this from Ready for QA to Done in Ongoing Dec 29, 2019
@csossi csossi added the QA passed Has passed QA and is done label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA QA passed Has passed QA and is done
Projects
Ongoing
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants