JavaScript addEventListener in loop

Question

I'm trying to work through Udacity's JavaScript Design Patterns course. One of the practice projects is to create a page with a couple of photos of cats. Each photo has its own title and counter indicating how many times each photo has been clicked on. I started off with having everything hard coded in html, but I'm now trying to make it more general and easier to update by generating most of the page from JavaScript. I've been able to get all the text and images to show up, and I've got the second image's counter increasing when it's clicked, but I can't figure out why the first image isn't updating its counter. Clicking on it doesn't even seem to fire the 'click' EventListener.

HTML:

<html>
<body>
<div id="cats"></div>
</body>
<script src="catclick.js"></script>
</html>

JS:

"use strict";

var cat1 = {};
cat1.photo = "cat.jpg";
cat1.name = "Whiskers";
cat1.count = 0;

var cat2 = {};
cat2.photo = "cat2.jpg";
cat2.name = "Socks";
cat2.count = 0;

var cats = [cat1, cat2];

var catDiv = document.getElementById('cats');

function increaseCount(cat, span) {
    cat.count++;
    span.innerHTML = cat.count;
}

for (var i=0; i<cats.length; i++) {
    (function () {
        var currCat = cats[i];

        var existingHTML = catDiv.innerHTML;
        var newHTML = '<h3>' + currCat.name + '</h3><img id="' + currCat.name + '-photo" src="' + currCat.photo + '">';
        newHTML += '<p>' + currCat.name + ' has been clicked <span id="' + currCat.name + '-count">0</span> times.</p>';    
        catDiv.innerHTML = existingHTML + newHTML;

        var photoObj = document.getElementById(currCat.name + "-photo");
        var countSpan = document.getElementById(currCat.name + "-count");

        photoObj.addEventListener('click', function(){increaseCount(currCat, countSpan);}, false);
    }())
}

I make an array of two cat objects, get a reference to the div in my html, define the increaseCount function that updates the span containing a given cat photo's click count, and then loop through each cat object in my array, generating the html for their title, image, and counter before adding an event listener for when their image is clicked.

The second image works as expected, but the first one doesn't even fire the event listener. I thought it might be related to closure, but I've wrapped everything in the loop in a function.

Update

With Pointy's suggestion, I updated the loop as follows, and it's working as expected. I've also added i to the function as suggested, though I'm not sure it's actually needed in this case.

for (var i=0; i<cats.length; i++) {
    (function (i) {
        var currCat = cats[i];

        var newDiv = document.createElement("div"); // div for this cat

        var catTitle = document.createElement("h3"); // cat name 
        catTitle.appendChild(document.createTextNode(currCat.name));
        newDiv.appendChild(catTitle);

        var catPhoto = document.createElement("img"); // cat photo
        catPhoto.setAttribute("id", currCat.name + "-photo");
        catPhoto.setAttribute("src", currCat.photo);
        newDiv.appendChild(catPhoto);

        var catPara = document.createElement("p"); // cat click count
        var catSpan = document.createElement("span");
        catSpan.setAttribute("id", currCat.name + "-count");
        catSpan.appendChild(document.createTextNode("0"));
        catPara.appendChild(document.createTextNode(currCat.name + " has been clicked "));
        catPara.appendChild(catSpan);
        catPara.appendChild(document.createTextNode(" times."));
        newDiv.appendChild(catPara);

        catDiv.appendChild(newDiv);

        catPhoto.addEventListener('click', function(){increaseCount(currCat, catSpan);}, false);
    }(i));
}

When you set the `.innerHTML`, even though you're putting back the previous contents you're losing the event handler assignments. Event handler assignments are associated with the DOM nodes themselves, and overwriting `.innerHTML` deletes that portion of the DOM and creates a whole new subtree. — Pointy, Feb 21 '16 at 15:42
Split it into two separate loops: first, build the content, then assign the event handlers. Or else build the content by appending DOM nodes instead of overwriting the DOM via `.innerHTML`. — Pointy, Feb 21 '16 at 15:43
I was able to fix the problem with your suggestion Pointy. Thanks. — WhiteHotLoveTiger, Feb 21 '16 at 16:18

Andy · Accepted Answer · 2016-02-21T16:21:07.327

There's a couple of things.

First you need to pass in i to the function:

(function (i) {
  var currCat = cats[i];
  ...
}(i));

But the second point is more tricky. You're creating elements on the fly and trying to attach event listeners to them which doesn't work. What you need to take advantage of is event delegation - attaching an event listener to a parent element and catching the events that bubble up from the attached elements.

Some changes need to be made to your code to make this work.

1) Add a couple of data attributes to the image elements to reference the index of the cat, and the cat name:

<img data-name="' + currCat.name + '" data-index="' + i + '" + id="' + currCat.name + '-photo" src="' + currCat.photo + '">

2) Create the new event listener on the catDiv element, picking up the values from the clicked element's data set, passing in those values to inceaseCount.

catDiv.addEventListener('click', function(e) {
  if (e.target && e.target.nodeName == "IMG") {
    var data = e.target.dataset;
    increaseCount(data.index, data.name + '-count');
  }
});

3) Rewrite the increaseCount function to account for those changes.

function increaseCount(index, span) {
  cats[index].count++;
  document.getElementById(span).innerHTML = cats[index].count;
}

Working example

This looks to me like it should work, but I'm actually still getting the same behaviour. — WhiteHotLoveTiger, Feb 21 '16 at 15:34
I ended up going with a different solution (see updated question), but this is nice because it still allows for the the total rewriting of the "cats" div, which would have been less re-writing of the existing code. Thanks. — WhiteHotLoveTiger, Feb 21 '16 at 16:35

JavaScript addEventListener in loop

1 Answers1