13

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.

8 Answers 8

19

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);
});​
5
  • Thanks. I am unfamiliar with the "data-status" attribute but it fits my needs. Note however, that to get your code to work as toggle, need to add "$(this).data('status', true);" to else statement.
    – 001
    Aug 23, 2012 at 15:13
  • If you only want to change that text you should take a look at my answer you don't need to use .data() at all
    – kannix
    Aug 23, 2012 at 15:16
  • Why do you mix up $("#test") and $(this) ?
    – kannix
    Aug 23, 2012 at 15:22
  • Ah i'll edit that, the OP had $("#test") references! Changing now Aug 23, 2012 at 15:23
  • 1
    @mcpDESIGNS you've left an extra call to .data() in the ternary operator. Other than that, this is a vast improvement over your original code.
    – Alnitak
    Aug 24, 2012 at 6:44
13

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/

1
  • Thanks. This works too but I think mcpDESIGNS's answer fits my needs better.
    – 001
    Aug 23, 2012 at 15:15
7

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"); }
);
2
  • I will look into toggle(). It may be what I need.
    – 001
    Aug 23, 2012 at 15:27
  • I added an update with a code sample that should help you out. Aug 23, 2012 at 15:29
3

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);
});
2

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

 $("#test").click(function() {
     $(this).text(($(this).text() == "Hello") ? "Goodbye" : "Hello");
 });
1
  • Thanks. This works but not for my project. The jsfiddle was just a trivial example for testing.
    – 001
    Aug 23, 2012 at 15:22
1

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).

3
  • no, the number of handlers will increase by one with every click (i.e. linearly)
    – Alnitak
    Aug 23, 2012 at 15:01
  • @Alnitak: I have checked via the console. The number of times toggleDiv is called (which is equal to the number of times click event is registered) double every click.
    – nhahtdh
    Aug 23, 2012 at 15:04
  • ah yes, so it does. Apologies.
    – Alnitak
    Aug 23, 2012 at 15:07
1

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.

1

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});

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.