Infinite scroll: rewrite from jQuery to vanilla JS #15017

dero · 2020-03-17T16:24:32Z

Changes proposed in this Pull Request:

Remove jQuery as a dependency for infinite-scroll.js.

This is a large change, please see comments inline of this PR for additional detail on how certain features continue to be supported in vanilla JS.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This is an update to the infinite-scroll module.

Testing instructions:

Simple IS (button trigger):

Make sure there is enough posts on the site to trigger pagination on the home page.
Go to Jetpack Settings > Writing > Infinite Scroll.
Enable Load more posts in page with a button.
On the front-end, make sure the Older Posts button is rendered.
Click the button and observe the loader animation being rendered.
Observe new posts being injected.
Click the button enough times to exhaust available posts.
Observe the Older Posts button not being rendered.
Scroll up and observe the URL being changed to represent the current page.

Simple IS (automatic trigger):

Make sure there is enough posts on the site to trigger pagination on the home page.
Go to Jetpack Settings > Writing > Infinite Scroll.
Enable Load more posts as the reader scrolls down.
On the front-end, scroll down and observe new posts being injected.
Continue scrolling down to exhaust available posts.
Observe that no additional content is being pulled in once all posts are displayed.
Scroll up and observe the URL being changed to represent the current page.

IS with audio / video shortcodes:

There is a good deal of code dedicated to making sure the [audio] and [video] shortcodes still work when infinite scroll is enabled. These shortcodes use the MediaElementJS from WP core to normalize media playback experience and that library is finicky and requires a lot of special treatment. The MediaElement library depends on jQuery and IS needs to be able to dynamically inject MediaElement and jQuery scripts lazily only when required.

Add an [audio] and / or [video] shortcodes to a post that will appear on a second page of the homepage, i.e. that post should be loaded by IS.
Make sure jQuery and wp-mediaelement scripts are not loaded on the initial page load.
Let IS load the second page.
Make sure jQuery and wp-mediaelement scripts are now loaded and executed dynamically.
Observe that the shortcode is rendered correctly using the MediaElement library (to check this, observe DOM elements and search for mejs-container classes).
Make sure the media player works as expected.

IS (customizer):

Make sure the IS functions in the Customizer.

Tested in:

Chrome 80, Ubuntu 19
IE 11, Windows 10

Proposed changelog entry for your changes:

The Infinite Scroll module has been rewritten to not depend on jQuery.

jetpackbot · 2020-03-17T16:26:52Z

	Warnings
⚠️	`pre-commit` hook was skipped for one or more commits
⚠️	The PR is missing at least one `[Status]` label. Suggestions: `[Status] In Progress`, `[Status] Needs Review`

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 bf701b4

dero · 2020-03-17T16:35:54Z

modules/infinite-scroll.php

@@ -215,23 +215,6 @@ public function action_wp_enqueue_scripts() {
 		if ( Jetpack::is_module_active( 'tiled-gallery' ) ) {
 			Jetpack_Tiled_Gallery::default_scripts_and_styles();
 		}
-
-		// Core's Audio and Video Shortcodes


Currently, IS loads wp-mediaelement eagerly on all pages, which is inefficient. Also, the wp-mediaelement script depends on jquery, which makes matters even worse.

WordPress will enqueue wp-mediaelement when a media element that requires it (e.g. [audio]) appears on any particular page. Infinite scroll is responsible for loading these assets only when it injects any media elements on subsequent pages. This and the related fixes (1, 2) accomplish just that.

dero · 2020-03-17T16:37:44Z

modules/infinite-scroll/infinity-customizer.js

@@ -0,0 +1,54 @@
+/* globals wp */


I have moved all purely Customizer logic into its own file and retained its coupling to jQuery, because the aim of this PR is to rewrite front-end code to vanilla JS.

modules/infinite-scroll/infinity.css

dero · 2020-03-17T16:55:26Z

modules/infinite-scroll/infinity.js

 		this.click_handle = settings.click_handle;
 		this.google_analytics = settings.google_analytics;
 		this.history = settings.history;
 		this.origURL = window.location.href;
-		this.pageCache = {};


Blocker: Removing page caching is a major change, which needs to be further analyzed and discussed.

I've consolidated all changes to pageCache to a single commit to make is simple for us to revert them if necessary. Please read the commit message, it contains more details about why this change is being made.

While I understand the reason for implementing the cache the way it's implemented right now, I wasn't able to verify it's actually efficient. Removed nodes from the invisible "pages" from DOM currently don't seem to be garbage collected. I've profiled the memory using Chrome Dev tools and the number of Nodes in memory stayed constant even as they were removed from DOM. I'm not sure why that is, these are my two theories:

My machine has enough memory and doesn't need to GC aggressively.

The Nodes remain to be referenced somewhere even after they are removed (likely in jQuery internally).

I'll try to construct a more representative test page with a lot of dynamically loaded content and try to verify whether memory can actually be freed by dynamically removing Nodes like this.

dero · 2020-03-17T17:01:50Z

modules/infinite-scroll/infinity.js

+	/**
+	 * Recursively convert a JS object into URL encoded data.
+	 */
+	Scroller.prototype.urlEncodeJSON = function( obj, prefix ) {


Note: Usually we could just POST with Content-Type: application/json, but that would send the JSON string in the body of the request and we would need to rewrite much of infinity.php to adapt to that.

Instead we URI encode the data object, so that we can send the parameters as application/x-www-form-urlencoded, which is correctly and automatically decoded by PHP and the $_POST ($_REQUEST) superglobals are correctly populated.

dero · 2020-03-17T17:04:55Z

modules/infinite-scroll/infinity.js

-		jqxhr.done( function( response ) {
+		xhr.onload = function() {
+			var response = JSON.parse( xhr.responseText ),
+				httpCheck = xhr.status >= 200 && xhr.status < 300,


Note: It's fine to check for 2xx response codes only. All redirects are followed by xhr and should never be exposed to us.

modules/infinite-scroll/infinity.css

jeherve · 2020-03-18T18:47:06Z

modules/infinite-scroll/infinity.css

+  0% { opacity: 1 }
+  100% { opacity: 0 }
+}
+.infinite-loader .spinner-inner div {


Could you switch to tabs instead of spaces in this file? This way this will ensure that the next person making a change in that file does not inadvertently make changes on all lines on save.

jeherve · 2020-03-18T18:49:32Z

modules/infinite-scroll/infinity.php

-			array( 'jquery' ),
-			'4.0.0',
+			array(),
+			'8.4.0',


We can switch to using JETPACK__VERSION here and below maybe?

zinigor

This is amazing, thank you! I have tested the code, it seems to be working fine with both button and scroll triggering. I have made two comments in the code, but there's nothing major.

zinigor · 2020-03-18T20:43:13Z

modules/infinite-scroll/infinity.js

@@ -517,16 +663,16 @@
 			self.ready = true;
 			self.refresh();
 		} else {
-			self.body.unbind( 'post-load', self.checkViewportOnLoad );
+			self.body.addEventListener( 'post-load', self.checkViewportOnLoadBound );


I'm not sure of the logic here, but should this be removeEventListener instead?

zinigor · 2020-03-18T20:48:03Z

modules/infinite-scroll/infinity.js

@@ -596,33 +746,7 @@
 			} else {
 				setsHidden.push( { id: id, top: setTop, bottom: setBottom, pageNum: setPageNum } );


I'm not seeing setsHidden being used anywhere in this method's scope after the array is populated, aside from the removed block where it was stored in the page cache. Maybe we can remove the array population as well?

dero · 2020-03-19T14:38:50Z

Thank you for your reviews @jeherve and @zinigor, I've implemented the suggested changes.

dero · 2020-03-19T14:52:59Z

modules/infinite-scroll/infinity.js

+		this.body.addEventListener( 'post-load', self.initializeMejs );
+	};
+
+	Scroller.prototype.triggerEvent = function( eventName, el, data ) {


Blocker: Events need to be triggered in a backwards-compatible fashion. See this commit for sample implementation.

Resolved in fda8991.

zinigor

Thank you for the fixes, looks good to me now!

dero · 2020-03-25T16:49:39Z

@jeherve Assigning to you to re-review, please.

Also, question: who is usually the assignee of a PR on the Jetpack project? Is it the person responsible for the "next step" or is always the PR author? I would like to be respectful of the process here, but I'm not sure what it is.

kraftbj · 2020-03-27T16:07:26Z

r204969-wpcom

kraftbj · 2020-03-27T16:59:44Z

@dero Found an interesting situation.

@sergeymitr spotted that if you scroll down using the keyboard (down button), new posts are inserted pretty quickly to the point of loading ~10 pages worth of posts before scrolling through the initial page load.

See https://cloudup.com/c9xX-6_P3dt

I'm okay merging this PR if you're able to debug that in a follow-up PR in the next couple of days.

dero · 2020-03-27T17:35:33Z

@kraftbj Sure thing, I can debug this tomorrow and (if needed) on Monday.

Do you know if this happens in all browsers? I wasn't able to verify the issue in Chrome, but the posts on my test site are displayed with full content, which is different from your test. I can spend more time on the issue tomorrow.

kraftbj · 2020-03-27T17:38:34Z

Sergey kept digging:

So here's the summary:
MacOS Firefox: scroll works, keyboard arrows don't
Windows Firefox: both scroll and keyboard arrows cause issues
Windows IE11: both scroll and keyboard arrows cause issues
We could ask somebody else with a windows machine test it in Firefox, to make sure it's not specific to my machine.

dero · 2020-03-27T17:40:05Z

@kraftbj Thank you for this additional detail.

I have a Windows machine and can test it out - but not before tomorrow. If you have more detail before then, I'd be happy if you could post it.

dero · 2020-03-27T18:03:05Z

@kraftbj Update: I can reproduce in Chrome on Ubuntu, but only on twentyseventeen so far. I'll investigate if other themes are affected (e.g. twentytwenty tweaked to allow for scroll loading behaves correctly).

dero · 2020-03-27T18:19:42Z

@kraftbj Last update tonight: I'm seeing the same issue on the existing master. Could you please confirm on your end?

It seems that the container element in twentyseventeen has no outerHeight (because the children are all floating elements), which makes the infinity scroller fetch an additional page on every little scroll. We can fix that by searching for maxims of the children bounds.

I'm going to return to this ticket tomorrow.

kraftbj · 2020-03-27T19:59:12Z

@dero Nice find. I'm going to go ahead and merge this (since you're right, this is happening on master too; I didn't catch that). A quick PR tomorrow/Monday for that will be easy to get in before the EOD Monday code freeze for 8.4.

I don't want to hold this up from landing in 8.4 if something happens and we can't address twentyseventeen in time for freeze.

kraftbj · 2020-03-27T20:02:53Z

Opened #15155 for tracking.

* Initial changelog entry * Changelog: add #14904 * Changelog: add #14910 * Changelog: add #14913 * Changelog: add #14916 * Changelog: add #14922 * Changelog: add #14924 * Changelog: add #14925 * Changelog: add #14928 * Changelog: add #14840 * Changelog: add #14841 * Changelog: add #14842 * Changelog: add #14826 * Changelog: add #14835 * Changelog: add #14859 * Changelog: add #14884 * Changelog: add #14888 * Changelog: add #14817 * Changelog: add #14814 * Changelog: add #14819 * Changelog;: add #14797 * Changelog: add #14798 * Changelog: add #14802 * Changelog: add #13676 * Changelog: add #13744 * Changelog: add #13777 * Changelog: add #14446 * Changelog: add #14739 * Changelog: add #14770 * Changelog: add #14784 * Changelog: add #14897 * Changelog: add #14898 * Changelog: add #14968 * Changelog: add #14985 * Changelog: add #15044 * Changelog: add #15052 * Update to remove Podcast since it remains in Beta * Changelog: add #14803 * Changelog: add #15028 * Changelog: add #15065 * Changelog:add #14886 * Changelog: add #15118 * Changelog: add #14990 * Changelog: add #14528 * Changelog: add #15120 * Changelog: add #15126 * Changelog: add #15049 * Chanegelog: add #14852 * Changelog: add #15090 * Changelog: add #15138 * Changelog: add #15124 * Changelog:add #15055 * Changelog: add #15017 * Changelog: add #15109 * Changelog: add #15145 * Changelog:add #15096 * Changelog:add #15153 * Changelog: add #15133 * Changelog: add #14960 * Changelog: add #15127 * Changelog: add #15056 * Copy current changelog to changelog archive. * Clarify changelog description

dero requested a review from a team March 17, 2020 16:24

matticbot added the Touches WP.com Files label Mar 17, 2020

dero added [Status] Needs Review This PR is ready for review. [Feature] Infinite Scroll labels Mar 17, 2020

dero commented Mar 17, 2020

View reviewed changes

modules/infinite-scroll/infinity.css Show resolved Hide resolved

dero commented Mar 17, 2020

View reviewed changes

jeherve added this to the 8.4 milestone Mar 18, 2020

jeherve previously requested changes Mar 18, 2020

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Focus] Performance and removed [Status] Needs Review This PR is ready for review. labels Mar 18, 2020

zinigor requested changes Mar 18, 2020

View reviewed changes

dero added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 19, 2020

dero commented Mar 19, 2020

View reviewed changes

brbrr assigned dero Mar 19, 2020

zinigor previously approved these changes Mar 24, 2020

View reviewed changes

dero assigned jeherve and unassigned dero Mar 25, 2020

kraftbj approved these changes Mar 27, 2020

View reviewed changes

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Tested on WP.com labels Mar 27, 2020

kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 27, 2020

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 27, 2020

kraftbj merged commit 360fb37 into master Mar 27, 2020

kraftbj deleted the updates/infinity-vanilla-js branch March 27, 2020 19:59

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 27, 2020

kraftbj mentioned this pull request Mar 27, 2020

Infinite Scroll + Twenty Seventeen: Over-aggressive polling of additional posts. #15155

Closed

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Mar 31, 2020

jeherve added a commit that referenced this pull request Mar 31, 2020

Changelog: add #15017

54f68cd

This was referenced Apr 7, 2020

Infinite Scroll: broken when footer is set to false #15348

Closed

Infinite Scroll: some posts are not loaded #15352

Closed

jeherve mentioned this pull request May 19, 2020

Infinite Scroll: No longer treated as an ajax request #15837

Closed

jeherve mentioned this pull request Sep 9, 2020

Infinite scroll: on scrolled page, browser "back" button creates offset issues #17108

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infinite scroll: rewrite from jQuery to vanilla JS #15017

Infinite scroll: rewrite from jQuery to vanilla JS #15017

dero commented Mar 17, 2020

jetpackbot commented Mar 17, 2020 •

edited

Loading

dero Mar 17, 2020

dero Mar 17, 2020

dero Mar 17, 2020

dero Mar 17, 2020

dero Mar 17, 2020

jeherve Mar 18, 2020

jeherve Mar 18, 2020

zinigor left a comment

zinigor Mar 18, 2020

zinigor Mar 18, 2020

dero commented Mar 19, 2020

dero Mar 19, 2020

dero Mar 19, 2020

zinigor left a comment

dero commented Mar 25, 2020

kraftbj commented Mar 27, 2020

kraftbj commented Mar 27, 2020

dero commented Mar 27, 2020

kraftbj commented Mar 27, 2020

dero commented Mar 27, 2020

dero commented Mar 27, 2020

dero commented Mar 27, 2020

kraftbj commented Mar 27, 2020

kraftbj commented Mar 27, 2020

		@@ -596,33 +746,7 @@
		} else {
		setsHidden.push( { id: id, top: setTop, bottom: setBottom, pageNum: setPageNum } );

Infinite scroll: rewrite from jQuery to vanilla JS #15017

Infinite scroll: rewrite from jQuery to vanilla JS #15017

Conversation

dero commented Mar 17, 2020

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Tested in:

Proposed changelog entry for your changes:

jetpackbot commented Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinigor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dero commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinigor left a comment

Choose a reason for hiding this comment

dero commented Mar 25, 2020

kraftbj commented Mar 27, 2020

kraftbj commented Mar 27, 2020

dero commented Mar 27, 2020

kraftbj commented Mar 27, 2020

dero commented Mar 27, 2020

dero commented Mar 27, 2020

dero commented Mar 27, 2020

kraftbj commented Mar 27, 2020

kraftbj commented Mar 27, 2020

jetpackbot commented Mar 17, 2020 •

edited

Loading