Skip to content

Include social-logos.woff for sake of AMP to reference#9513

Merged
gravityrail merged 1 commit intoAutomattic:masterfrom
xwp:add/social-logos-woff
Jun 1, 2018
Merged

Include social-logos.woff for sake of AMP to reference#9513
gravityrail merged 1 commit intoAutomattic:masterfrom
xwp:add/social-logos-woff

Conversation

@westonruter
Copy link
Copy Markdown
Contributor

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 .woff file 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

@westonruter westonruter requested a review from a team as a code owner May 9, 2018 05:59
@jeherve jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Theme Tools [Status] Needs Review This PR is ready for review. [Pri] Normal labels May 9, 2018
@gravityrail gravityrail added this to the 6.2 milestone Jun 1, 2018
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jun 1, 2018
@gravityrail gravityrail merged commit 5509709 into Automattic:master Jun 1, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 1, 2018
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

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Theme Tools [Pri] High

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants