AMP: move checks for AMP requests later, inside modules. #11195

jeherve · 2019-01-24T17:31:16Z

Fixes #11151
Fixes #11148
Fixes #11123
Fixes #11169

Changes proposed in this Pull Request:

Internal discussion: p1HpG7-6cV-p2

See #11169 (comment)

the AMP checks should be deferred to as late as possible.

Testing instructions:

Install the AMP plugin.
Switch AMP mode (AMP > General) to Paired or Classic.
Activate the Carousel module, the sharing module, and ensure that SCRIPT_DEBUG is set to false on your install.
Create a post with a gallery
Add a Facebook sharing button to that post.
Share that post on Facebook once.
Comment on one of the images in the gallery.
Load the post in a non-AMP view, and in the 3 modes available in the AMP options screen: Native, Paired, Classic. (Native mode - all views are AMP views; Paired mode - add ?amp to get to the AMP view; Classic mode - add /amp to get to the AMP view)
- In non-AMP views: Does the Carousel modal work? Do you see the comment in the Carousel modal? Do you see the sharing buttons? Do you see the counter next to the sharing button? Do you see the jetpack.css file when viewing source?
- In AMP views: you should not see the Carousel. You should see a special styling of the sharing buttons. If you check the network tab in your browser tools, you should see a request to pixel.wp.com when logged out. You should not see a jetpack.css file in the source.

In all cases:

You should not see any js errors in the browser console.
You should not get any PHP notices in your debug log.

Now try adding the following to a functionality plugin on your site:

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

Once you've done so, check the non-AMP view again:

you should not see the sharing counter on the Facebook button.
you should not see the jetpack.css file in your source.

Proposed changelog entry for your changes:

Carousel: avoid errors when fetching comments in the Carousel modal.
CSS: fix the behaviour of the CSS concatenation filter.

See #11169 (comment)

matticbot · 2019-01-24T17:31:23Z

D23501-code. (newly created revision)

jetpackbot · 2019-01-24T17:34:13Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: February 5, 2019.
Scheduled code freeze: January 29, 2019

Generated by 🚫 dangerJS against 206b614

jeherve · 2019-01-25T16:42:37Z

@westonruter Would you have the chance to give this PR a try, by any chance?

westonruter · 2019-01-25T17:50:18Z

Yes. I will try to do so later today or tomorrow.

westonruter

I examined the changes and they all seem valid.

westonruter · 2019-01-26T01:56:30Z

modules/lazy-images/lazy-images.php

@@ -40,6 +40,10 @@ private function __construct() {
 			return;
 		}

+		if ( Jetpack_AMP_Support::is_amp_request() ) {
+			return;
+		}


👍 This is works because Jetpack_Lazy_Images is instantiated at the wp action:

jetpack/modules/lazy-images.php

Lines 29 to 40 in cb4dcbb

/*

* Initialize lazy images on the wp action so that conditional

* tags are safe to use.

*

* As an example, this is important if a theme wants to disable lazy images except

* on single posts, pages, or attachments by short-circuiting lazy images when

* is_singular() returns false.

*

* See: https://github.com/Automattic/jetpack/issues/8888

*/

add_action( 'wp', array( 'Jetpack_Lazy_Images', 'instance' ) );

It might be a good idea to mention AMP as being the reason for the wp action there.

westonruter · 2019-01-26T04:00:03Z

modules/sharedaddy/sharing-service.php

@@ -577,6 +577,10 @@ function sharing_maybe_enqueue_scripts() {
 }

 function sharing_add_footer() {
+	if ( Jetpack_AMP_Support::is_amp_request() ) {
+		return;
+	}


👍 This will work fine because it will run at the wp_footer action, which happens after wp.

westonruter · 2019-01-26T04:07:12Z

class.jetpack.php

@@ -6873,6 +6873,11 @@ public function implode_frontend_css( $travis_test = false ) {
 			$do_implode = false;
 		}

+		// Do not implode CSS when the page loads via the AMP plugin.
+		if ( Jetpack_AMP_Support::is_amp_request() ) {


👍 This call is made during wp_print_scripts so it will work well.

westonruter · 2019-01-26T04:08:59Z

3rd-party/class.jetpack-amp-support.php

-
-		// disable imploding CSS
-		add_filter( 'jetpack_implode_frontend_css', array( __CLASS__, 'is_not_amp_request' ) );
-


Good that these are integrated with the modules they relate to.

brbrr

FWIW: When testing on JN site - I see this error on jetpack_modules page: Notice: Undefined index: copy-post in /srv/users/user3bb07467/apps/user3bb07467/public/wp-content/plugins/jetpack-dev/modules/module-headings.php on line 248

jeherve · 2019-01-28T12:20:11Z

I see this error on jetpack_modules page

This seems unrelated to this PR, and should disappear if you run yarn build.

brbrr

Tests well. Thanks!

@font-face

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.

Summary: After adding the AMP compat file that ships with Jetpack (this was done in D22313, D26814, D26819, D26921, and D26928), let's start using this class. This replaces D22154 and D23501, and allows us to start syncing any future changes that would rely on the Jetpack_AMP_Support class. Related Jetpack PRs: - #10945 - #11195, which reverted some of the changes in the PR above. - #12054 - #12053 - #12026 Discussion: - https://[private link] We'll need to test for issues like the ones that popped up on Jetpack at the time: #11169 We will also need to account for the fact that WordPress.com does not run the latest version of the AMP plugin. It runs an old version (0.6.2 vs. the current version on W.org 1.0.2), with its own version of some of the compatibility fixes that come with the class we've added (see [[ https://[private link].php | here ]]) and needs to be updated as per the discussions here: - https://[private link] (D14521) - https://[private link] Test Plan: * Create a post with a gallery * Add a Facebook sharing button to that post. * Try Liking that post. * Comment on one of the images in the gallery. * Load the post in an AMP view, and repeat the steps below. In all cases: - You should not see any js errors in the browser console. - You should not get any PHP notices in your debug log. Reviewers: zinigor Tags: #touches_jetpack_files Differential Revision: D26821-code This commit syncs r190790-wpcom.

AMP: move checks for AMP requests later, inside modules.

206b614

See #11169 (comment)

jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review A Jetpack Crew expert's review is needed. Typically for significant changes to core functionality. [Pri] High AMP labels Jan 24, 2019

jeherve added this to the 7.0 milestone Jan 24, 2019

jeherve self-assigned this Jan 24, 2019

jeherve requested a review from a team January 24, 2019 17:31

matticbot added the Touches WP.com Files label Jan 24, 2019

jeherve added [Pri] BLOCKER and removed [Pri] High labels Jan 24, 2019

westonruter reviewed Jan 26, 2019

View reviewed changes

brbrr reviewed Jan 28, 2019

View reviewed changes

brbrr approved these changes Jan 28, 2019

View reviewed changes

zinigor merged commit 3625ed1 into master Jan 28, 2019

matticbot added the [Status] Needs Changelog label Jan 28, 2019

zinigor deleted the add/amp-checks-modules branch January 28, 2019 13:44

jeherve added a commit that referenced this pull request Jan 29, 2019

Changelog: add #11195

6bccee9

westonruter mentioned this pull request Feb 16, 2019

Short-circuit Asset CDN in AMP responses #11374

Merged

westonruter mentioned this pull request Feb 27, 2019

AMP compatibility issues #9730

Open

jeherve mentioned this pull request Apr 16, 2019

AMP: check if class exists before to use it. #12062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AMP: move checks for AMP requests later, inside modules. #11195

AMP: move checks for AMP requests later, inside modules. #11195

jeherve commented Jan 24, 2019 •

edited by brbrr

Loading

matticbot commented Jan 24, 2019

jetpackbot commented Jan 24, 2019

jeherve commented Jan 25, 2019

westonruter commented Jan 25, 2019

westonruter left a comment

westonruter Jan 26, 2019

westonruter Jan 26, 2019

westonruter Jan 26, 2019

westonruter Jan 26, 2019

brbrr left a comment

jeherve commented Jan 28, 2019

brbrr left a comment

	/*
	* Initialize lazy images on the wp action so that conditional
	* tags are safe to use.
	*
	* As an example, this is important if a theme wants to disable lazy images except
	* on single posts, pages, or attachments by short-circuiting lazy images when
	* is_singular() returns false.
	*
	* See: https://github.com/Automattic/jetpack/issues/8888
	*/

	add_action( 'wp', array( 'Jetpack_Lazy_Images', 'instance' ) );


		// disable imploding CSS
		add_filter( 'jetpack_implode_frontend_css', array( __CLASS__, 'is_not_amp_request' ) );

AMP: move checks for AMP requests later, inside modules. #11195

AMP: move checks for AMP requests later, inside modules. #11195

Conversation

jeherve commented Jan 24, 2019 • edited by brbrr Loading

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Jan 24, 2019

jetpackbot commented Jan 24, 2019

jeherve commented Jan 25, 2019

westonruter commented Jan 25, 2019

westonruter left a comment

Choose a reason for hiding this comment

westonruter Jan 26, 2019

Choose a reason for hiding this comment

westonruter Jan 26, 2019

Choose a reason for hiding this comment

westonruter Jan 26, 2019

Choose a reason for hiding this comment

westonruter Jan 26, 2019

Choose a reason for hiding this comment

brbrr left a comment

Choose a reason for hiding this comment

jeherve commented Jan 28, 2019

brbrr left a comment

Choose a reason for hiding this comment

jeherve commented Jan 24, 2019 •

edited by brbrr

Loading