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

Prevent adding adding AMP hooks for Infinite Scroll on legacy AMP post templates #17175

Merged
merged 2 commits into from
Sep 17, 2020

Conversation

westonruter
Copy link
Contributor

Fixes a regression introduced in #16048. See #16048 (comment):

This appears to not be working properly. AMP validation errors are being emitted on this AMP page, for example: https://alankent.me/2020/06/12/musings-about-5g-and-ar/amp/

The problem seems to be twofold:

  1. The <amp-next-page> element is erroneously getting added when in legacy Reader mode.
  2. The amp-next-page script is not getting injected because output buffer is being used to inject the tag and/or it's not getting detected during validation because WordPress.com is still on 1.4.x and in that old version the page is not sent through the post-processor. See Pass entire Reader mode templates through post-processor ampproject/amp-wp#3781.

The validation errors currently occurring on AMP pages on WordPress.com using the Reader mode template when infinite scroll is enabled:

The tag 'amp-next-page' requires including the 'amp-next-page' extension JavaScript. line 1260, column 0 The mandatory reference point 'AMP-NEXT-PAGE > SCRIPT[type=application/json]' for 'amp-next-page with inline config' is missing. line 1260, column 0 The tag 'script' requires including the 'amp-next-page' extension JavaScript. line 1261, column 1 The tag 'template' requires including the 'amp-mustache' extension JavaScript. line 1268, column 2

Changes proposed in this Pull Request:

  • Prevent initializing AMP infinite scroll on legacy Reader post templates.

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

  1. Active AMP plugin
  2. Set the template mode to "Reader"
  3. Select the AMP Legacy reader theme
  4. Enable Infinite Scroll in Jetpack,
  5. View an AMP page.
  6. See validation errors (on AMP plugin v1.4.x) or else unexpected amp-next-page (on AMP plugin v1.5+).

Proposed changelog entry for your changes:

  • Fix infinite scroll fro erroneously being added on AMP Reader mode post templates.

@jetpackbot
Copy link

jetpackbot commented Sep 15, 2020

Scheduled Jetpack release: October 6, 2020.
Scheduled code freeze: September 29, 2020

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

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.

Generated by 🚫 dangerJS against 4d0627a

@jeherve jeherve added [Pri] High [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended AMP [Feature] Infinite Scroll labels Sep 16, 2020
@jeherve jeherve added this to the 9.0 milestone Sep 16, 2020
@jeherve
Copy link
Member

jeherve commented Sep 16, 2020

Thanks for the report! @pereirinha Do you think you could take a look? Thanks!

jeherve
jeherve previously approved these changes Sep 16, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well on my end.

@jeherve
Copy link
Member

jeherve commented Sep 16, 2020

Internal reference: D49673-code -- r213721-wpcom

@westonruter
Copy link
Contributor Author

@jeherve I just found that this also causes a problem with Web Stories. It is causing the <amp-next-page> element to be injected on pages that should only have <amp-story>. I'm going to amend this PR with that fix.

@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Sep 16, 2020
@westonruter
Copy link
Contributor Author

westonruter commented Sep 16, 2020

Done. When viewing a singular Web Story:

Before

</amp-story><amp-next-page max-pages="0">
	<script type="application/json">
		[
			{"title":"","url":"","image":""}		]
	</script>
	<div separator>
		<hr class="post-separator styled-separator is-style-wide section-inner" aria-hidden="true">
		</div>
	<div recommendation-box class="recommendation-box">
		<template type="amp-mustache">
			{{#pages}}
			<div id="infinite-handle" class="read-more-button-wrap">
	<span>
		<a href="{{url}}" class="more-link" rel="amphtml">
			<span class="faux-button">
				Older posts			</span>
		</a>
	</span>
</div>
				{{/pages}}
		</template>
	</div>
	<div footer>
		
	</div>
</amp-next-page>
		</body></html>

After

</amp-story></body></html>

cc @swissspidy

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still tests well for me, nice work! 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress labels Sep 17, 2020
@jeherve jeherve merged commit ad8ec79 into Automattic:master Sep 17, 2020
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 17, 2020
@jeherve
Copy link
Member

jeherve commented Sep 17, 2020

r213776-wpcom

@pereirinha
Copy link
Contributor

Thank you @westonruter and @jeherve.
It is also working fine for me in the mentioned scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants