Allow invalid AMP in Jetpack using a filter #16901

gravityrail · 2020-08-19T21:30:40Z

A few widgets, such as OpenTable, don't work in AMP due to their reliance on external javascript.

However, in order to transition to AMP standard mode, sites would like the option of reaping the performance benefits of AMP without necessarily having every page be 100% strict AMP.

This PR provides a filter, jetpack_allow_unsanitary_amp_content (better names suggestions welcome) which - when set to true - allows various widgets to work at the expense of 100% AMP compatibility.

It does this by setting the ampdevmode attribute on the html element and also any matching invalid assets.

Repaired widgets:

OpenTable
Pinterest
Eventbrite
Calendly

matticbot · 2020-08-19T21:30:45Z

Caution: This PR has changes that must be merged to WordPress.com
Hello gravityrail! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D48337-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

jetpackbot · 2020-08-19T21:55:14Z

	Fails
🚫	Please add these new PHP files to PHPCS required list in bin/phpcs-requirelist.js for automatic linting: 3rd-party/class-jetpack-amp-feature-assets-sanitizer.php

	Warnings
⚠️	"Testing instructions" are missing for this PR. Please add some
⚠️	"Proposed changelog entry" is missing for this PR. Please include any meaningful changes
⚠️	The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16901

Generated by 🚫 dangerJS against 369d732

westonruter · 2020-09-03T20:44:59Z

I'm going to review this.

westonruter

There's a key difficulty with attempting to use AMP Dev Mode for exempting certain elements from AMP validation. And that is the AMP pages in Dev Mode will always have one validation error:

Tag 'html' marked with attribute 'data-ampdevmode'. Validator will suppress errors regarding any other tag with this attribute.

When logged-in to WordPress and the admin bar is showing, Dev Mode is enabled in order to include the additional scripts on the page and intentionally not be a valid AMP page. Nevertheless, these AMP-invalid scripts are only present when the user is logged in: they won't show up when being crawled by Googlebot. If, however, you force AMP Dev Mode to be enabled then users will start seeing Google Search Console warnings for AMP validation errors.

If intending to use AMP Dev Mode in this way, we'd need to also provide the capability to omit the amp attribute from the html element. In this way, the page won't even be marked as AMP and so GSC won't try to validated it as an AMP page. This capability would make sense for a site in Standard mode.

For example, let's say that you force Dev Mode to be enabled via:

add_filter( 'amp_dev_mode_enabled', '__return_true' );

And then we introduce a new filter for whether the document should be marked as AMP, so you could turn it off:

add_filter( 'amp_dev_mode_document_marked', '__return_false' ); // Tentative naming.

This really should only be allowed on a Standard mode site, as if you do it on a paired mode site, crawlers will navigate to the amphtml links and complain about being linked to a non-AMP page.

Then on a Standard mode site, you'd have an HTML page with the AMP runtime but it would not be marked as AMP. Note that this usage of AMP is not yet official supported. It will only be really safe/supported to do this once Bento AMP ships, as otherwise the AMP runtime is not expecting for 3rd-party JS to be mutating the document.

Note that omitting the amp attribute from the html element is also the behavior of Standard mode sites that have excessive CSS: instead of stripping out the excessive CSS to make it a valid AMP page (which is the behavior in Transitional mode), in Standard mode the default behavior is to not strip it out and then to just mark the page as non-AMP. This is because in Standard mode there is no non-AMP version to go to if the AMP version is broken due to the removed styles.

westonruter · 2020-09-05T05:45:20Z

3rd-party/class-jetpack-amp-feature-assets-sanitizer.php

+	 * @since 1.3
+	 */
+	public function sanitize() {
+		$this->dom->documentElement->setAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, '' );


As opposed to setting this attribute manually, you can use the amp_dev_mode_enabled filter.

Nevertheless, this filter is probably not ideal either.

What is really needed is to conditionally add this attribute on the root html element if any element in the page had data-ampdevmode attribute added. This would mean AMP pages that don't have any such widgets on them would not need to enable AMP Dev Mode and could be fully valid AMP pages.

westonruter · 2020-09-05T05:46:07Z

3rd-party/class-jetpack-amp-feature-assets-sanitizer.php

+		$xpath = new DOMXPath( $this->dom );
+		foreach ( self::$asset_xpaths as $element_xpath ) {
+			foreach ( $xpath->query( $element_xpath ) as $node ) {
+				if ( $node instanceof DOMElement ) {
+					$node->setAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, '' );
+				}
+			}
+		}


Instead of manually setting these, you can use the amp_dev_mode_element_xpaths filter.

Or not, if the amp_dev_mode_enabled filter is not being used to force AMP Dev Mode to be enabled.

westonruter · 2020-09-05T06:06:22Z

At the moment when I try adding a Calendly block to a page, and I view the AMP page when being logged out, I see a validation error reported. This error will be reported by GSC unless the amp attribute is removed (which again will only make sense on a Standard mode site).

westonruter · 2020-09-06T08:46:42Z

@gravityrail I've opened a separate PR for what I believe is a better solution for making the OpenTable block AMP-compatible: #17085

westonruter · 2020-09-06T21:53:13Z

PR for AMP-compatibility for the Pinterest block: #17086.

gravityrail · 2020-09-08T21:15:18Z

Thanks for the contributions @westonruter !

westonruter · 2020-09-15T06:41:12Z

PR for Eventbrite block: #17164

gravityrail · 2020-09-16T17:45:19Z

@westonruter looks like we only need a solution for Calendly now - nice work! Any chance you'll be able to put something together for that service?

westonruter · 2020-09-20T19:44:56Z

looks like we only need a solution for Calendly now - nice work! Any chance you'll be able to put something together for that service?

See #17216.

Allow invalid AMP in Jetpack using a filter

8ebcd5f

matticbot added the Touches WP.com Files label Aug 19, 2020

gravityrail requested a review from jeherve August 19, 2020 21:30

gravityrail requested a review from kraftbj August 19, 2020 21:30

gravityrail added the [Status] In Progress label Aug 19, 2020

gravityrail added 2 commits August 19, 2020 14:35

Add pinterest support

b6b0ec8

Add eventbrite support

2ccd086

gravityrail added 2 commits August 19, 2020 16:23

Add calendly support

2692ec6

Merge branch 'master' into try/allow-embeds-in-amp

369d732

jeherve added the AMP label Aug 24, 2020

westonruter reviewed Sep 5, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

westonruter mentioned this pull request Sep 16, 2020

Prevent removal of amp-story that lacks mandatory attributes GoogleForCreators/web-stories-wp#4521

Merged

This was referenced Oct 29, 2020

feat: allow gam scripts in amp pages Automattic/newspack-plugin#688

Merged

Facilitate documents in AMP Dev Mode to be unmarked as AMP pages when site is in Standard mode ampproject/amp-wp#5549

Closed

gravityrail closed this Aug 2, 2021

kraftbj removed the [Status] In Progress label Oct 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow invalid AMP in Jetpack using a filter #16901

Allow invalid AMP in Jetpack using a filter #16901

gravityrail commented Aug 19, 2020 •

edited by jeherve

Loading

matticbot commented Aug 19, 2020

jetpackbot commented Aug 19, 2020 •

edited

Loading

westonruter commented Sep 3, 2020

westonruter left a comment •

edited

Loading

westonruter Sep 5, 2020

westonruter Sep 5, 2020

westonruter Sep 5, 2020

westonruter Sep 5, 2020

westonruter commented Sep 5, 2020 •

edited

Loading

westonruter commented Sep 6, 2020

westonruter commented Sep 6, 2020

gravityrail commented Sep 8, 2020

westonruter commented Sep 15, 2020

gravityrail commented Sep 16, 2020

This comment has been minimized.

westonruter commented Sep 20, 2020

Allow invalid AMP in Jetpack using a filter #16901

Allow invalid AMP in Jetpack using a filter #16901

Conversation

gravityrail commented Aug 19, 2020 • edited by jeherve Loading

matticbot commented Aug 19, 2020

jetpackbot commented Aug 19, 2020 • edited Loading

westonruter commented Sep 3, 2020

westonruter left a comment • edited Loading

Choose a reason for hiding this comment

westonruter Sep 5, 2020

Choose a reason for hiding this comment

westonruter Sep 5, 2020

Choose a reason for hiding this comment

westonruter Sep 5, 2020

Choose a reason for hiding this comment

westonruter Sep 5, 2020

Choose a reason for hiding this comment

westonruter commented Sep 5, 2020 • edited Loading

westonruter commented Sep 6, 2020

westonruter commented Sep 6, 2020

gravityrail commented Sep 8, 2020

westonruter commented Sep 15, 2020

gravityrail commented Sep 16, 2020

This comment has been minimized.

westonruter commented Sep 20, 2020

gravityrail commented Aug 19, 2020 •

edited by jeherve

Loading

jetpackbot commented Aug 19, 2020 •

edited

Loading

westonruter left a comment •

edited

Loading

westonruter commented Sep 5, 2020 •

edited

Loading