cancel_on_exit cancels hidden elements even when loadAll() is called

[starfishpatkhoo]

I'm not sure if this is a bug or simply by design. If so, perhaps documentation can be updated. :)

<div class="hidden">
    <img data-src="some-image.jpg">
</div>

var ll = new LazyLoad({
    elements_selector: "img"
}
ll.loadAll();

In 16.1, it would load, but in 17.0 the image loading would be cancelled because the div is hidden (I guess?). If you add cancel_on_exit: false as an option then the image is loaded just fine.

Not sure if that's how I had intepreted loadAll() which I assumed was to force loading of all images regardless of visibility.

Version: 17.0
Browser: Chrome 83

[verlok]

Hi again @starfishpatkhoo ,

I've tried to reproduce the bug you're reporting on the following demo, but I couldn't reproduce the bug.
https://www.andreaverlicchi.eu/vanilla-lazyload/demos/image_hidden.html

The instance of LazyLoad is public under the global variable LL, so you can call LL.loadAll() from devtools console to load all the images.

Cases I've tested:

Calling the LL.loadAll() method both before scrolling down.
I can see all images are loaded, including the hidden ones, as you expect.

Calling the LL.loadAll() method after having scrolled to the bottom.
I can see only the hidden images are loaded, because they're the only ones they hadn't loaded before.

Would you be able to double check it?
Thanks

[verlok]

@starfishpatkhoo any feedback would be appreciated

[starfishpatkhoo]

OK, so I wanted to send you a codepen, but.. I could nor reproduce the error either. I went back to my original code, and the problem wasn't there! I thought I was going crazy or something.. LOL!

The problem is therefore intermittent, and comes and goes (my F5 key is worn out now :) But after a lot of trial and error, I think I have managed to narrow it down. Essentially, it is a race condition.

No Error:

Have Error:

The two screenshots above are on the same site, with no code changes in between, just trying and pressing F5 many times. Usually, on the second or third try, the error will show up.

What happens is that loadAll() starts loading the image, but while it is still loading, IO triggers, but this is a hidden div, so IO says it is "exit-ed" and triggers cancel_on_exit(). Which of course, cancels it (because loading has not completed yet). This is quite difficult to reproduce, since it has to happen in exactly the right timing. In my case, I fire loadAll() via a setTimeout() on document.ready(), but I guess as a race condition, it could happen in some other "coincidental" situation. I suspect this will be more obvious on a slow or overloaded browser/PC. Maybe with many extensions/tabs/JS/VMs active for example.

With that in mind, the fix I tried out is this, on function loadAll():

--- src/lazyload.js	2020-06-25 13:43:34.000000000 +0800
+++ src/lazyload.NEW.js	2020-06-29 14:27:19.514757500 +0800
@@ -56,6 +56,7 @@
         const settings = this._settings;
         const elementsToLoad = getElementsToLoad(elements, settings);
         elementsToLoad.forEach((element) => {
+        	unobserve(element, this);
             load(element, settings, this);
         });
     }

As you can see, normally when the element has been loaded, it will be unobserved. Since we are doing a force load, we stop observing it before attempting to load the element. Seems OK so far, after a few reloads.

Again, this is just my rough understanding of the problem. I am not too sure of the correct/proper way to fix this race condition. Possibly there are other problems that will arise that I'm not aware of. :/

[verlok]

Hey @starfishpatkhoo ,
thank you again for the debugging and the explanation. That really helped.

I think the solution you're proposing makes lots of sense and I can't think of a better one.
I will apply that in the next patch.

Let me just point out that calling loadAll on document.ready() is not a best practice in terms of performance.
If you're doing that because you don't want your users to see any loading happening, you could set a high value in the threshold option to load them much in advance, but not them all.
This would also leave the hidden images unloaded, saving bandwidth to your users and improving speed.

[verlok]

I tried to put some hidden images in the load_all.html demo, but I wasn't able to reproduce the bug in any case.
Anyway I applied the suggestion to unobserve all elements at load all, beause it makes sense anyway.

Also, a couple of versions ago this script was doing exacly that: unobserve when start loading. I decided to leave the element observed until it finished loading only a couple of versions ago to implement the cancel_on_exit feature.

[verlok]

Hey @starfishpatkhoo ,
I've just released version 17.1.0 with the fix.

You might want to double check that everything is fine on your side.

After that, If you’re happy with my support, the documentation and the effort I’ve been putting on this project in the latest years, I hope you might want to buy me a coffee to show your appreciation.

Open-source software is great for everyone, but it takes passion and time to grow and evolve.

Thank you for thinking about it.
Have a nice day!

[starfishpatkhoo]

Hi @verlok, 17.1.0 works well so far! I will keep using it and monitoring, since it is a race condition. But so far, I haven't seen it fail yet. Nice one!

I think you did the right thing by keeping it observed for cancel_on_exit(). But yes, loadAll() is probably a special case. Heck, I think my whole usage is a special case anyway.

I rely on some JS events to do some image stuff, but browser support is not very stable at the moment. As a result, I ended up using what I call a "deferred" loading approach together with "eager", "lazy". Basically, it is simply "eager", but delayed for a certain amount of time until after document.ready(). The main page and everything else works as per normal, eager is eager, everything else is lazy. But for these special images, I load it say 7000ms after the rest of the document is done.

Anyway, like I said, this is a very uncommon use case. And probably I won't need "deferred" once browser support is rock solid. But right now, sad to say, have to use some UA sniffing. Sniff. Sniff.

Thank you again, excellent stuff! :)

[verlok]

Hey @starfishpatkho,
it’s complicated but... I’m glad you solved!

If you’re happy with my support, the documentation and the effort I’ve been putting on this project in the latest years, I hope you might want to buy me a coffee to show your appreciation.

Thank you for thinking about it.
Have a nice day!