CSS-Tricks / MovingBoxes

Simple horizontal slider which grows up the current box when it's in focus (image, title & text) and back down when it's not in focus.
http://css-tricks.github.io/MovingBoxes/
GNU Lesser General Public License v3.0
280 stars 147 forks source link

Memory leak if one of the images is not loaded #124

Open d-u-a-l opened 10 years ago

d-u-a-l commented 10 years ago

It's in base.imagesLoaded function

ic = ('fileSize' in t[i] && t[i].fileSize < 0 && t[i].count > 10) ? true : t[i].complete;

must be like

ic = (('fileSize' in t[i] && t[i].fileSize < 0) || t[i].count > 10) ? true : t[i].complete;

because in your case ic would never be true, even after 10 iterations it would be t[i].complete, which is false, because img is not loaded

Mottie commented 10 years ago

Hi @d-u-a-l!

Were you experiencing a problem with this?

IE is actually the only browser that would set the image complete property to false, other browsers would set it to true even if the image failed to load. Also, IE is the only browser with a fileSize property, and it returns -1 until the file has actually loaded.

Try it yourself with this demo. Click the "Load Images" button, then click on any image and it will show the image properties of complete, fileSize and count in the console.

d-u-a-l commented 10 years ago

Tried this just now - Opera does not set property to true, if this image is not loaded. I especially set src attr of one of images to non-existent file and value of property complete of this image remained false. My corrections were wrong, I admit, but you have to change something, because when this happens in Opera I got heavy load on processor and dramatic increase in memory consumption

d-u-a-l commented 10 years ago

oh, by the way, I'm using Opera 12.16

d-u-a-l commented 10 years ago

One more problem - I'm experiencing problems with navigation arrows in IE10. They seems to be not working, but sometimes, when you click at them few times, they begin to work.

Mottie commented 10 years ago

ok, try this code:

ic = ('fileSize' in t[i] && t[i].fileSize < 0 && t[i].count > 10) || t[i].count > 10 ? true : t[i].complete;

I just tried it in Opera, but since I'm on a Windows platform, I have version 17.0 and it does set the complete property to true when an image isn't found.

If that code works, I'll include it with the next update, thanks!

Edit: And I'll look into that IE issue.

d-u-a-l commented 10 years ago

Guess it will work. Opera 17 works on Webkit engine, like Chrome and Safari, so it sets complete to true. Opera 12 has it's own engine

I've made a website, where navigation is not working, it's on russian language, thou. It may help finding problem on IE http://al.devep.ru/

Mottie commented 10 years ago

Well, it looks like the problem is that base.initialized isn't set to true.... which is set when all images are loaded, which doesn't seem to be triggering the callback function (which sets the base.initialized flag), even with the above change.

I haven't had a chance to test it, but I think this additional change is needed:

if (c || img.length === 0) {
    // all complete, run the callback
    if (typeof callback === "function") { callback(); }
}
d-u-a-l commented 10 years ago

it didn't help, because img.length stays the same, as it was after first iteration, because you only push elements to array, but not delete them from it. The thing is, in my case, there's many images, that are hidden, so their height equals 0. That's why code c = (c && ic && t[i].height !== 0) always returns false after it checks height of hidden images why is it so important to check height of images, if we know, that they already loaded?

There's another inconvenience - that is a period after page loaded, when you can't use navigation (something like timeout 2animation speed, o.speed \ 2) - but it's not that bad (even thou I use a fairly long animation speed)

Mottie commented 10 years ago

Hmm, you're right... I didn't think about hidden images. Even failed images have a height (alt text), so yeah that height doesn't really do anything, so it should be removed as well.

You can always set the o.speed to a low number then elevate it after initialization:

$('#slider').on('initialized.movingBoxes', function(event, mb){
    mb.options.speed = 2000; // or whatever
});

Thanks!