Question

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

Was it helpful?

Solution

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

OTHER TIPS

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.

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.

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.

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);
            })}
 } 
Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top