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

Ensure font stylesheets are requested in CORS mode in both AMP and non-AMP documents #1486

Merged
merged 2 commits into from Oct 3, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

commented Oct 3, 2018

The AMP plugin is currently using the post-processor to add crossorigin=anonymous to font stylesheet links. This is not being done, however, in the non-AMP responses. This is a problem because when a service worker is caching the font stylesheet responses with CORS but then tries to serve it in response to a stylesheet being requested without CORS, then the fetch fails entirely with:

The FetchEvent resulted in a network error response: an "opaque" response was used for a request whose type is not no-cors

So we need to make sure that such font URLs always get served with CORS (crossorigin="anonymous") when in AMP and not-AMP alike to prevent this problem from happening.

Another benefit of CORS is that it ensures that a service worker caching the external stylesheet will not inflate the storage quota. See:

This PR is cherry-picking 02fd0a7 from #1261. We'll not ship the PWA plugin integration for 1.0, but this fix needs to be deployed first.

@westonruter westonruter added this to the v1.0 milestone Oct 3, 2018

@westonruter westonruter requested review from amedina and kienstra Oct 3, 2018

@amedina

amedina approved these changes Oct 3, 2018

Copy link
Member

left a comment

LGTM.

@westonruter westonruter force-pushed the fix/font-stylesheet-cors branch from 7ad5fca to 4f2259c Oct 3, 2018

@westonruter westonruter merged commit 9bc5c04 into 1.0 Oct 3, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/font-stylesheet-cors branch Oct 3, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2018

This Looks Good

Hi @westonruter,
To echo @amedina, this PR looks good. As expected, a font stylesheet in Twenty Seventeen has crossorigin="anonymous" in both the AMP and non-AMP URLs:

crossorigin-anonymous-present

kienstra added a commit that referenced this pull request Oct 3, 2018

Add PR #1486 to the Changelog
This was recently merged,
so ensure that it's added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.