Include social-logos.woff for sake of AMP to reference #9513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeherve
added
[Type] Enhancement
jeherve
added
[Status] Ready to Merge
ghost
removed
the
[Status] Ready to Merge
jeherve
pushed a commit
that referenced
this pull request
Feb 18, 2019
I noticed that when turning on the Asset CDN module that styles were broken in AMP responses. In looking at the AMP plugin's CSS manifest comment I saw that that the Dashicons were were way larger than expected: ``` 35249 B (90%): link#dashicons-css[rel=stylesheet][id=dashicons-css][href=https://c0.wp.com/c/5.0.3/wp-includes/css/dashicons.min.css][type=text/css][media=all] ``` This is due to the fact that the AMP plugin was loading the CSS from the remote URL as opposed to using the local copy. Recall that AMP requires all CSS to be inlined (except for CDN stylesheets). Also, AMP allows a max of 50KB. Since the `data:` URL for the Dashicons font is very large, the AMP plugin will instead try to reference the font file with `https:` instead of the `data:` URL. See previously in #9513. All of this to say, the Asset CDN is breaking this since it fetches the CSS from a remote location. And in the `style[amp-custom]` element I see unexpectedly: ```css @font-face{font-family:dashicons;src:url("data:application/font-woff;charset=utf-8;base64,d09GRgABAAAAAGYMAA4AAAAAowAAAQ..." ``` When there should instead be: ```css @font-face{font-family:dashicons;src:url("https://example.com/wp-includes/fonts/dashicons.woff") format("woff"); ``` The `data:` URL is causing the theme's stylesheet to be excluded: ``` The following stylesheets are too large to be included in style[amp-custom]: 23967 B (64%): link#twentyseventeen-parent-style-css[rel=stylesheet][id=twentyseventeen-parent-style-css][href=https://amp-jetpack-westonruter.pantheonsite.io/wp-content/themes/twentyseventeen/style.css?ver=1.1][type=text/css][media=all] Total excluded size: 23,967 bytes (64% of 37,093 total after tree shaking) ``` So the fix is simply to short-circuit the Asset CDN from doing its thing when the response will be an AMP page. As noted in the comments, the Asset CDN is not relevant in AMP because custom JS is not allowed and CSS is inlined. Note also that it is not suitable to use the `jetpack_force_disable_site_accelerator` filter for this because it will be applied before the `wp` action, which is the point at which the queried object is available and we know whether the response will be AMP or not. This is particularly important for AMP-first (native AMP) pages where there are no AMP-specific URLs. For more on that, see #11195. See #9730 which is the master issue for AMP compatibility. #### Changes proposed in this Pull Request: * Short-circuit Asset CDN module in AMP responses. #### Testing instructions: 0. Activate the Twenty Seventeen theme. 1. Activate the [AMP plugin](https://github.com/ampproject/amp-wp). 2. On the AMP settings screen, turn on Native mode. 3. Enable the Asset CDN module. 4. View the frontend and notice that the page looks broken in terms of styling, and the AMP CSS unexpectedly includes a `data:` URL for the font. 5. Switch to this branch and notice the styling is no longer broken and the AMP CSS no longer uses the `data:` URL for the Dashicons font. #### Proposed changelog entry for your changes: * Add AMP compatibility for the Asset CDN module.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because fonts loaded via
data:URLs contribute a lot toward the 50KB limit in AMP, the AMP plugin attempts to load the font via a regular URL if the font file can be found to be located on the filesystem. See:https://github.com/Automattic/amp-wp/blob/7439d2ab5eebd0ee071749a849fda946a2eff4be/includes/sanitizers/class-amp-style-sanitizer.php#L920-L1030
I noticed that while there is a WOFF font in the
data:URL there is no such.wofffile in the repo. This fixes that. This better aligns the Jetpack repo with core which includes the original WOFF files in the repo for Dashicons and Genericons:https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/css/dashicons.css
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/fonts/dashicons.woff
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-content/themes/twentyfifteen/genericons/Genericons.woff
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-content/themes/twentyfifteen/genericons/genericons.css