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

AMP: last AMP compatibility changes impact CSS concatenation #11123

Closed
jeherve opened this issue Jan 10, 2019 · 7 comments · Fixed by #11195
Closed

AMP: last AMP compatibility changes impact CSS concatenation #11123

jeherve opened this issue Jan 10, 2019 · 7 comments · Fixed by #11195
Assignees
Labels
AMP [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Milestone

Comments

@jeherve
Copy link
Member

jeherve commented Jan 10, 2019

In #10945, we made changes to our AMP compatibility file that impact the way we load CSS. Unfortunately, this change impacts all site owners currently using the jetpack_implode_frontend_css filter

Steps to reproduce the issue

  1. Start from a site using Jetpack 6.8.1 and with the following code snippet: add_filter( 'jetpack_implode_frontend_css', '__return_false' );
  2. Separate CSS files are loaded by Jetpack on the frontend.
  3. Update to Jetpack 6.9
  4. Your code snippet does not have any impact anymore, the concatenated jetpack.css is loaded instead. You have to change the priority for it to start working again, like add_filter( 'jetpack_implode_frontend_css', '__return_false', 99 );.

I have not tested, but I suspect this also impacts the other filters in use by the AMP compatibility file.

Reported here:
https://jetpack.com/2019/01/10/jetpack-6-9-introducing-more-tools-for-the-new-block-editor/#comment-193496

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Focus] Performance AMP labels Jan 10, 2019
@jeherve jeherve added this to the 7.0 milestone Jan 10, 2019
@superpoincare
Copy link

add_filter( 'jetpack_implode_frontend_css', '__return_false', 11 );

itself works, not just 99.

@jeherve
Copy link
Member Author

jeherve commented Jan 10, 2019

@macmanx2 confirmed this also impacts another filter on that list, jetpack_sharing_counts

@macmanx2
Copy link
Contributor

Thanks, Jeremy!

Confirming that add_filter( 'jetpack_sharing_counts', '__return_false', 99 ); does indeed work.

If you're using the Code Snippets plugin, simply setting the priority there to 99 doesn't seem to fix the problem, but if you change your snippet to add_filter( 'jetpack_sharing_counts', '__return_false', 99 ); and set whatever priority level you want in the plugin, it works perfectly.

@chriscoyier
Copy link

Hey gang! Just noting I got burned by this. It's a heck of a thing to have essentially dequeued Jetpack CSS only to have it show up all the sudden by upgrading a version with no warning.

@Garconis
Copy link

Garconis commented Nov 6, 2019

It would be nice if your plugin would first check to see if any of the modules need to have your jetpack.css and devicepx-jetpack.js loaded. Right now, none of the modules that we have active within Jetpack require those files to be loaded on the frontend. Rather than having to remove those via PHP snippets, it would be good if your plugin removed them automatically when they aren't needed.

@jeherve
Copy link
Member Author

jeherve commented Nov 6, 2019

@Garconis We currently treat those 2 files a bit differently, as they do different things:

@Garconis
Copy link

Garconis commented Nov 6, 2019

@jeherve thanks! For now, I will keep both removed via PHP, but will keep an eye on the pulls/issues to adjust as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants