airfranceklm / ng-lazy-image

Angular directive for loading responsive image when container (which is preventing reflow) is in viewport.
http://afklm.github.io/ng-lazy-image/
MIT License
351 stars 64 forks source link

Below the fold images are sometimes being loaded #16

Closed sagrawal31 closed 8 years ago

sagrawal31 commented 8 years ago

We came across this library recently and found it very helpful. But we are experiencing a random behaviour where images below the fold (which are not visible to the screen) are loaded before.

We digged into the code and found that a variable remaining (here) is coming negative.

SquadraCorse commented 8 years ago

Do you have an example online?

SquadraCorse commented 8 years ago

Will check and try to get usecase myself then, thanks for https://github.com/causecode/ng-lazy-image/commit/b71a28726b4655d3246f05ab0578c619a455b631but i could not merge it because of failing tests.

manifestinteractive commented 8 years ago

@SquadraCorse I having the exact same issue :( I put up my entire project online. This is the exact page with the issue.

http://peter.build/doing/app/#/app/events/now/

BTW, if you are viewing that link on a desktop, once the page is done loading, use the little refresh icon on that mobile layout viewer to restart the page load, otherwise you'll get the size changes of the page animating the mobile layout window down to size.

Each event listed is correctly using afkl-lazy-image="{{event.image | srcSet}}" ( I have a srcSet filter that builds out the image assets, and this is working fine ) with the afkl-lazy-image-options="{'background': true}" background option, but all the images are loading at once, even though most are off screen. I modified the release file ONLY to add console.log output.

On this app, there are 22 events. All returning the same position data:

{
    height: 640,
    scroll: 0,
    remaining: -640,
    elOffset: 0,
    offset: 50,
    windowBottom: 640
}

I am familiar enough with CSS that I tried seeing if this was a positioning issue with relative / absolute positioned elements, and perhaps there were issues fetching height information. This led me to use the afkl-image-container and even going as far as to make sure my nested markup had the correct positions for parent / child elements.

On that link, I had placed ( removed now ) afkl-image-container on the div.event-stream element. But it returned the following for all 22 images:

{
    height: 0,
    scroll: 0,
    remaining: 0,
    elOffset: 0,
    offset: 50,
    windowBottom: 0
}

I'm completely at a loss on where this is not working. The code looks good, and looks like it should be functioning correctly, so my gut is telling me that it's not correctly detecting a hight somewhere correctly, and perhaps if someone who wrote the code could look at this and KNOW what that height was supposed to be, I could work on contributing to make sure the conditions I have my code setup with are checked within the project. Certainly, I think my issue is the same as the others reported.

Lastly, I tried the patch @causecode posted, but that just made none of my background images show up. So no luck there either.

SquadraCorse commented 8 years ago

content should have the directive 'afkl-image-container' (NOT the .event-streams) so add that to same div, since that is the scrolling container. Then the lazy directives inside '.event-stream' knows what position it is relative to this container, and listens to the correct scroll (div or window)!

SquadraCorse commented 8 years ago

Your event streams have the correct offsets, so theoretically the above should work.

offsetParent: div.event-stream.one-half-responsive.ng-scope offsetHeight: 164 offsetLeft: 14 offsetTop: 169 offsetWidth: 292

manifestinteractive commented 8 years ago

@SquadraCorse Thanks for the super quick response :)

I had actually tried putting afkl-image-container on the #content element. I previously tried all the parents elements up the DOM tree, all the way to body, without luck.

HOWEVER, after some more digging I found the cause of the issue. Elements acting as the container MUST be visible when lazy image initializes. If the container has an animation on it and starts off hidden, it will not work and all images will load in container. This initial hidden state seems to screw up the calculations as the container element is not technically visible at the time lazy image is executed. I am going to play around with see how I might check for this with your code and adding a delay for when it becomes visible. I think that would solve the issue.

I have uploaded the new build of the app with the class="transition" that was on the parent element. You can see that fixed the issue for me. So it is 100% related to the visibility of the parent container and children at the time the lazy image plugin inits. I think digging around with why initial hidden elements that transition into visible would solve some of the issue people are seeing.

I'm on it. Wish me luck ;) If I find the issue I will commit a PR. But I figure I would post the cause for anyone else finding this issue the same way I did. It's probably not the code, your container is likely not visible when the page loaded.

Thanks again for the great plugin, and fast responses.

manifestinteractive commented 8 years ago

Proposed solution for issue #16, Below the fold images are sometimes being loaded.

After some digging I found that elements acting as the container MUST be visible when lazy image initializes. If the container has an animation on it and starts off hidden, it will not work and all images will load in container. This initial hidden state seems to screw up the calculations as the container element is not technically visible at the time lazy image is executed. This edge case causes the remaining to be a negative version of height. Simply checking if remaining and height cancel each other out seemed to fix the edge case.

Submitting PR with proposed fix.

Simply changed:

shouldLoad = remaining <= offset;

to

shouldLoad = (remaining <= offset && (remaining + height) !== 0);
SquadraCorse commented 8 years ago

Thanks. I pushed a possible fix based on your review. I now check for visibility of the lazy directive. Your usecase was excluding other usecases so we had to think about something different. The simulation i did can be viewed overhere http://afklm.github.io/ng-lazy-image/sample-hide-show.html