23

Refactoring standard onClick within html tag to listeners ,faced problem with my code:

var td;
    for (var t=1;t<8;t++){
        td = document.getElementById('td'+t);
        if (typeof window.addEventListener==='function'){
                td.addEventListener('click',function(){
                    console.log(td);
            })}
 }  

When td element is clicked on,it's assumed that clicked td with last index from loop,e.g. 7
Looks like ,eventListeners been populated for last element in this loop only.
Loop initialization looks correct.
Why so happened?

Here is live code

sergionni
  • 13,290
  • 42
  • 132
  • 189

5 Answers5

62

You need to wrap the assignment of the event listener in a closure, something like:

var td;
for (var t = 1; t < 8; t++){
    td = document.getElementById('td'+t);
    if (typeof window.addEventListener === 'function'){
        (function (_td) {
            td.addEventListener('click', function(){
                console.log(_td);
            });
        })(td);
    }
}
jabclab
  • 14,786
  • 5
  • 54
  • 51
  • it works absoulutely OK.Thank you.Onr more question about closure: how it's happened,that `_td` is equal to `td`? – sergionni Jan 18 '12 at 12:22
  • 4
    The closure is just a function which executes immediately, therefore preserving the state of the variables at the time of execution. We execute the function / closure by passing in `td` for the `_td` input parameter to the function. – jabclab Jan 18 '12 at 12:27
  • 1
    Thank you for explanation.I just confused with `_td` and `td` naming.Looks like ,I can use just `td` – sergionni Jan 18 '12 at 12:29
  • yeah it would be great to get a bit of explanation of what that _td is and also what that (td) at the end is. – john-jones Feb 19 '14 at 09:16
  • 2
    @HermannIngjaldsson np, the `_td` is simply the input parameter for the `function` which forms the closure and `td` at the end is the value which is passed to the `function`. They don't actually have to have different names, e.g. they could both be `td`. Hope that helps :-) – jabclab Feb 19 '14 at 09:28
  • Taking away the _ from `_td`, this did the trick for me too. *With* the underscore the function applies to the last item from the `for()` loop. Thanks – paddotk Jun 26 '14 at 15:44
36

What's happening

The variable td is defined outside of your event handlers, so when you click on a cell you are logging the last value it was set to.

More technically: each event handler function is a closure—a function that references variables from an outer scope.

The general solution

The general solution to this kind of problem is to return the event handler from a wrapper function, passing the variables you want to "fix" as arguments:

td.addEventListener('click', function(wrapTD) {
    return function() { console.log(wrapTD); }
}(td));

The arguments are now bound to the scope of the invoked wrapper function.

Simpler solution: use this

There's an even simpler option, however. In an event handler, this is set to the element on which the handler is defined, so you can just use this instead of td:

td.addEventListener('click', function() {
    console.log(this);
});

Even simpler: no loop!

Finally, you can get rid of the for loop entirely and set a single event handler on the whole table:

var table = document.getElementById('my-table');

table.addEventListener('click', function(e) {
    if (e.target.nodeName.toUpperCase() !== "TD") return;

    var td = e.target;
    console.log(td);
});

This is a much better solution for larger tables, since you are replacing multiple event handlers with just one. Note that if you wrap your text in another element, you will need to adapt this to check if the target element is a descendant of a td.

Jordan Gray
  • 16,306
  • 3
  • 53
  • 69
  • 10 years later, just wanted to say thanks for this: '...since you are replacing multiple event handlers with just one'. – nograde Mar 03 '22 at 19:28
8

So what's happening here is that you are keeping the variable 'td' in scope for the event listener function. There is only 1 instance of 'td', and that is getting updated each time the for loop iterates. Thus, when the for loop finished, the value of td is now set to the element '#td7' and your event handler is simply logging the current value of td.

In the example above, you could simply log 'this':

var td;
for (var t=1;t<8;t++){
    td = document.getElementById('td'+t);
    if (typeof window.addEventListener==='function'){
      td.addEventListener('click',function(){
        console.log(this);
      });
    }
}

since 'this' is set to the element an event was bound on for the execution of an event handler.

I'm guessing you're looking for more of an answer about keeping hold of the iterator when creating closures from within a for loop. To do this you want to define a function outside of the for loop.

for (var t=1;t<8;t++){
  bind_event(t);
}

function bind_event(t) {
    var td = document.getElementById('td'+t);
    if (typeof window.addEventListener==='function'){
      td.addEventListener('click',function(){
        console.log(td);
      });
    }
}

This way, an instance of a variable named 'td' is created each time bind_event is run, and that instance will be kept in closure for the event listener function. It's worth noting that 't' in bind_event is also a new variable.

Will Tomlins
  • 1,436
  • 16
  • 12
1

As i understand, it happens because of closure...you assign event handler within scope of FOR statement. When click happens, it takes the last version if TD variable in scope of FOR and writes it to log.

Alex Dn
  • 5,465
  • 7
  • 41
  • 79
0

the following should work as expected:

  for (var t=1;t<8;t++){
        var td;
        td = document.getElementById('td'+t);
        if (typeof window.addEventListener==='function'){
                td.addEventListener('click',function(){
                    console.log(this);
            })}
 } 
Evert
  • 8,161
  • 1
  • 15
  • 17