CODEWITHSUNDEEP
jquery
001
001

Reputation: 13533

jquery .click being called multiple times

I am getting unexpected results with jQuery trying to set the "click" method of a div. Please see this jsfiddle. Be sure to open the console window. Click the word a few times and watch the console output. The click function gets called multiple times when it should only be called once.

The commented out code at the end works just fine. Am I doing something wrong? I'm new to jQuery.

Here's the code:

function toggleDiv(status)
{
    console.log("toggleDiv(" + status + ")");
    if (status) {
        $("#test").html("Goodbye");
    }
    else  {
        $("#test").html("Hello");
    }
    $("#test").click(function() {
        toggleDiv(!status);
    });

    // Non-jquery method works fine....
    //document.getElementById("test").onclick = function () {
    //    toggleDiv(!status);
    //}
}​

Update: looks like there are lots of ways to skin this cat. The real issue here was my not understanding that the jQuery "click" functions ADDS another handler. I thought it REPLACED the current handler.

Upvotes: 13

Views: 18728

Answers (8)

Binita Bharati
Binita Bharati

Reputation: 5888

The same problem was happening with me . Joseph Erickson's answer about unbind() works. If it helps , here is the summary of how onclick event ended up being called multiple times in my code :

  • AJAX call is updating a table's tbody. The tbody content is totally dynamic.
  • The table rows are retrieved batch-wise (via server side catching).
  • A specific table data(td) had a hyperlink that has a click event associated to it.(click event added as part of AJAX callback function)
  • Now, every time, the AJAX gets called, one click event gets associated to the td.
  • Consequently, as many times, a AJAX is invoked, that many click handlers got attached to the td.

I solved this as per suggestion given by Joseph Erickson. Within the AJAX callback, I do this now : $(".msgLinkDiv").unbind(); (".msgLinkDiv").click(function(e){do my stuff});

Upvotes: 1

Mark Pieszak - Trilon.io
Mark Pieszak - Trilon.io

Reputation: 66921

You are setting a new .click() eventHandler each time you keep clicking it (which in turn creates even more events). On a side note, try to never use onclick / onmouseover / onmouseout / etc events on DOM elements. In Internet explorer these create script blocks (that you can visibly see if you use Visual Studio. Pages with thousands of these slow down performance tremendously!

It looks like you are trying to achieve this:

jsFiddle DEMO

$("#test").on('click', function() {
    var this$   = $(this),
        _status = !!this$.data('status'); // force to boolean
                  // ^will default to false since we have no data-status attribute yet

    this$.html(_status ? 'Hello' : 'Goodbye')
         .data('status', !_status);
});​

Upvotes: 19

Joseph Erickson
Joseph Erickson

Reputation: 2294

The jQuery click function doesn't overwrite a previous click handler but instead adds the new one to a queue. So when click is called again, a new click handler is added along with all the old ones.

To prevent this, you just need to clear the old handlers before defining your new one.

function toggleDiv(status)
{
    console.log("toggleDiv(" + status + ")");
    if (status) {
        $("#test").html("Goodbye");
    }
    else  {
        $("#test").html("Hello");
    }

    $("#test").unbind();

    $("#test").click(function() {
        toggleDiv(!status);
    });
}​

You may also want to look at the .toggle() event handler.

UPDATE: To be clearer about .toggle(), this will also do what you want:

$("#test").toggle(
    function(event) { $(event.target).html("Goodbye"); },
    function(event) { $(event.target).html("Hello"); }
);

Upvotes: 7

kannix
kannix

Reputation: 5157

The cleanest solution would be this: http://jsfiddle.net/kannix/fkMf9/4/

 $("#test").click(function() {
     $(this).text(($(this).text() == "Hello") ? "Goodbye" : "Hello");
 });

Upvotes: 2

Alnitak
Alnitak

Reputation: 339786

You're re-registering the click handler over and over recursively.

One correct solution (of many possible variations) is this:

$(function() {
    var status = false;

    $('#test').click(function() {
        status = !status;
        $(this).html(status ? 'Goodbye' : 'Hello');
    });
});

and then you need to remove the onclick attribute from the HTML - it's not good to mix DOM0 and DOM3 event handling.

See http://jsfiddle.net/alnitak/8aBxp/

Upvotes: 13

Corey Ogburn
Corey Ogburn

Reputation: 24717

After your if, you're adding another click event to #test. It will call all click handlers when it's clicked. You probably don't need that at all since onclick is defined in the html.

Upvotes: 1

nhahtdh
nhahtdh

Reputation: 56809

You should bind the click event outside the toggleDiv function. The current code will register new click event handler every time the elements in $('#test') is clicked, with exponential growth (since all previous click handler will generate a new click handler, so the number of handlers will double with every click).

Upvotes: 1

Kevin B
Kevin B

Reputation: 95030

Most likely you are running toggleDiv mutliple times, resulting in the click event being bound multiple times. Bind the click event outside of the toggleDiv function.

var status = false;
function toggleDiv()
{
    console.log("toggleDiv(" + status + ")");
    if (status) {
        $("#test").html("Goodbye");
    }
    else  {
        $("#test").html("Hello");
    }
    status = !status;
}​
$("#test").click(function() {
    toggleDiv(status);
});

Upvotes: 3

Related Questions