3

This is probably an easy question but I can't figure out the best answer for it.

I have 10 <div> elements on screen. Each of them has a click() event listener:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
</head>
<body>
    <div id="element0">Click me! (0)</div>
    <div id="element1">Click me! (1)</div>
    <div id="element2">Click me! (2)</div>
    <div id="element3">Click me! (3)</div>
    <div id="element4">Click me! (4)</div>
    <div id="element5">Click me! (5)</div>
    <div id="element6">Click me! (6)</div>
    <div id="element7">Click me! (7)</div>
    <div id="element8">Click me! (8)</div>
    <div id="element9">Click me! (9)</div>
    <script type="text/javascript">
    for ( var i = 0; i < 10; i++ ) {
        var element = document.getElementById( "element" + i );
        element.onclick = function () {
            alert( "Element " + i );
        }
    }
    </script>
</body>
</html>

But every time I click on an element it says "Element 10"! It seems all those event handlers are using the same value for i.

I want it to show "Element N" where N is the number of the current element. I don't want to extract N from the element id. Neither I want to store it using data() method of jQuery. I believe there must be a much simpler solution to this problem, but I can't find it. Anyone?

Improve this question
1
  • i is declared in the global scope and not within the onclick function, therefore it is being read from the global scope when accessed within the onclick. It shows 10 because that was the last iteration of the loop. Commented May 18, 2012 at 16:34

3 Answers 3

Reset to default
6

You only have one variable i in an outer scope shared by all your click handlers. You need to create a local variable i for each closure. This will work:

for ( var i = 0; i < 10; i++ ) {
    var element = document.getElementById( "element" + i );
    element.onclick = (function(i){
        // returns a new function to be used as an onclick handler
        return function () {
            alert( "Element " + i );
        }
    })(i); // Pass in the value of the outer scope i
}

Check "The Infamous Loop" problem in this article (and read the whole artice) for more information :)

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

so I'm creating two functions for every event listener. Isn't there a more efficient and cleaner version?
@AlexStack This is pretty efficient. You have an anonymous function in your loop, which executes immediately and returns a new function with a local i variable for each click handler. Each event handler is still only one function.
I know, but in our project there will be thousands of this function created every few seconds. That's why I'm concerned about efficiency.
@AlexStack I don't think that will be an issue. I'm getting consistently under 700 milliseconds to generate 10000 divs with that onclick handler and a mouseover and a mouseout handler plus some styles: jsfiddle.net/GVxSk/2
@AlexStack Of course Jamund's event delegation solution is about twice as fast giving me under 350 milliseconds consistently: jsfiddle.net/GVxSk/3
3

So your problem is the nature of doing async code inside of loops as the other post said. In your case though there's altogether a better way to solve this problem and that's using event delegation.

document.body.onclick = function(e) {
  if (e.target.tagName.toLowerCase() === "div") {
    alert( "Element " + e.target.id );
  }
}

If you had jQuery you could do this:

$(document.body).on("click", "div", function() {
  alert($(this).attr("id"));
});

For more info check out this article: http://www.sitepoint.com/javascript-event-delegation-is-easier-than-you-think/

Obviously jQuery and other libs handle this stuff automatically as well.

Improve this answer

5 Comments

Thanks, but as I said I don't want to parse the id of the triggering element to get N. Also I tried the code. It seems to need this.id be replaced with e.target.id to work properly.
Thanks for the fix. Event delegation uses way less resources than assigning click events to every div, so I'd still recommend it over the other approach.
Is this the actual HTML or are you trying to solve a more complex problem?
api.jquery.com/index might actually help here if you just want to know the index of the one clicked.
yeah it's a part of a bigger project, but it was hard to describe so I put up a code snippet to demonstrate exactly the part that I couldn't solve. Thanks for the code and the link.
2

You could grab the number from the element's id attribute:

for ( var i = 0; i < 10; i++ ) {
    var element = document.getElementById( "element" + i );
    element.onclick = function () {

        var i = this.id.substr(7);
        alert( "Element " + i );
    }
}​

JsFiddle here.

Improve this answer

1 Comment

interesting solution. Though as I said I don't want to parse the id to get the number. This is just an example to demonstrate a bigger "scope problem" from a project.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.