Help with crashing jquery code

Question

Hi

Working on an image gallery, but it keeps crashing on me (you control it by clicking on 'left' and 'right') if i click on the images more than 10 times:

http://korrektur.adnuvo.com/hejven/index_test2.html

Here's the code:

$(document).ready(function(){

/*''''''''''' WORKING COPY !!!!! V2 '''''''''''*/
var gal_prev = null;
var gal_next = null;

var click_img = function() {
    console.profile();

    $(this).unbind();

    if($(this).attr('class') == 'gal_right') {
        /* IF RIGHT */
        /*$('#test').append('Im right. ');*/
        get_pos($(this));

        $(this).siblings('.gal_left').removeClass().addClass('gal_img');
        $(this).siblings('.gal_right').removeClass().addClass('gal_img');
        $(this).removeClass().addClass('gal_active');

        $(gal_prev).removeClass().addClass('gal_left');
        $(gal_prev).bind('click',click_img);

        $(gal_next).removeClass().addClass('gal_right');
        $(gal_next).bind('click',click_img);
    } else {
        /* IF LEFT */
        /*$('#test').append('Im left. ');*/

        get_pos($(this));

        /*$(this).parent().children().removeClass().addClass('gal_img');*/
        $(this).siblings('.gal_left').removeClass().addClass('gal_img');
        $(this).siblings('.gal_right').removeClass().addClass('gal_img');
        $(this).removeClass().addClass('gal_active');

        $(gal_prev).removeClass().addClass('gal_left');
        $(gal_prev).bind('click',click_img);

        $(gal_next).removeClass().addClass('gal_right');
        $(gal_next).bind('click',click_img);
    }

    console.profileEnd();
}

/* BIND ACTIONS TO IMAGES */
$('#img2').bind('click',click_img);
$('#img5').bind('click',click_img);

/* GET POSITION */
var get_pos = function(gal_clicked) {
    if(($(gal_clicked).next().length != 0) && ($(gal_clicked).prev().length != 0)) {
        /*$('#test').append('Im in the middle somewhere. ');*/
        gal_prev = $(gal_clicked).prev();
        gal_next = $(gal_clicked).next();
    } else if($(gal_clicked).prev().length != 0) {
        /*$('#test').append('Im last. ');*/
        gal_prev = $(gal_clicked).prev();
        gal_next = $(gal_clicked).parent().find('div:first');
    } else {
        /*$('#test').append('Im first. ');*/
        gal_prev = $(gal_clicked).parent().find('div:last');
        gal_next = $(gal_clicked).next();
    }
} });

Can anyone help me out. I'm not a master at jquery, so all tips and comments are more than welcome :)

Answer 1

It looks to me like you're binding functions to click without unbinding them later. Either you can call $('#gal_container > div').unbind('click', click_img) at the top of click_img, or you can just call .live(...) once on the required CSS selectors at initialisation time, which will absolve you of any binding in the future.

Answer 2

You are binding events, but never unbinding them.

var click_img = function() {
    ...
    $(gal_prev).bind('click',click_img);
    $(gal_next).bind('click',click_img);
    ...
}

Click once and click_img executes once, adding a event handler to a gal img. Click again and both the old click_img event handler and the newly-bound click_img event handler execute. Each adds another copy of click_img to the event handler list. Now you have four event handlers. Click again and you have eight, and so on... by ten clicks you have a thousand event handlers and it is not surprising the browser grinds to a halt trying to call them all.

Binding and unbinding a load of events all the time is a pain. Let's simplify by having one event handler for all the imgs:

var divs= $('#gal_container>div');
var activei= 0;
divs.click(function() {

    // Find div number and number of left and right div
    // only allow click if the left or right div was clicked
    //
    var divi= divs.index(this);
    var nexti= (divi-1+divs.length)%divs.length;
    var previ= (divi+1)%divs.length;
    var isadjacent= activei===nexti || activei===previ;
    activei= divi;
    if (!isadjacent) return;

    // Set classes on each div to reflect new state
    //
    divs.each(function(i) {
        $(this).toggleClass('gal_active', i===divi);
        $(this).toggleClass('gal_left', i===previ);
        $(this).toggleClass('gal_right', i===nexti);
        $(this).toggleClass('gal_img', i!==nexti && i!==previ && i!==divi);
    });
});

If you don't need to limit clicks to only the left and right divs, you can lose isadjacent and activei completely. It would be normal to allow a click on any div in a gallery.

(Also you'd typically leave the gal_img class on every div.)

Answer 3

You only unbind the the click event from the currently clicked element (either left or right, but not from both buttons), and then reassign new events..

A simple solution to your problem, thus, would be to change $(this).unbind(); to $(this).unbind().siblings().unbind()

You could just assign the click event to all divs and sniff the action to perform like you do now from the class the element has (without any unbinding / rebinding) ..

Additionally you do not have to wrap jquery object with the $ again ..

All those changes in your code would yield

var gal_prev = null;
var gal_next = null;

var click_img = function() {
    console.profile();
    if($(this).attr('class') == 'gal_right') {
        /* IF RIGHT */
        /*$('#test').append('Im right. ');*/
        get_pos($(this));

        $(this).siblings('.gal_left').removeClass().addClass('gal_img');
        $(this).siblings('.gal_right').removeClass().addClass('gal_img');
        $(this).removeClass().addClass('gal_active');

        gal_prev.removeClass().addClass('gal_left');

        gal_next.removeClass().addClass('gal_right');
    } else if($(this).attr('class') == 'gal_left'){
        /* IF LEFT */
        /*$('#test').append('Im left. ');*/

        get_pos($(this));

        /*$(this).parent().children().removeClass().addClass('gal_img');*/
        $(this).siblings('.gal_left').removeClass().addClass('gal_img');
        $(this).siblings('.gal_right').removeClass().addClass('gal_img');
        $(this).removeClass().addClass('gal_active');

        gal_prev.removeClass().addClass('gal_left');

        gal_next.removeClass().addClass('gal_right');
    }
    console.profileEnd();
}

/* BIND ACTIONS TO IMAGES */
$('.gal_container div').bind('click',click_img);

/* GET POSITION */
var get_pos = function(gal_clicked) {
    if((gal_clicked.next().length != 0) && (gal_clicked.prev().length != 0)) {
        /*$('#test').append('Im in the middle somewhere. ');*/
        gal_prev = gal_clicked.prev();
        gal_next = gal_clicked.next();
    } else if(gal_clicked.prev().length != 0) {
        /*$('#test').append('Im last. ');*/
        gal_prev = gal_clicked.prev();
        gal_next = gal_clicked.parent().find('div:first');
    } else {
        /*$('#test').append('Im first. ');*/
        gal_prev = gal_clicked.parent().find('div:last');
        gal_next = gal_clicked.next();
    }
}