-
Notifications
You must be signed in to change notification settings - Fork 798
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
Lazy Images: Drop jQuery, convert to vanilla JS #14886
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: April 7, 2020. |
theClone = theImage.clone(true); | ||
srcset = image.getAttribute( 'data-lazy-srcset' ); | ||
sizes = image.getAttribute( 'data-lazy-sizes' ); | ||
imageClone = image.cloneNode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
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 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting.
This in Jetpack could be edited to use the non-jQuery event:
"jQuery( document.body ).on( 'jetpack-lazy-loaded-image', function () { jQuery( window ).trigger( 'resize' ); } );" |
...but of course the question of 3rd-party code remains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
@kienstra Could you please review this when you have a moment? |
$( '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 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn 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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good
Hi @dero,
This looks good, and lazy-loading still works as expected 😄
It's approved, pending your points below.
sizes = theImage.attr( 'data-lazy-sizes' ); | ||
theClone = theImage.clone(true); | ||
srcset = image.getAttribute( 'data-lazy-srcset' ); | ||
sizes = image.getAttribute( 'data-lazy-sizes' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting.
This in Jetpack could be edited to use the non-jQuery event:
"jQuery( document.body ).on( 'jetpack-lazy-loaded-image', function () { jQuery( window ).trigger( 'resize' ); } );" |
...but of course the question of 3rd-party code remains.
|
||
if ( ! theImage.length ) { | ||
if ( ! image instanceof HTMLImageElement ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea
jetpackLazyImagesLoadEvent = document.createEvent( 'Event' ) | ||
jetpackLazyImagesLoadEvent.initEvent( 'jetpack-lazy-images-load', true, true ); | ||
} | ||
jQuery( 'body' ).get( 0 ).dispatchEvent( jetpackLazyImagesLoadEvent ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
jQuerytrigger
creates proprietary events that are only observable using jQuery (on
,bind
etc.). Meaning that there is no 3rd party code out there usingaddEventListener
to observe them.The nativedispatchEvent
is observable using bothaddEventListener
andjQuery
methods. Importantly, jQuery will normalize the incoming event and expose any additionaldetail
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tested well!
Caution: This PR has changes that must be merged to WordPress.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
9689cad
to
3bf681a
Compare
@jeherve The issue should be fixed by 3bf681a. The reason for introducing a new
While 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 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 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 Final solution: Emit jQuery events when jQuery is available and also fire new native events.When Any feedback and suggestions welcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dero, Your synced wpcom patch D40433-code has been updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work well in my tests. Merging.
r204798-wpcom |
* 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
Rewrites the
lazy-images
module to not depend on jQuery.Fixes #14863
Changes proposed in this Pull Request:
Is this a new feature or does it add/remove features to an existing part of Jetpack?
lazy-images
module.Testing instructions:
twentytwenty
locally.)Proposed changelog entry for your changes:
lazy-images
in vanilla JS.