addEventListener using for loop and passing values

Question

I'm trying to add event listener to multiple objects using a for loop, but end up with all listeners targeting the same object --> the last one.

If I add the listeners manually by defining boxa and boxb for every instance, it works. I guess it's the addEvent for-loop that's not working the way I hoped for. Maybe I use the wrong approach altogether.

Example using 4 of the class="container" Trigger on container 4 works the way it´s supposed to. Trigger on container 1,2,3 trigger event on container 4, but only if trigger has already been activated.

// Function to run on click:
function makeItHappen(elem, elem2) {
  var el = document.getElementById(elem);
  el.style.backgroundColor = "red";
  var el2 = document.getElementById(elem2);
  el2.style.backgroundColor = "blue";
}

// Autoloading function to add the listeners:
var elem = document.getElementsByClassName("triggerClass");

for (var i = 0; i < elem.length; i += 2) {
  var k = i + 1;
  var boxa = elem[i].parentNode.id;
  var boxb = elem[k].parentNode.id;

  elem[i].addEventListener("click", function() {
    makeItHappen(boxa, boxb);
  }, false);
  elem[k].addEventListener("click", function() {
    makeItHappen(boxb, boxa);
  }, false);
}

<div class="container">
  <div class="one" id="box1">
    <p class="triggerClass">some text</p>
  </div>
  <div class="two" id="box2">
    <p class="triggerClass">some text</p>
  </div>
</div>

<div class="container">
  <div class="one" id="box3">
    <p class="triggerClass">some text</p>
  </div>
  <div class="two" id="box4">
    <p class="triggerClass">some text</p>
  </div>
</div>

Solution 1

Closures! :D

This fixed code works as you intended:

// Function to run on click:
function makeItHappen(elem, elem2) {
    var el = document.getElementById(elem);
    el.style.backgroundColor = "red";
    var el2 = document.getElementById(elem2);
    el2.style.backgroundColor = "blue";
}

// Autoloading function to add the listeners:
var elem = document.getElementsByClassName("triggerClass");

for (var i = 0; i < elem.length; i += 2) {
    (function () {
        var k = i + 1;
        var boxa = elem[i].parentNode.id;
        var boxb = elem[k].parentNode.id;
        elem[i].addEventListener("click", function() { makeItHappen(boxa,boxb); }, false);
        elem[k].addEventListener("click", function() { makeItHappen(boxb,boxa); }, false);
    }()); // immediate invocation
}

<div class="container">
  <div class="one" id="box1">
    <p class="triggerClass">some text</p>
  </div>
  <div class="two" id="box2">
    <p class="triggerClass">some text</p>
  </div>
</div>

<div class="container">
  <div class="one" id="box3">
    <p class="triggerClass">some text</p>
  </div>
  <div class="two" id="box4">
    <p class="triggerClass">some text</p>
  </div>
</div>

---

Why does this fix it?

for(var i=0; i < elem.length; i+=2){
    var k = i + 1;
    var boxa = elem[i].parentNode.id;
    var boxb = elem[k].parentNode.id;

    elem[i].addEventListener("click", function(){makeItHappen(boxa,boxb);}, false);
    elem[k].addEventListener("click", function(){makeItHappen(boxb,boxa);}, false);
}

Is actually non-strict JavaScript. It's interpretted like this:

var i, k, boxa, boxb;
for(i=0; i < elem.length; i+=2){
    k = i + 1;
    boxa = elem[i].parentNode.id;
    boxb = elem[k].parentNode.id;

    elem[i].addEventListener("click", function(){makeItHappen(boxa,boxb);}, false);
    elem[k].addEventListener("click", function(){makeItHappen(boxb,boxa);}, false);
}

Because of variable hoisting, the var declarations get moved to the top of the scope. Since JavaScript doesn't have block scope (for, if, while etc.) they get moved to the top of the function. Update: as of ES6 you can use let to get block scoped variables.

When your code runs the following happens: in the for loop you add the click callbacks and you assign boxa, but its value gets overwritten in the next iteration. When the click event fires the callback runs and the value of boxa is always the last element in the list.

Using a closure (closing the values of boxa, boxb etc) you bind the value to the scope of the click handler.

---

Code analysis tools such JSLint or JSHint will be able to catch suspicious code like this. If you're writing a lot of code it's worthwhile to take the time to learn how to use these tools. Some IDEs have them built-in.

Solution 2

You can use Function Binding.You dont need use closures.See below:

Before:

function addEvents(){
   var elem = document.getElementsByClassName("triggerClass");

   for(var i=0; i < elem.length; i+=2){
      var k = i + 1;
      var boxa = elem[i].parentNode.id;
      var boxb = elem[k].parentNode.id;

      elem[i].addEventListener("click", function(){makeItHappen(boxa,boxb);}, false);
      elem[k].addEventListener("click", function(){makeItHappen(boxb,boxa);}, false);
   }
}

After:

function addEvents(){
   var elem = document.getElementsByClassName("triggerClass");

   for(var i=0; i < elem.length; i+=2){
        var k = i + 1;
      var boxa = elem[i].parentNode.id;
      var boxb = elem[k].parentNode.id;
      elem[i].addEventListener("click", makeItHappen.bind(this, boxa, boxb), false);
      elem[k].addEventListener("click", makeItHappen.bind(this, boxa, boxb), false);
   }
}

Solution 3

You facing the scope/closure problem as function(){makeItHappen(boxa,boxb);} boxa and boxb references then always the last one element(s).

To solve the issue:

function makeItHappenDelegate(a, b) {
  return function(){
      makeItHappen(a, b)
  }
}

// ...
 elem[i].addEventListener("click", makeItHappenDelegate(boxa,boxb), false);

Solution 4

It's because of closures.

Check this out: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures#Creating_closures_in_loops_A_common_mistake

The sample code and your code is essentially the same, it's a common mistake for those don't know "closure".

To put it simple, when your create a handler function inside addEvents(), it does not just accesses the variable i from the addEvents()'s environment, but it also "remembers" i.

And because your handler "remembers" i, the variable i won't vanish after addEvents() was executed.

So when the handler is called, it will use the i but the variable i is now, after the for-loop, 3.

Solution 5

I also had this problem a while ago. I solved it by using a "adds" function outside the loop, to assign events, and it worked perfectly.

Your script should look like.

function addEvents(){
  var elem = document.getElementsByClassName("triggerClass");
  for(var i=0; i < elem.length; i+=2){
    var k = i + 1;
    var boxa = elem[i].parentNode.id;
    var boxb = elem[k].parentNode.id;

//- edit ---------------------------|
    adds(boxa, boxb);
  }
}
//- adds function ----|
function adds(obj1, obj2){
  obj1.addEventListener("click", function(){makeItHappen(obj1, obj2);}, false);
  obj2.addEventListener("click", function(){makeItHappen(obj1, obj2);}, false);
}
//- end edit -----------------------|

function makeItHappen(elem, elem2){
  var el = document.getElementById(elem);
  el.style.transform = "flip it";
  var el2 = document.getElementById(elem2);
  el2.style.transform = "flip it";
}

View more solutions

Comments

[Abhidev]

its because of closures :D ...but why don't you add the event listener on the parent element rather than adding to it to each child element?

[Halcyon]

@musefan they are the problem, because of variable hoisting. Please see my explanation.

[lastr2d2]

should }(i, k)) be })(i, k)?

[Felix Kling]

Is actually invalid JavaScript.: If it was invalid, it would not parse or execute. Using the closure like I showed you, you do get the behaviour you were expecting: Correct, but the important point is that you are executing a function inside the loop, which creates a new scope.

[Felix Kling]

@Wayne: Either way is fine. Personally I find the first one more readable.

[Halcyon]

This is essentially the same solution. You create a closure in makeItHappenDelegate by returning a function that is bound to the actual values of boxa and boxb.

[Halcyon]

@Felix potato potato, changed wording to non-strict. @Wayne either works but }()) is my preference.

[tenbits]

yes, but it is much better as we do not create self invoking function on each loop. Yes we create the delegate, but this is minimal overhead.

[Halcyon]

In this case I doubt the (extremely minimal) performance overhead outweighs the readability benefits. If you have long lists, say 100.000+ you might see a performance benefit but below that I really wouldn't worry about it. Readability is far more important.

[tenbits]

Hm, delegation is much more better approach, even in sense of readability and maintainability. SOLID

[Bernd]

I do agree, I find this much more readable and maintainable.

[Magnus]

I like this solution. It is very clear and readable.

[Pak Ho Cheung]

i like this too. thanks.

[Jeff P.]

@Halcyon This is a really great explanation on closures! Favorited this question and answer.

[Pedro Gomes]

Nice solution, easy to implement and works. Would you be able to provide some more insights in how/why it works? (I know I can just search for it, but it would be nice to have 1 or 2 paragraphs directly here for the ones who end up here in the future.)

[MarketerInCoderClothes]

So well explained, thank you so much !

[B.Z.]

up vote for mentioning let option

[bolkay]

I literally have no idea how this works but it does, and boy is it clean! Thanks very muchie!