Fix implementation of Infinite Scroll in AMP

[westonruter]

As reported in Automattic/newspack-theme#1108 (comment) and Automattic/newspack-theme#1107, the AMP implementation of Infinite Scroll is broken in Jetpack. For example, as also reported on an AMP support forum topic, in the Twenty Seventeen theme the footer shows the text %%footer%%.

The implementation appears to do more work than it needs to, by starting its own output buffering for the entire page when the AMP plugin is already doing this. Instead of doing search/replace on the output buffered document to make the necessary
changes, the better approach is to register an AMP plugin sanitizer which can make those changes on the DOM that is already being constructed from the output buffer by the AMP plugin.

This pull request is a start at fixing the implementation. I admit that I am not well-versed in Infinite Scroll or amp-next-page. The behavior currently in production for Twenty Twenty and Twenty Nineteen doesn't even seem to work as expected: I scroll down to the footer and then scroll back upward in order to start seeing the additional pages inserted. (This may have been due to not choosing the right setting for Infinite Scroll.)

Changes proposed in this Pull Request:

• Prevent %%footer%% from showing up in themes that don't support Infinite Scroll.
• Use an AMP sanitizer to inject the required markup for amp-next-page in the DOM, rather than creating a secondary output buffer and perform brittle string replacements.

Jetpack product discussion

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

Testing instructions:

• Go to '..'

Proposed changelog entry for your changes:

[jetpackbot]

	Fails
🚫	Please add these new PHP files to PHPCS required list in bin/phpcs-requirelist.js for automatic linting: modules/infinite-scroll/class-jetpack-amp-infinite-scroll-sanitizer.php

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

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 🤖

Generated by 🚫 dangerJS against 118e11a

[westonruter]

The behavior currently in production for Twenty Twenty and Twenty Nineteen doesn't even seem to work as expected: I scroll down to the footer and then scroll back upward in order to start seeing the additional pages inserted.

I think I determined what is wrong here, or at least the scenario that causes it to not work properly. I was testing with Twenty Nineteen and Twenty Twenty that was populated with the Theme Unit Test data. This means the footer widget areas were fully populated. The footer element is actually 4461px high for me, which is even larger than my testing viewport (1024px). When this happens, it appears that amp-next-page gets confused about when to start loading the next page. It appears that it starts to load the next page when the bottom of the footer is scrolled into view rather than once the top of the footer is scrolled into view.

cc @eduardogoncalves

[westonruter]

I tried to reproduce the issue on a standalone page but I was not able to. Even with a very tall footer that is 4x the viewport height it is loading as expected: https://amp-next-page-large-footer-test.glitch.me/long-footer-page-1.html

[westonruter]

I think I may actually have been at fault for misconfiguring Jetpack. I think I selected the “Load more posts in page with a button” option instead of “Load more posts as the reader scrolls down”.

[westonruter]

Squashing my 3 WIP commits into one.

[eduardogoncalves]

@westonruter For me the next page starts to load after amp-next-page-links hit above the middle of viewport height
https://youtu.be/Ls28teiPe4I

[westonruter]

Another report of this being a problem: https://wordpress.org/support/topic/footer-text-in-last-line-of-footer/

[pereirinha]

Thank you, @westonruter, for the PR.

I gave it a try and tested different possible scenarios, and in all, this is working great. The only issue I've found can be addressed by #17778, which I opened against this. Please, have a look at it.

In the early stages of addressing this feature, we had an internal discussion about going with output buffers vs parsing the DOM. There was no strong argument, so we ended up going with the output buffers. However, I see no problem going with the parsed DOM.

My other concern is about documentation around how themes can extend the amp_content_sanitizers filter to support this feature.

Pending your review on that PR, I think this is in a good position to be merged.

[pereirinha]

I tested this PR and is working great.

@westonruter do you plan to keep improving this PR — as it is flagged as draft —, or it's good to move with the A8C review?

[westonruter]

@westonruter do you plan to keep improving this PR — as it is flagged as draft —, or it's good to move with the A8C review?

I don't plan to continue improving the PR, no. I was hoping to just get it started so that you or someone else could take the baton to the finish line. I don't have enough experience with the Infinite Scroll module to know everything that needs to be accounted for.

[MuhammedSwalihct]

Any update on this? Seems like the issue still hasn't fixed.

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello westonruter! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D88770-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