Lazy Images: Drop jQuery, convert to vanilla JS #14886

dero · 2020-03-04T20:55:12Z

Rewrites the lazy-images module to not depend on jQuery.

Fixes #14863

Changes proposed in this Pull Request:

Removed jQuery calls and rewritten the code in vanilla JS.

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

This is an improvement to the lazy-images module.

Testing instructions:

Make sure to test on a theme that doesn't pull jQuery in by default. (Tested on twentytwenty locally.)
Go to Jetpack > Settings > Performance.
Enable Lazy Loading for images.
Make sure no other Jetpack features are enabled.
Create a post with a couple of images.
Visit the post.
Observe there is no jQuery being added to the page.
Observe the images continue to be loaded lazily as before.

Proposed changelog entry for your changes:

Rewritten the jQuery logic in lazy-images in vanilla JS.

jetpackbot · 2020-03-04T20:57:22Z

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.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 485b0cc

dero · 2020-03-04T21:02:25Z

modules/lazy-images/js/lazy-images.js

-		theClone = theImage.clone(true);
+		srcset = image.getAttribute( 'data-lazy-srcset' );
+		sizes = image.getAttribute( 'data-lazy-sizes' );
+		imageClone = image.cloneNode();


Note: This is a potential regression.

The jQuery clone method would clone all custom event handlers hooked directly onto the cloned element, whereas cloneNode doesn't do it and it's generally not possible to clone event listeners. The only viable option that follows the existing pattern is to redefine (or rather wrap) the native Node.addEventListener method and keep a track of event listeners this way.

Another solution could be processing existing img elements in-place instead of swapping it with their clones. Direction welcome.

I was curious as to why the image is cloned in the first place and found these:

Lazy Images: Add jetpack-lazy-loaded-image event #9451

Lazy Images: Breaks outdenting of big images in compatible themes #8879

I suggest we take a closer look at the originally reported issue and verify that cloning is the correct solution, because not cloning might open interesting opportunities for improvement:

It would mean no event listeners would be lost on lazy loaded images.

It would allow more advanced placeholders to be inlined instead of 1×1 GIFs, e. g. placeholder SVGs with dimensions identical to the source image. That could solve the issue with page jumps when the image is being loaded / placeholder being replaced, because the image element would not get swapped in the DOM at all.

The latter (arguably very annoying) UX issue is currently pronounced due to the Gutenberg Image Block not outputting the width and height image attributes, which breaks the browsers' ability to layout the image ahead of time.

@dero I've also tested and it looks good to me.

I was just wondering if there is anyone else we should do here (maybe open another issue?), or should we ok to go?

dero · 2020-03-04T21:12:49Z

modules/lazy-images/js/lazy-images.js

+		try {
+			lazyLoadedImageEvent = new Event( 'jetpack-lazy-loaded-image', {
+				bubbles: true,
+				cancelable: true
+			} );
+		} catch ( e ) {
+			lazyLoadedImageEvent = document.createEvent( 'Event' )
+			lazyLoadedImageEvent.initEvent( 'jetpack-lazy-loaded-image', true, true );
+		}
+
+		imageClone.dispatchEvent( lazyLoadedImageEvent );


Note: This is a potential regression.

The original implementation uses the jQuery.trigger method which fires custom jQuery events and unifies event data into jQuery Event objects. Such events are proprietary to jQuery and can't be observed using native APIs, e.g. addEventListener.

The issue here is that any 3rd party code relying on jetpack-lazy-loaded-image being a proprietary jQuery event might not work after this change, because the native event object is different.

Ah, interesting.

This in Jetpack could be edited to use the non-jQuery event:

jetpack/modules/theme-tools/compat/twentysixteen.php

Line 63 in 1115c95

"jQuery( document.body ).on( 'jetpack-lazy-loaded-image', function () { jQuery( window ).trigger( 'resize' ); } );"

...but of course the question of 3rd-party code remains.

@kienstra Looked into differences between the native an jQuery event system. Shouldn't be an issue after all. Could you please review my reasoning in the comment above https://github.com/Automattic/jetpack/pull/14886/files#r390905356?

dero · 2020-03-06T16:12:47Z

@kienstra Could you please review this when you have a moment?

dero · 2020-03-09T18:47:34Z

modules/lazy-images/js/lazy-images.js

-		$( 'body' ).bind( 'post-load', lazy_load_init );
+		bodyEl.addEventListener( 'post-load', lazy_load_init );

 		// Add event to provide optional compatibility for other code.
-		$( 'body' ).bind( 'jetpack-lazy-images-load', lazy_load_init );
-	} );
+		bodyEl.addEventListener( 'jetpack-lazy-images-load', lazy_load_init );


Note: This is a potential regression.

I just realized these are jQuery events and we can't simply hook into those using addEventListener. We'll probably need to dispatch regular events instead of using jQuery.trigger in code that actually creates these events. I'll need to look into this a bit more.

This has been addressed in cff56d8 and the solution is explained in a comment above https://github.com/Automattic/jetpack/pull/14886/files#r390905356

josephscott · 2020-03-09T22:28:35Z

This is great, I've been trying out the patch and my tests ( mobile device emulation, slow network conditions ) are showing solid improvements in start render.

kienstra

Looks Good

Hi @dero,
This looks good, and lazy-loading still works as expected 😄

It's approved, pending your points below.

kienstra · 2020-03-10T04:39:52Z

modules/lazy-images/js/lazy-images.js

-		sizes = theImage.attr( 'data-lazy-sizes' );
-		theClone = theImage.clone(true);
+		srcset = image.getAttribute( 'data-lazy-srcset' );
+		sizes = image.getAttribute( 'data-lazy-sizes' );


The lazy-loading functionality works well:

kienstra · 2020-03-10T04:54:00Z

modules/lazy-images/lazy-images.php

@@ -351,7 +351,7 @@ public function enqueue_assets() {
 				'_inc/build/lazy-images/js/lazy-images.min.js',
 				'modules/lazy-images/js/lazy-images.js'
 			),
-			array( 'jquery' ),
+			array(),


Nice, jquery doesn't appear when using a theme without it, like Twenty Nineteen:

kienstra · 2020-03-10T05:16:08Z

modules/lazy-images/js/lazy-images.js

+		try {
+			lazyLoadedImageEvent = new Event( 'jetpack-lazy-loaded-image', {
+				bubbles: true,
+				cancelable: true
+			} );
+		} catch ( e ) {
+			lazyLoadedImageEvent = document.createEvent( 'Event' )
+			lazyLoadedImageEvent.initEvent( 'jetpack-lazy-loaded-image', true, true );
+		}
+
+		imageClone.dispatchEvent( lazyLoadedImageEvent );


Ah, interesting.

This in Jetpack could be edited to use the non-jQuery event:

jetpack/modules/theme-tools/compat/twentysixteen.php

Line 63 in 1115c95

"jQuery( document.body ).on( 'jetpack-lazy-loaded-image', function () { jQuery( window ).trigger( 'resize' ); } );"

...but of course the question of 3rd-party code remains.

kienstra · 2020-03-10T05:23:45Z

modules/lazy-images/js/lazy-images.js


-		if ( ! theImage.length ) {
+		if ( ! image instanceof HTMLImageElement ) {


dero · 2020-03-11T11:25:22Z

3rd-party/woocommerce.php

+				jetpackLazyImagesLoadEvent = document.createEvent( 'Event' )
+				jetpackLazyImagesLoadEvent.initEvent( 'jetpack-lazy-images-load', true, true );
+			}
+			jQuery( 'body' ).get( 0 ).dispatchEvent( jetpackLazyImagesLoadEvent );


Note: ~~While it may feel like it, switching from jQuery trigger to dispatching native events does not introduce a regression. There are two notable differences in the usage of both:~~

~~jQuery trigger creates proprietary events that are only observable using jQuery (on, bind etc.). Meaning that there is no 3rd party code out there using addEventListener to observe them.~~

The native dispatchEvent is observable using both addEventListener and jQuery methods. Importantly, jQuery will normalize the incoming event and expose any additional detail data in a jQuery way.

~~This means any pre-existing handler is not going to be affected by a change from the proprietary trigger to native dispatchEvent.~~ 🎉

All of the above is incorrect. See #14886 (comment)

It seems the issue reported by @jeherve disproves the above. It now seems jQuery listeners (on, bind) do not pass the detail data as a second parameter to event handlers after all. I've however tested that behavior before and I'm not certain why I'm seeing a different outcome now. I will revisit this issue tomorrow.

roccotripaldi

This tested well!

matticbot · 2020-03-17T13:52:05Z

Caution: This PR has changes that must be merged to WordPress.com
Hello dero! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D40433-code before merging this PR. Thank you!

jeherve

I am getting the following JS error on my end when triggering Infinite Scroll with a click:

Uncaught TypeError: Cannot read property 'html' of undefined
infinity.js?m=1584556613h&ver=4.0.0:455

dero · 2020-03-19T12:41:26Z

I am getting the following JS error on my end

@jeherve The issue should be fixed by 3bf681a.

The reason for introducing a new is.post-load event is that jQuery events and native JS events are not compatible when we need to attach data to the event object.

jQuery passes data as a second parameter: function listener( event, data ).
Native events contain the data in event.detail: function listener( { detail } ).

While jQuery observes native events, it doesn't normalize the data, i.e. won't pass the event.detail value as a second parameter to the listener methods.

This is not a problem for any code in Jetpack, because we can rewrite all the listeners appropriately. It is, however, a problem for any 3rd party code that currently uses jQuery to listen to the events - such integrations would not receive the data parameter out of the sudden, which is a significant regression. Especially considering that the post-load event is explicitly described in the official documentation.

I've considered a few approaches towards resolving this incompatibility:

Solution 1: Emit both the native and the jQuery events.

if ( jQuery ) {
  jQuery( el ).trigger( eventName, data );
}
el.dispatchEvent( evt ); // evt created by new CustomEvent

Native event listeners are fine, but all jQuery listeners fire twice. Once with the data passed as the second parameter and once without. This breaks backwards compatibility.

Solution 2: Emit jQuery events when jQuery is available, native events when it's not.

if ( jQuery ) {
  jQuery( el ).trigger( eventName, data );
} else {
  el.dispatchEvent( evt ); // evt created by new CustomEvent
}

This potentially breaks new integrations, which may begin to use addEventListener to hook into the event. This would work until jQuery is added to the page for any reason. Then those addEventListeners would stop working.

Final solution: Emit jQuery events when jQuery is available and also fire new native events.

When jQuery is on the page, we should fire all events (containing data) like before. In addition, we should fire a corresponding native event with a different name. That allows Jetpack's listeners to hook into it in vanilla JS and it also allows 3rd party developers to refactor their code to use the new native event instead of depending on jQuery.

Any feedback and suggestions welcome.

roccotripaldi

I like your proposed solution. I tried this on a jquery and non-jquery theme, and had no issues and did not see any console errors.

No errors.

matticbot · 2020-03-24T21:08:24Z

dero, Your synced wpcom patch D40433-code has been updated.

jeherve

This seems to work well in my tests. Merging.

jeherve · 2020-03-25T17:10:21Z

r204798-wpcom

…/jetpack#14886)

* 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

Lazy Images: Drop jQuery, convert to vanilla JS

81c1260

dero added [Focus] Performance [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Feature] Lazy Images labels Mar 4, 2020

dero requested a review from a team as a code owner March 4, 2020 20:55

dero commented Mar 4, 2020

View reviewed changes

dero mentioned this pull request Mar 5, 2020

Lazy Images: Update to not require jQuery #14863

Closed

jeherve added this to the 8.4 milestone Mar 5, 2020

jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Mar 5, 2020

dero commented Mar 9, 2020

View reviewed changes

kienstra previously approved these changes Mar 10, 2020

View reviewed changes

Lazy Images: Use the native events system

cff56d8

dero dismissed kienstra’s stale review via cff56d8 March 11, 2020 11:13

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

dero commented Mar 11, 2020

View reviewed changes

roccotripaldi self-assigned this Mar 17, 2020

roccotripaldi previously approved these changes Mar 17, 2020

View reviewed changes

roccotripaldi added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 17, 2020

leogermani added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Mar 17, 2020

roccotripaldi added the [Status] Tested on WP.com label Mar 17, 2020

Lazy Images: Replace the image inline, don't clone

16e6bfc

dero dismissed roccotripaldi’s stale review via 16e6bfc March 18, 2020 15:25

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 18, 2020

jeherve previously requested changes Mar 18, 2020

View reviewed changes

dero dismissed leogermani’s stale review via 9689cad March 19, 2020 12:10

Lazy Images: Back-compat for events

3bf681a

dero force-pushed the update/lazy-images-vanilla-js branch from 9689cad to 3bf681a Compare March 19, 2020 12:13

dero mentioned this pull request Mar 19, 2020

Infinite scroll: rewrite from jQuery to vanilla JS #15017

Merged

Lazy Images: Fix the jQuery check

485b0cc

jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 19, 2020

roccotripaldi approved these changes Mar 20, 2020

View reviewed changes

kraftbj requested a review from jeherve March 24, 2020 19:36

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 24, 2020

jeherve approved these changes Mar 25, 2020

View reviewed changes

jeherve merged commit 85e65ca into master Mar 25, 2020

jeherve deleted the update/lazy-images-vanilla-js branch March 25, 2020 16:58

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

nicokaiser added a commit to nicokaiser/lazy-images-without-jetpack that referenced this pull request Mar 31, 2020

backport: Lazy Images: Drop jQuery, convert to vanilla JS (Automattic…

0521f1b

…/jetpack#14886)

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 #14886

bc9df73

jeherve mentioned this pull request Apr 7, 2020

Infinite Scroll: some posts are not loaded #15352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy Images: Drop jQuery, convert to vanilla JS #14886

Lazy Images: Drop jQuery, convert to vanilla JS #14886

dero commented Mar 4, 2020

jetpackbot commented Mar 4, 2020 •

edited

Loading

dero Mar 4, 2020

dero Mar 4, 2020

leogermani Mar 17, 2020

dero Mar 4, 2020

kienstra Mar 10, 2020

dero Mar 11, 2020

dero commented Mar 6, 2020

dero Mar 9, 2020

dero Mar 11, 2020

josephscott commented Mar 9, 2020

kienstra left a comment •

edited

Loading

kienstra Mar 10, 2020

kienstra Mar 10, 2020

kienstra Mar 10, 2020

kienstra Mar 10, 2020

dero Mar 11, 2020 •

edited

Loading

dero Mar 18, 2020

roccotripaldi left a comment

matticbot commented Mar 17, 2020

jeherve left a comment

dero commented Mar 19, 2020

roccotripaldi left a comment

matticbot commented Mar 24, 2020

jeherve left a comment

jeherve commented Mar 25, 2020


		if ( ! theImage.length ) {
		if ( ! image instanceof HTMLImageElement ) {

Lazy Images: Drop jQuery, convert to vanilla JS #14886

Lazy Images: Drop jQuery, convert to vanilla JS #14886

Conversation

dero commented Mar 4, 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:

Proposed changelog entry for your changes:

jetpackbot commented Mar 4, 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

dero commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephscott commented Mar 9, 2020

kienstra left a comment • 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

dero Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roccotripaldi left a comment

Choose a reason for hiding this comment

matticbot commented Mar 17, 2020

jeherve left a comment

Choose a reason for hiding this comment

dero commented Mar 19, 2020

Solution 1: Emit both the native and the jQuery events.

Solution 2: Emit jQuery events when jQuery is available, native events when it's not.

Final solution: Emit jQuery events when jQuery is available and also fire new native events.

roccotripaldi left a comment

Choose a reason for hiding this comment

matticbot commented Mar 24, 2020

jeherve left a comment

Choose a reason for hiding this comment

jeherve commented Mar 25, 2020

jetpackbot commented Mar 4, 2020 •

edited

Loading

kienstra left a comment •

edited

Loading

dero Mar 11, 2020 •

edited

Loading