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

Add srcset support for data-lazy #1661

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vuongdothanhhuy
Copy link

Hello,

During working with my project, I need to use slick with the picturefill and lazyload the image. I found that slick has data-lazy for that, but it links with src, so I made some minor modifications to the source code to add support for data-lazy-srcset -> srcset.
Here is a very simple demo to prove my data-lazy-srcset works: https://jsfiddle.net/pwq4g54b/embedded/result/

Thank you.

@sephie
Copy link

sephie commented Aug 28, 2015

+1, this would be a great addition for apps with responsive images.

@juancabrera
Copy link

👍

2 similar comments
@marius1
Copy link

marius1 commented Aug 31, 2015

👍

@baamenabar
Copy link

+1

@masi
Copy link

masi commented Nov 5, 2015

Yes, please. This feature would be most welcome.

@hultberg
Copy link

Duplicate of #916 ?

@janein
Copy link

janein commented Dec 29, 2015

+1

@jtree5757
Copy link

+1 - This would take it to the next level!

p.s.sorry for missing this before posting Does lazy loading work with srcset? #2260

@petebarr
Copy link

+1

@kenwheeler
Copy link
Owner

@leggomuhgreggo what do you think?

@leggomuhgreggo
Copy link
Collaborator

leggomuhgreggo commented Aug 17, 2016

It's a great idea. I think srcset still needs the src attribute as a fallback for older browsers. It'd be nice to get that factored in here.

Also anticipating requests for the full picture tag. Here's an example library with support for picture.

@julienchazal
Copy link

+1

1 similar comment
@mwinkelm
Copy link

mwinkelm commented Jan 2, 2017

+1

@kenwheeler
Copy link
Owner

can we get these conflicts resolved?

@VesterDe
Copy link

I'm really looking forward to this update, is there any eta perhaps?

@samjonesigd
Copy link

+1

1 similar comment
@jobbrown
Copy link

+1

@leggomuhgreggo leggomuhgreggo self-assigned this Apr 20, 2017
@GGrossberger
Copy link

+1

@samjonesigd
Copy link

any update on this on another branch or release?

@89gsc
Copy link

89gsc commented Jul 11, 2017

Used this and it doesn't seem to load srcset correctly, on desktop its loading the large image as it should but on mobile its loading both large first then small (at same time but in that order in the requests)

image

EDIT:

I see progressive should be used this brings another issue with this modification. When used on a page with say 20 images in a slider the entire page will freeze up and will also increase the memory / CPU usage of Chrome by a huge amount and then unfreeze once all images have been loaded.

@NeilJS
Copy link

NeilJS commented Oct 17, 2017

Similar problem with @GSC89 - two images are being loaded according to Chrome + Firefox Network tabs (although regardless of screen size), defeating the purpose of srcset. One is the correct image from the available images in the data-srcset attribute but the data-lazy image (aka src image) is also always being loaded.

The generated markup for each slide seems correct, for example, if I was to copy the generated markup for a slide and use it directly in a template, the srcset aspects do work as intended (eg the following generated slide from this referenced demo correctly loads just the one relevant image, if pasted directly into php template - but it won't benefit from lazy-load obviously!)...

<div>
  <img src="http://placehold.it/350x300?text=1-350w" srcset="http://placehold.it/650x300?text=1-650w 650w, http://placehold.it/960x300?text=1-960w 960w" sizes="100vw">
</div>

Is there any definitive demo of lazy-load + srcset (+ ideally 'anticipated')? Some demos out there use sligltly different syntax (eg data-lazy, data-lazy-srcset etc).

@NeilJS
Copy link

NeilJS commented Oct 18, 2017

Looks like the reason two images are loaded is because imageToLoad.src is used to load an initial image (first image loaded); then, imageToLoad.onload sets the srcset attributes (second image loaded).

I've looked at a way of setting the src, srcset, sizes attributes all at the same time with the aim of just downloading the one relevant image. Replacing imageToLoad.src (within Slick.prototype.lazyLoad() and Slick.prototype.progressiveLazyLoad() ) with the following seems to help:

image.attr('src', imageSource);
image.attr('srcset', imageSrcSet);
image.attr('sizes', imageSizes);

The imageToLoad.onload and imageToLoad.onerror functions updating accordingly and I'm having issues with some later slides not loading the image, so still needs work, but thought I'd mention this idea.
Thanks

@janwidmer
Copy link

Any Idea when this change might come to a release?

@tristanbes
Copy link

tristanbes commented Nov 28, 2017

I'm also voting for this issue. It's a must to avoid downloading x2 or x5 images at the same time if you have different image size for responsive.

@mxgz
Copy link

mxgz commented Dec 19, 2017

+1

@sephie
Copy link

sephie commented Dec 22, 2017

It seems that this feature was already released in 1.7.1. Close?
#2681 #2456 #2981 #2942 #2843

@MathiasF
Copy link

MathiasF commented Dec 22, 2017 via email

@NeilJS
Copy link

NeilJS commented Dec 24, 2017

Needs bugs addressing before closing issue. Multiple images are loaded by the browser as mentioned above, defeating the purpose of using srcset.

@devinwalkley
Copy link

I noticed a site the other day using slick with srcset for their hero slider and it seems to be working good on all browsers www.insomniac.com

@dmitrymar
Copy link

Is it possible to combine slick carousel built with picture tag items that also uses lazyLoad option? I only found srcset example https://github.com/kenwheeler/slick/pull/2681/files but it won't do in my case.

@projer-zz
Copy link

projer-zz commented May 4, 2018

If you are using source elements, you can do it like this:

<picture>
	<source data-lazy-srcset="my-fancy-image-xs.jpg" media="(max-width: 767px)">
	<img data-lazy="my-fancy-image.jpg" alt="">
</picture>
$( '.my-slider' ).on( 'lazyLoaded', function( evt ) {
	$( evt.currentTarget ).find( 'source' ).each( function( i, source ) {
		var $source = $( source );
		$source.attr( 'srcset', $source.data( 'lazy-srcset' ) );
	} );
} );

Works for me. Doesn't work if you have srcset on the img element, but can be adapted.

But could be awesome avoiding this :)

@mikehaertl
Copy link

mikehaertl commented Aug 16, 2018

@projer While your solution seems to work I found it to have a serious drawback: It always first loads the normal version and then afterwards loads the alternative version from the <source> tag. So now you have 2 files loaded each time! This of course renders the <picture> tag useless, as the whole meaning is to keep the size of downloaded assets small.

Any other idea how to fix this?

@rahulbpatel
Copy link

rahulbpatel commented Oct 16, 2018

Here is my solution based on @projer's solution:

<picture>
        <!-- Media query specific image --> 
	<source data-lazy-srcset="my-fancy-image-xs.jpg" media="(max-width: 767px)">

        <!-- Default image --> 
	<source data-lazy-srcset="my-fancy-image.jpg">       

        <!-- Use transparent gif to trigger Slick lazyLoaded event --> 
	<img data-lazy="data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==" alt="">
</picture>
$( '.my-slider' ).on( 'lazyLoaded', function( evt, slick, $img ) {

    $img
        // Find the parent <picture> tag of img
        .closest('picture')
        // Find <source> tags with data-lazy-srcset attribute
        .find('source[data-lazy-srcset]')
        // Copy data-lazy-srcset to srcset
        .each(function (i, source) {
            $source = $(source);
            $source.attr('srcset', $source.data('lazy-srcset'));
        }); 

} );

@gtempesta
Copy link

gtempesta commented Dec 21, 2018

@rahulbpatel's solution is perfect if you don't want to load two srcs. I'm using it for a slightly different case as I'm loading webp or jpg files depending on browser support. This is an example markup (javascript code would be the same as in @rahulbpatel's solution).

<picture>
    <!-- WebP image --> 
    <source data-lazy-srcset="my-fancy-image-xs.webp">

    <!-- Default image --> 
    <source data-lazy-srcset="my-fancy-image.jpg">       

    <!-- Use transparent gif to trigger Slick lazyLoaded event --> 
    <img data-lazy="data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==" alt="">
</picture>

@Kristinita
Copy link

Unfortunately, PageSpeed Insights doesn't support @projer solution.

I opened issue in Lighthouse repository. Write if any ideas.

Thanks.

@DisasterMan78
Copy link

@rahulbpatel Thank you for the suggestions, very interesting.
I'm having a problem with Slick not resizing though. the lazyLoaded trigger works, the attributes update, but when the slides switch they have no visible content and the carousel resizes to minimum height (The first slide is fine, because my template doesn't write the lazyload attributes for it so it will load ASAP).

Do you have any idea how to get around this? I'm using the latest Slick release.

@rahulbpatel
Copy link

@rahulbpatel Thank you for the suggestions, very interesting.
I'm having a problem with Slick not resizing though. the lazyLoaded trigger works, the attributes update, but when the slides switch they have no visible content and the carousel resizes to minimum height (The first slide is fine, because my template doesn't write the lazyload attributes for it so it will load ASAP).

Do you have any idea how to get around this? I'm using the latest Slick release.

Do you have an example online to check out? I can't tell from your description why content is not showing up

@DisasterMan78
Copy link

Sorry I missed your reply @rahulbpatel, been away for a while.

I'd love to produce a reduced test case, but sadly crushed under a pile of other people's bugs being thrown at me. We've de-prioritised the issue for now, frustratingly, but I hope to come back to it later.

@artilishes
Copy link

Here is my workaround, in case someone faces the same issue while picture elements are not supported and still wants the img tag inside the picture to hold an actual image.
It's build upon @rahulbpatel solution, but without using the slick internal lazyload function.
So make sure to turn off lazyload in the settings:

$('.PR_js-detail-slickSlider').slick({
  ...
  lazyLoad: false,
  ...
});

Custom Lazyload:

$('.slider').on('beforeChange', function (event, slick, currentSlide, nextSlide) {
  var nextSlideImg = $('.slider .slick-slide[data-slick-index="' + nextSlide + '"]');

  var lazySlides = function (el, attr, lazyAttr) {
    nextSlideImg
      .find(el)
      .each(function (i, img) {
        $img = $(img);
        $img.attr(attr, $img.data(lazyAttr))
      })
  }

  lazySlides('picture source[data-lazy-srcset]', 'srcset', 'lazy-srcset');
  lazySlides('img[data-lazy]', 'src', 'lazy');
});

@offirpeer
Copy link

Any idea when this will be ready? @kenwheeler

@seltix5
Copy link

seltix5 commented Sep 18, 2019

I'm locking for this feature too. I wold like to implement webp images and it wold need the picture tag to work correctly as fallback

@offirpeer
Copy link

You can solve this issue using

Intersection observer

code example by me:

$(function () {

    var $images = $("[data-src-observer]");
    var imgOptions = {
        threshold: 0,
        rootMargin: "0px 0px 0px 0px"
    };

    var imgObserver = new IntersectionObserver((entries, imgObserver) => {
        entries.forEach(entry => {
            if (!entry.isIntersecting) {
                return;
            }
            else {
                PreloadImage(entry.target);
                imgObserver.unobserve(entry.target);
            }
        });
    }, imgOptions);


    $images.each(function (i, el) {
        imgObserver.observe(el);
    })


    function PreloadImage(img) {
        var src = $(img).data("src-observer");

        if (!src) {
            return;
        } else {
            $(img).attr("src", src);
        }

        var srcset = $(img).data("srcset-observer");
        if (!srcset) {
            return;
        } else {
            $(img).attr("srcset", srcset);
        }
    }

});

@RahulKRatan
Copy link

+1
Looking forward for this feature.

@Niloys7
Copy link

Niloys7 commented Jan 5, 2020

Any Update when this will be ready?

@pratham2003
Copy link

This input can be added as a separate proposal. I'm only posting it here as a reference.

Initial thought being this: What if my slide's mobile image is different than the desktop image?

According to srcset, if the larger resolution image is already in browser's cache the browser will choose to show the larger resolution image instead of the smaller resolution on mobile. There is a small possibility this happens when switching between landscape and portrait modes on a mobile/tablet.

@mobiusint
Copy link

Any update on this?

@samjonesigd
Copy link

Any update on this?

I just use a lazy load library like lazysizes

@laudeco
Copy link

laudeco commented Sep 21, 2021

You can solve this issue using

Intersection observer

code example by me:

$(function () {

    var $images = $("[data-src-observer]");
    var imgOptions = {
        threshold: 0,
        rootMargin: "0px 0px 0px 0px"
    };

    var imgObserver = new IntersectionObserver((entries, imgObserver) => {
        entries.forEach(entry => {
            if (!entry.isIntersecting) {
                return;
            }
            else {
                PreloadImage(entry.target);
                imgObserver.unobserve(entry.target);
            }
        });
    }, imgOptions);


    $images.each(function (i, el) {
        imgObserver.observe(el);
    })


    function PreloadImage(img) {
        var src = $(img).data("src-observer");

        if (!src) {
            return;
        } else {
            $(img).attr("src", src);
        }

        var srcset = $(img).data("srcset-observer");
        if (!srcset) {
            return;
        } else {
            $(img).attr("srcset", srcset);
        }
    }

});

This solution is still an expiremental one and is not supported by all the browsers ...

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.