Onclick adds to last element in loop only

Question

Possible Duplicate:
Assign click handlers in for loop

I need help with a loop in my code.

I loop through an array and add on clicks to divs. But it always adds onclicks to the last cycle through the loop and effectively cancels out the ones before it.

So i have this as my loop:

    start = 0;

for(i = start; i < (start+8); i++){ //8 per page
    if(i == array.length){ 
        break;  //page end
    } else {
        (function(i){
             document.getElementById('cell'+i).onclick = function(){ select(i); }
        })(i);  
    }
}

What occurs here is div id cell7 gets the on click added, but the div ids cell0 to cell6 do not. I'm guessing its something to do with the fact that i changes in the loop and so is also effected the i in the function ?

How do I fix this problem ?

I don't use jquery or any library for that matter so its not really useful to point to jquery's solution. — Sir, Oct 09 '12 at 03:21
@Dave: Is this your actual code? you're closing over the `i` variable, so each iteration creates a separate scope with its own `i` separate from the outer loop's `i`. — I Hate Lazy, Oct 09 '12 at 03:22
Yes this is my code - im not following regarding the `closing over the i`? — Sir, Oct 09 '12 at 03:23
@Dave The immediately invoked function creates a new variable scope with its own `i` variable. The handlers created inside that function invocation closes over the local `i` variable, and keeps its reference to it. Basically, you've shadowed the outer `i` with an inner `i` so your handlers will reference that one and work properly. — I Hate Lazy, Oct 09 '12 at 03:26
select currently just has `function select(i){console.log(i);}` — Sir, Oct 09 '12 at 03:28
@Dave: Not to keep second-guessing you, but are you sure the `i` parameter is defined for the `select` function? If not, it'll get the `i` from the outer scope. — I Hate Lazy, Oct 09 '12 at 03:29
When i click the div id's only the last div in the loop alerts the i value, the rest don't do any thing (they don't call my select function) like the onclick did not set =/ Also your demo i don't see a difference to mine =/ — Sir, Oct 09 '12 at 03:30
@Dave: Not enough info in the question. The code you're showing is written properly. Please post the full code that demonstrates the issue, including HTML markup. — I Hate Lazy, Oct 09 '12 at 03:32
@Dave: There's no difference in your code and my demo. I'm showing you that your code works. I'll wait for the rest of it. — I Hate Lazy, Oct 09 '12 at 03:33
here is a simplified demo of what mines does and how the div's are set: http://jsfiddle.net/NAKBk/14/ - but jsfiddle does not like my syntax for some reason. Basically i create the divs that i later assign on clicks... — Sir, Oct 09 '12 at 03:42
As an aside, what's with the if/else structure? Can't you put that test in the `for` statement's condition clause where it belongs, e.g., `for(i = start; i != array.length && i < (start+8); i++){` — nnnnnn, Oct 09 '12 at 04:28
This was closed as a duplicate? Did *anyone* happen to notice that the code already contains the "solution" in the dupe? — I Hate Lazy, Oct 09 '12 at 12:38
@Dave: Looking at your jsFiddle above, I see that you're using the dreaded `elem.innerHTML += "...new content..."`. That's a horribly destructive way to build a DOM. The DOM is *not* HTML markup. When you do that, it has to read the DOM, turn it into an HTML string, add your new string to the old one, parse the new string, make DOM elements out of it, and replace the previous content with the new. So you're destroying existing elements, and replacing them with *(nearly)* identical ones. And you're doing that in a loop, always replacing the work from the previous iteration. It's really bad. — I Hate Lazy, Oct 09 '12 at 12:44
@user1689607 can you link me where i can read up on the more effective method (i don't use a library so please no jquery links :P ) — Sir, Oct 12 '12 at 03:22

score 4 · Answer 1 · edited Feb 28 '18 at 12:06

You're close:

(function(i){
    document.getElementById('cell' + i).onclick = function(){ select(i); }
})(i);

I think what you want is:

document.getElementById('cell'+i).onclick = (function(i){
    return function(){ select(i); }
})(i);

Also, because you have:

for(i = start; i < (start+8); i++)

i is not declared, so it becomes global. Declare 'i' to keep it local (presuming this is function code):

for (var i = start; i < (start+8); i++)

If it's global code it won't make any difference (but declare it anyway).

Edit

The above doesn't fix your issue, it just changes the syntax.

The following "works" for me, cell0 to cell7 get a listener, the rest don't. select shows the correct value (0 to 7 depending on which one is clicked on).

<script>
    function addListener() {
      var start = 0;
      for(i = start; i < (start+8); i++){ 
        document.getElementById('cell'+i).onclick = (function(i){
            return function(){ select(i); }
        })(i);  
      }
    }
    function select(arg) {
      alert(arg);
    }
    window.onload = addListener;
</script>

<p id="cell0">0</p>
<p id="cell1">1</p>
<p id="cell2">2</p>
<p id="cell3">3</p>
<p id="cell4">4</p>
<p id="cell5">5</p>
<p id="cell6">6</p>
<p id="cell7">7</p>
<p id="cell8">8</p>
<p id="cell9">9</p>

Still same issue =/ It only assigns to the last div the rest don't do any thing. — Sir, Oct 09 '12 at 03:51
When i simplified it to this : `document.getElementById('cell'+i).onclick = select(i); ` It did as it should except the functions ran straight away (obviously), yet using the above method u suggest only makes it work on the last div in the loop. — Sir, Oct 09 '12 at 03:55
There is something you're not showing, perhaps related to `if(i == array.length)`. The example I posted "works". — RobG, Oct 09 '12 at 05:32

krg · Answer 2 · 2012-10-09T12:45:09.100

2

Try this example. var i = 1 assuming your HTMLElement id starts at 1. Change to reflect the situation.

var select = function(i) {
    console.log(i);
};

var arr = ["1", "2", 3, 4, 5, 6];

var start = 0;
var max = 8;

for (var i=1; i<(start + max); i++) {
    if (i === arr.length) {
        break;
    } else {
        (function(i) {
            var element = document.getElementById(['cell', i].join(''));
            element.onclick = function() {
                 select(i);  
            };
        })(i);
    }
}

Calling an external function should be a faster implementation, however.

edited Oct 09 '12 at 12:45

answered Oct 09 '12 at 03:59

krg

2,650
2
12
9

Seems to work for me. Just curious: why use `['cell', i].join('')` instead of `'cell' + i`? I've seen it more and more lately. – MiniGod Oct 09 '12 at 04:10
1

I do quite a bit of node.js and during some research, I stumbled on this Google [writeup](https://developers.google.com/speed/articles/optimizing-javascript). I've been using it ever since and it's now a convention of mine. – krg Oct 09 '12 at 04:20

score 1 · Accepted Answer · answered Oct 09 '12 at 04:12

1

I'm not sure why but these suggestions didn't fix my problem - but i managed to find a way around it by adding to the onclicks as i display the divs during the loop so i ended up doing:

innerHTML = '<Div onclick="">'; //etc etc

Instead of adding the divs to the output "then" assigning the onclicks later... don't know if my new method is the best choice but it works so i'll have to stick to it!

answered Oct 09 '12 at 04:12

Sir

8,135
17
83
146

2

Yours and everyone else's demos worked for me. Are you using IE6 or something? Btw, tag names should *always* be lowercase, ie: `
`
– MiniGod Oct 09 '12 at 05:06
Thanks for the tip with the lower case @MiniGod – Sir Oct 12 '12 at 03:22

Onclick adds to last element in loop only

3 Answers3

Edit