lazysizes icon indicating copy to clipboard operation
lazysizes copied to clipboard

occasional miss on ls.attrchange.js

Open 719media opened this issue 5 years ago • 6 comments

In certain conditions I can't realiably repeat every time (due to loading race conditions), the ls.attrchange.js plugin fails to see a change in attributes. I can repeat only on initial page load from an empty browser cache, and then only on occasion. I've narrowed down to the place in the code that it's happening, but I don't understand lazysizes internals well enough to know how best to address the issue.

First, the "lazyload" class is removed: https://github.com/aFarkas/lazysizes/blob/gh-pages/src/lazysizes-core.js#L498

Then, the mutation observer fires, but the regex condition fails: https://github.com/aFarkas/lazysizes/blob/gh-pages/plugins/attrchange/ls.attrchange.js#L57

It's looking for /(\s|^)(lazyloaded|lazyloading)(\s|$)/, but these classes have NOT been added yet because the rAF has not yet fired here:

https://github.com/aFarkas/lazysizes/blob/gh-pages/src/lazysizes-core.js#L500

according to my inspector, when this issue happens, the rAF is firing 0.001 seconds after the regex check. When things are working normally, of course, the rAF doesn't have a problem and fires before the regex check.

I'll try to find a more reliable test case, but perhaps this is enough information for someone with great knowledge (e.g. the author :) ) of the library to see the issue?

Thanks for any help

719media avatar Feb 11 '19 05:02 719media

In the meantime, is there a standard public method exposed for basically "recalculating" an element manually?

For example, is there a simpler one-liner way of doing https://github.com/aFarkas/lazysizes/blob/gh-pages/plugins/attrchange/ls.attrchange.js#L29-L43

in lazysizes main js? Like a lazySizes.refresh(target); or something? Seems like this would be a useful method to have? I couldn't see anything in the documentation.

719media avatar Feb 11 '19 05:02 719media

Yeah, I see there is a possible race condition. I might have a simple solution for that. But currently travelling so might take me some hours before I apply it.

About the one liner no at the end this functionality is not needed by core and for the core it is as simple as adding the lazyload class. The additions where only added for some plugins (like unload).

aFarkas avatar Feb 11 '19 06:02 aFarkas

@719media

I just wanted to work on this and while checking the code I can not see that there is actually a race condition.

The loading class is just added here so right before the load class is removed. Can you check or explain why this is not happening in your case?

aFarkas avatar Feb 11 '19 09:02 aFarkas

Thanks for the reply.

I've tracked it down to this line being falsy for me (on occasion) on load:

https://github.com/aFarkas/lazysizes/blob/gh-pages/src/lazysizes-core.js#L463

Specifically, in my hard-to-repeat test case, the src is an empty string in the (srcset || src || isPicture) condition when it doesn't work (and has a string value when it does). The same problem happens if you just have no src attribute at all. An example is:

<img src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" data-src="" class="lazyload">. So lazysizes is firing, seeing the empty data-src, and failing the condition, so lazyloaded class is not added until later, when the latter rAF adds it.

Does that make sense?

719media avatar Feb 11 '19 18:02 719media

Ok, this is an extrem edge case. Can I ask why you set the image to an empty string. Should be a no-go.

aFarkas avatar Feb 12 '19 12:02 aFarkas

Hi there,

I can confirm. I have a similar issue in a paginated list in React. When I get back to a previous page, the library takes a little time before setting the lazyloaded class. Thus, I see the alt text for a while then the image. I need a way to set the lazyloaded class manually once the image is loaded.

goldmont avatar Jun 01 '22 17:06 goldmont