Prevent Jetpack features from breaking AMP validation with amp-wp by gravityrail · Pull Request #9458 · Automattic/jetpack

gravityrail · 2018-05-01T19:51:00Z

This patch does the minimum necessary to allow Jetpack features to work on AMP pages, and in some cases to prevent Jetpack features from rendering to the front end at all. This deprecates the Jetpack helper included in AMP, and some parts of the WPCOM helper - see this PR.

Q. Why don't we want some features on the front end at all?
A. Because in some cases features aren't possible, due to the limitations of AMP. In other cases, it would require a lot of work to make a feature AMP-compatible and in the meantime we want pages to validate correctly without broken-looking output.

This also includes WPCOM support where possible, fixing ampproject/amp-wp#1021

Future work should be done to ensure that these features can be used fully with AMP.

disable likes (fix: Port Likes and Comment Likes to work with AMP #9555)
disable comment likes (fix: ^^ )
disable related posts (fix: Port Related Posts to work with AMP #9556)
modify social sharing to use <amp-social-share>
disable carousel (fix: Port Carousel to work with AMP #9557)
disable devicepx
disable photon javascript (but allow photon to work)
disable notifications
disable milestone widget (fix: Port Milestone widget to work with AMP #9558)
force 'freedom' attribute for videopress shortcode
use <amp-pixel> for stats pixel
force using separate stylesheets to avoid unnecessary tree shaking
Include jetpack_og_tags in head of legacy AMP pages where possible.
Includes blavatar/site_icon and post image metadata in post head where possible.
Disables some content filters, both Jetpack and WPCOM-only.

Some implementation notes:

We do not use is_amp_endpoint() as it is not reliable when called during init or earlier. This is because it requires parse_query to have been run. Instead, we look for the amp query var or canonical amp support. We can go back to using is_amp_endpoint() once Discontinue use of amp endpoint in favor of query var when amp theme support is present ampproject/amp-wp#1148 is resolved.

Known issues:

Masterbar doesn't appear on AMP, probably needs to be rewritten.
Tiled galleries display as slideshow

Testing

Check out my PR of amp-wp in your wp-content/plugins dir: Deprecate Jetpack helper and some parts of WPCOM helper ampproject/amp-wp#1149
Run npm run build inside the checkout.
Activate AMP in your site's plugins list

Legacy AMP

Add ?amp=1 to any post URL, e.g. http://example.com/2018/03/23/a-post/?amp=1

All Jetpack features should work (or be hidden if broken) in this mode.

Canonical AMP

Put this in a plugin:

function add_amp_theme_support() {
	add_theme_support( 'amp' );
}

add_action( 'plugins_loaded', 'add_amp_theme_support' );

This will set the home page to use "canonical amp", which the the preferred way of using AMP after 1.0.

All Jetpack features should work (or be hidden if broken) in this mode.

Now try using various Jetpack features and make sure they don't break validation (or simply break).

Screenshots

Legacy AMP

Before	After

Canonical AMP

Before	After

Changelog entry

AMP: Allow Jetpack features to work on AMP pages, and prevent Jetpack features from rendering to the front end at all.

…of new module

gravityrail · 2018-05-15T17:07:24Z

cc @westonruter, whose work this is based on, for review and comment.

westonruter · 2018-05-15T18:12:31Z

Regarding tiled galleries rendering as carousel, see ampproject/amp-wp#1065

ebinnion · 2018-05-16T20:24:02Z

_inc/lib/class.jetpack-amp-support.php

@@ -0,0 +1,141 @@
+<?php


Should this file go in the 3rd-party directory?

Good call, will move there.

jeherve

Just a few questions about the current implementation.

jeherve · 2018-05-17T08:21:07Z

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

+	static function add_fallback_image_to_metadata( $metadata ) {
+		$metadata['image'] = array(
+			'@type'  => 'ImageObject',
+			'url'    => self::staticize_subdomain( 'https://wordpress.com/i/blank.jpg' ),


Should we leverage the existing jetpack_open_graph_image_default filter here?

jeherve · 2018-05-17T08:24:32Z

modules/sharedaddy/sharing-service.php

@@ -828,7 +830,7 @@ function sharing_display( $text = '', $echo = false ) {
 	 *
 	 * @param string $sharing_content Content markup of the Jetpack sharing links


Could you add the new parameter you've added here?

jeherve · 2018-05-17T08:26:04Z

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

+		return function_exists( 'amp_is_canonical' ) && amp_is_canonical();
+	}
+
+	static function is_amp_request() {


Could we make this function filterable, so other AMP plugins like this one can leverage the work we've done in Jetpack?

Good call. Done.

jeherve · 2018-05-17T08:28:06Z

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

+		return $widgets;
+	}
+
+	static function is_supported_widget( $widget_path ) {


What makes an AMP supported widget today? Are there any additional widgets we should add to that list, like the Twitter and Facebook widgets?

Almost certainly there are other compatibility issues; this PR aims only to port the fixes that have already been implemented by XWP in the amp-wp plugin. We should do a larger audit of other things that aren't working so well, but I didn't want this patch to get too big before we merge it.

It looks like slideshow is also not supported, but we can address that in a follow-up.

It appears gists don't work as well.

Really all of the core widgets work in AMP, except for the dropdown mode for Categories/Archives, which require a non-JS solution for navigating to the selected option. We should extend the widgets in Jetpack with the necessary checks for is_amp_endpoint() to output the necessary AMP components.

ebinnion

I left a few minor comments. I tested against your amp-wp patch and things seem to work well.

ebinnion · 2018-05-16T20:24:13Z

_inc/lib/class.jetpack-amp-support.php

+ * @see https://github.com/Automattic/amp-wp
+ */
+class Jetpack_AMP_Support {
+	// static $modules_to_disable = array( 'likes', 'comment-likes', 'related-posts', 'carousel', 'photon', 'lazy-images', 'notes' );


We should remove this before merging.

ebinnion · 2018-05-16T20:29:03Z

_inc/lib/class.jetpack-amp-support.php

+		// enforce freedom mode for videopress
+		add_filter( 'videopress_shortcode_options', array( 'Jetpack_AMP_Support', 'videopress_enable_freedom_mode' ) );
+
+		// DONE


Perhaps we should track this in an issue instead of in the code?

ebinnion · 2018-05-16T20:44:16Z

_inc/lib/class.jetpack-amp-support.php

+// this is necessary since for better or worse Jetpack modules are loaded during plugins_loaded, which means we must
+// take the opportunity to intercept initialisation before that point, either by adding explicit detection into the module,
+// or preventing it from loading in the first place (better for performance)
+add_action( 'plugins_loaded', array( 'Jetpack_AMP_Support', 'init_filter_jetpack_modules' ), 1 );


There should be an empty newline at the end of this file.

ebinnion · 2018-05-16T21:01:12Z

modules/lazy-images/lazy-images.php

 	}

 	public function setup_filters() {
+		if ( Jetpack_AMP_Support::is_amp_request() ) {


Since lazy images loads on the wp hook, I think we could probably just hook like this:

add_filter( 'lazyload_is_enabled', '__return_false' );

This would allow us to bail as lazy images is initializing so that we don't have to place checks in multiple spots.

Good call. Done.

ebinnion · 2018-05-16T21:08:22Z

modules/sharedaddy/sharing-service.php

@@ -828,7 +830,7 @@ function sharing_display( $text = '', $echo = false ) {
 	 *
 	 * @param string $sharing_content Content markup of the Jetpack sharing links


We should update this docblock to reflect the new $enabled parameter and probably add a second @since. Perhaps something like:

@since 6.2.0 Started sending $enabled as a second parameter.

gravityrail · 2018-05-17T16:39:53Z

Thanks @enejb - I added your suggestion.

ebinnion · 2018-05-17T19:43:45Z

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

+	}
+
+	static function is_supported_widget( $widget_path ) {
+		return substr($widget_path, -14) !== '/milestone.php';


Minor style nitpick: There should be spaces inside the parentheses.

Fixed in new PR

ebinnion · 2018-05-17T19:46:06Z

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

+	/**
+	 * Add publisher and image metadata.
+	 *
+	 * @since 0.3


Should these methods be 6.2.0 instead? I imagine that they're brought over from amp-wp and were probably added in that version there.

Perhaps it could be something like:

@since 0.3 In amp-wp plugin @since 6.2.0 Ported to Jetpack

I'm not sure. It's also pretty minor.

Fixed in new PR

ebinnion · 2018-05-17T19:47:25Z

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

+// this is necessary since for better or worse Jetpack modules and widget files are loaded during plugins_loaded, which means we must
+// take the opportunity to intercept initialisation before that point, either by adding explicit detection into the module,
+// or preventing it from loading in the first place (better for performance)
+add_action( 'plugins_loaded', array( 'Jetpack_AMP_Support', 'init_filter_jetpack_widgets' ), 1 );


Minor nitpick: There should an empty newline at the end of the file.

Fixed in new PR

ebinnion · 2018-05-17T19:49:26Z

class.jetpack.php

 		}

+		// detect amp-wp and modify output as appropriate
+		require_once JETPACK__PLUGIN_DIR . '3rd-party/class.jetpack-amp-support.php';


If we're just running this in the construct without a specific condition, should it go in the main 3rd-party file instead? That file should be loaded before the devicepx method in the Jetpack class is called.

Good call, fixed.

ebinnion · 2018-05-17T19:56:37Z

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

+		return $widgets;
+	}
+
+	static function is_supported_widget( $widget_path ) {


It looks like slideshow is also not supported, but we can address that in a follow-up.

ebinnion

This looks good to me and tests well. I left some minor comments that are probably best addressed in a follow-up PR. I'll leave that up to you though.

westonruter · 2018-05-18T16:53:49Z

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

+				'width'  => $size,
+				'height' => $size,
+			);
+		} else if ( $site_icon_url = Jetpack_Sync_Functions::site_icon_url( $size ) ) {


Note that Custom Logo in core should be used if available. See ampproject/amp-wp#975

Added this as an issue: #9587

westonruter · 2018-05-18T17:02:52Z

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

+			&&
+				function_exists( 'amp_is_canonical' ) // this is really just testing if the plugin exists
+			&&
+				( amp_is_canonical() || isset( $_GET[ amp_get_slug() ] ) );


Until ampproject/amp-wp#1148 is implemented, this should also check if the URL path ends in /amp/. Otherwise, AMP URLs on sites with amp theme support like https://make.xwp.co/2018/05/03/wordpress-amp-plugin-0-7-release/amp/ won't get recognized as an AMP request here.

Added this as an issue: #9588

kraftbj · 2018-05-22T02:33:01Z

I'm trying to track an issue I'm seeing on my personal site where the stats pixel isn't loading on normal requests (but working on AMP requests). The timing of this merge lines up. Anyone else seeing anything similar?

kraftbj · 2018-05-22T02:39:21Z

modules/stats.php

+	_stq.push([ 'clickTrackerInit', '{$data['blog']}', '{$data['post']}' ]);
+</script>
+
+END;


We're missing a print here from moving everything around.

After #9458, the pixel was not printed to the front-end.

AMP hackery

1c82495

gravityrail requested a review from a team as a code owner May 1, 2018 19:51

Merge branch 'master' into add/amp-wp-support

d96c6aa

gravityrail changed the title ~~AMP hackery~~ Prevent Jetpack features from breaking AMP validation with amp-wp May 14, 2018

Refactor AMP support into small changes to existing modules, instead …

1b42e6f

…of new module

matticbot added the Touches WP.com Files label May 15, 2018

Add videopress and stats pixel support

6f40576

gravityrail mentioned this pull request May 15, 2018

Deprecate Jetpack helper and some parts of WPCOM helper ampproject/amp-wp#1149

Merged

gravityrail requested review from beaulebens and ebinnion May 15, 2018 16:39

gravityrail added this to the 6.2 milestone May 15, 2018

This was referenced May 15, 2018

Port Likes and Comment Likes to work with AMP #9555

Open

Port Related Posts to work with AMP #9556

Closed

Port Carousel to work with AMP #9557

Open

Port Milestone widget to work with AMP #9558

Open

gravityrail self-assigned this May 15, 2018

Port some features from wpcom-helper.php

eafa25d

gravityrail mentioned this pull request May 15, 2018

Move WPCOM and Jetpack-specific codebase into the Jetpack plugin ampproject/amp-wp#1021

Closed

gravityrail requested a review from philipjohn May 15, 2018 18:23

jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it General [Status] In Progress [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels May 15, 2018

ebinnion reviewed May 16, 2018

View reviewed changes

Move amp support to 3rd-party

af4db8f

jeherve reviewed May 17, 2018

View reviewed changes

ebinnion reviewed May 17, 2018

View reviewed changes

ebinnion approved these changes May 17, 2018

View reviewed changes

gravityrail merged commit 1bd4eaa into master May 17, 2018

matticbot added the [Status] Needs Changelog label May 17, 2018

gravityrail deleted the add/amp-wp-support branch May 17, 2018 20:03

gravityrail mentioned this pull request May 17, 2018

Cleanup amp support #9570

Merged

jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release and removed [Status] Needs Review This PR is ready for review. labels May 17, 2018

westonruter reviewed May 18, 2018

View reviewed changes

This was referenced May 20, 2018

Use Custom Logo for AMP site icon if available #9587

Open

Check for /amp/ suffix when determining is_amp_request #9588

Closed

jeherve added the AMP label May 21, 2018

kraftbj reviewed May 22, 2018

View reviewed changes

kraftbj added a commit that referenced this pull request May 22, 2018

Print non-AMP stats footer

f996eb6

After #9458, the pixel was not printed to the front-end.

kraftbj mentioned this pull request May 22, 2018

Stats: Print stats pixel again #9596

Merged

oskosk pushed a commit that referenced this pull request May 24, 2018

Print non-AMP stats footer (#9596)

32df762

After #9458, the pixel was not printed to the front-end.

matticbot added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release Enhancement Changes to an existing feature — removing, adding, or changing parts of it AMP General Touches WP.com Files labels May 28, 2018

oskosk added [Status] Has Changelog and removed [Status] Needs Changelog labels May 28, 2018

oskosk added a commit that referenced this pull request May 28, 2018

Add #9458 to changelog

ae692d4

westonruter mentioned this pull request Feb 27, 2019

AMP compatibility issues #9730

Open

jeherve removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label Apr 27, 2021

		@@ -828,7 +830,7 @@ function sharing_display( $text = '', $echo = false ) {
		*
		* @param string $sharing_content Content markup of the Jetpack sharing links

Conversation

gravityrail commented May 1, 2018 • edited by oskosk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Legacy AMP

Canonical AMP

Screenshots

Legacy AMP

Canonical AMP

Changelog entry

Uh oh!

gravityrail commented May 15, 2018

Uh oh!

westonruter commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebinnion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gravityrail commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gravityrail commented May 1, 2018 •

edited by oskosk

Loading