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

Remove restriction that external stylesheets have .css extension #1317

Closed
westonruter opened this Issue Aug 4, 2018 · 4 comments

Comments

3 participants
@westonruter
Copy link
Member

westonruter commented Aug 4, 2018

Initially the inclusion of external styles were limited to those that were on the filesystem. But since #1083 we also fetch external styles over HTTP and then cache them. So now if a URL lacks the .css extension (or it is not found on the filesystem even) we can just fallback to fetching the stylesheet over the network.

This issue was noticed on WordPress.com VIP where automatic CSS concatenation results in a stylesheet URL that does not have a static filename extension.

@westonruter westonruter added the css label Aug 4, 2018

@westonruter westonruter added this to the v1.0 milestone Aug 4, 2018

@westonruter westonruter added this to To do in v1.0 Aug 5, 2018

@westonruter westonruter added the release label Aug 22, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 24, 2018

Another related issue we can solve here. I've seen this come up twice now (e.g. #1269 (comment)) where a theme author includes a Google Font with an @import rule like:

@import 'https://fonts.googleapis.com/css?family=Merriweather:300|PT+Serif:400i|Open+Sans:800|Zilla+Slab:300,400,500|Montserrat:800|Muli:400&subset=cyrillic-ext,latin-ext,cyrillic,greek,greek-ext,vietnamese';

This causes a strange validation error message to be raised:

Skipped file which does not have an allowed file extension (//fonts.googleapis.com/css).

Using @import is not a recommended way to load CSS. It should use wp_enqueue_style() instead, similar to how Twenty Seventeen is doing. This will ensure that the stylesheet is loaded at the optimal time, and that WordPress can add the preconnect link.

Nevertheless, the AMP plugin should prevent trying to fetch such font stylesheets server-side to include in the concatenated script because AMP allows them to be referenced externally. In addition, the font should be cached according to its Cache-Control headers and not stored in a transient.

So when we come across such an @import, we should actually dynamically create a link element in the document with an href pointing to the imported URL.

@westonruter westonruter self-assigned this Aug 24, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 24, 2018

/cc @igkuz

@igkuz

This comment has been minimized.

Copy link

igkuz commented Aug 27, 2018

👍

@hellofromtonya hellofromtonya moved this from Ready for review to Ready for QA in v1.0 Aug 27, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 18, 2018

Moving To 'Ready For Merging'

If it's alright, I'm moving this to 'Ready For Merging,' as I don't think it needs functional testing. Feel free to move it back.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Sep 18, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

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.