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

Short-circuit Asset CDN in AMP responses #11374

Merged
merged 1 commit into from
Feb 18, 2019

Conversation

westonruter
Copy link
Contributor

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:

@font-face{font-family:dashicons;src:url("data:application/font-woff;charset=utf-8;base64,d09GRgABAAAAAGYMAA4AAAAAowAAAQ..."

When there should instead be:

@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:

  1. Activate the Twenty Seventeen theme.
  2. Activate the AMP plugin.
  3. On the AMP settings screen, turn on Native mode.
  4. Enable the Asset CDN module.
  5. 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.
  6. 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.

@westonruter westonruter requested a review from a team as a code owner February 16, 2019 11:21
@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 5649ba7

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it AMP labels Feb 18, 2019
@jeherve jeherve added this to the 7.1 milestone Feb 18, 2019
Copy link
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.

This tests well. Thank you! Merging.

@jeherve jeherve merged commit d93ec03 into Automattic:master Feb 18, 2019
jeherve added a commit that referenced this pull request Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants