Bind click event to elements created in a nested for loop

Question

I've been doing a lot of reading on closures today, and have looked through quite a few similar questions. I haven't been able to find one that touches on the exact same issue I'm having and how to fix it. Most answers seem to be jQuery related, but there are some fantastic answers out there on how closures work. I know that's where my problem is here. I've tried many different closure examples with no luck.

I have a set of data representing 3 campuses. Each campus has some buildings in it. The first for loop runs through the campuses. The nested for loop runs through the buildings in the current campus and binds a click event to it (which allows me to pan and zoom a leaflet map programmatically.)

I've created a very simplified JSFiddle to represent what I'm trying to do, which shows the issue. Only the buildings in the last "campus" group retain the click event.

Here's the JS code from JS Fiddle. http://jsfiddle.net/scwutpno/3/

var campuses = {
    "campus1": {
        "buildings": {
            "building1": {
                "id": 1
            },
                "building2": {
                "id": 2
            },
                "building3": {
                "id": 3
            }
        },
        "name": "Campus 1"
    },
        "campus2": {
        "buildings": {
            "building1": {
                "id": 4
            },
                "building2": {
                "id": 5
            },
                "building3": {
                "id": 6
            }
        },
        "name": "Campus 2"
    },
        "campus3": {
        "buildings": {
            "building1": {
                "id": 7
            },
                "building2": {
                "id": 8
            },
                "building3": {
                "id": 9
            }
        },
        "name": "Campus 3"
    }
};

var buildingLinksContainer = document.getElementById("building-list");

function buildExternalLinks() {

    for (var campus in campuses) {
        var container,
        item;


        if (buildingLinksContainer !== null) {
            buildingLinksContainer.innerHTML += '<div class="building-links" id="' + campus + '-buildings">' +
                '<div class="building-links__campus-name">' + campuses[campus].name + '</div></div>';

            container = document.getElementById(campus + '-buildings');
        }


        for (var building in campuses[campus].buildings) {

            item = container.appendChild(document.createElement('li'));
            item.innerHTML = 'Building ' + campuses[campus].buildings[building].id;

            item.addEventListener("click", function () {
                alert('clicked!');
            });
        }

    }
}

buildExternalLinks();

I've tried putting the click event in a self-invoking closure, but I don't think it's the data inside the click event that's having issues. It's the node that the click event is being applied to. I don't know if because I'm using a nested for loop, I need some sort of nested closure concept?

Would love to see how this issue is solved. I've already overcome it once by using a closure in a for loop, but this nested for loop just has me stumped.

cskrovseth · Accepted Answer · 2014-12-17 12:56:24Z

The problem is that your code rewrites the DOM each time you append to the "buildingLinksContainer" element. Every time your code appends to the "buildingLinksContainer"'s innerHTML with "+=", the DOM is created from scratch. All elements that previously had event listeners inside "buildingLinksContainer" are created as new elements without any event listeners.

Only on the last iteration, when the DOM isn't torn down afterwards, do the event listeners stick. And since only the last three li elements are created in the last iteration, only they have event listeners.

The culprit:

buildingLinksContainer.innerHTML += '<div class="building-links" id="' + campus + '-buildings">' +
'<div class="building-links__campus-name">' + campuses[campus].name + '</div></div>';

I've rewritten your solution in a way that works (with the bonus of better performance and markup)

var buildingLinksContainer = document.getElementById("building-list");

function buildExternalLinks(campuses, rootElement) {

    var ul = document.createElement('ul'),
        li = document.createElement('li'),
        h1 = document.createElement('h1'),
        list = document.createDocumentFragment();

    for(var campus in campuses) {
        if(campuses.hasOwnProperty(campus)) {
            var c = ul.cloneNode(false);
            c.classList.add('building-links');
            c.setAttribute('id', campus+'-buildings');

            var name = h1.cloneNode(false);
            name.appendChild(document.createTextNode(campuses[campus].name));

            c.appendChild(name);

            for(var building in campuses[campus].buildings) {
                if(campuses[campus].buildings.hasOwnProperty(building)) {
                    var b = li.cloneNode(false);
                    var text = 'Building ' + campuses[campus].buildings[building].id;
                    b.appendChild(document.createTextNode(text));
                    b.addEventListener('click', function(event) {
                        alert(event.target.innerHTML);
                    });

                    c.appendChild(b);
                }
            }
        }

        list.appendChild(c);
    }

    rootElement.appendChild(list);
}

buildExternalLinks(campuses, buildingLinksContainer);

I've also updated/forked your JSFiddle.

The problem was what Teemu answered below actually. The "+=" on the innerHTML of the container. I'll update my answer with this explanation.
Interesting. So, this really wasn't a closure issue at all. I didn't know what what was happening with the dom listeners. What about Edwin's suggestion of binding the event listener to the container instead of each element?
Correct, the closure was fine, it was the rewriting of the DOM. Edwin's solution depends on the events bubbling up from the li elements to the parent's parent element. I would say you have more control over your events if they are added to their own elements. If you were to do an event.stopPropagation() on the click event before they reach the parent it'll stop working. Also if you need other click events, on each campus for example, this may have unintended consequences. You can use event.target to check what element the event originated from (like Edwin's solution).
Whether to use event delegation or not depends on the use case. If the DOM is going to change often and event listeners must be removed and added every time, event delegation may be easier and more efficient, it consumes more CPU on event trigger, but less in the binding of them. If the DOM is not going to change often, then it's more efficient to add events to each element. This consumes more CPU when binding the events, but less for each event triggered. In your case it seems to be a fairly static list, so I would not use event delegation.

Edwin Reynoso · Accepted Answer · 2014-12-17 09:10:38Z

0

Use event delegation only add an event listener to the building-list element

document.querySelector('#building-list').addEventListener('click', function(evt){
   var clickedElement = evt.target;
   //just use the clickedElement variable to get the element that was clicked
   //ex: you can get the id from it or whatever you want
});

answered Dec 17, 2014 at 9:10

Edwin Reynoso

1,5519 silver badges21 bronze badges

Comments

Teemu · Accepted Answer · 2014-12-17 15:19:21Z

+= is just a shortcut to something like x = x + y. When setting buildingLinksContainer.innerHTML += ... you'll actually re-create the whole content of that element. All the previous content is removed, including the events attached to the elements. Then a new string is set to the DOM, and when parsing it, there's no events anymore.

A quick-fix would be to use insertAdjacentHTML() instead of innerHTML, like so:

buildingLinksContainer.insertAdjacentHTML('beforeend',
    '<div class="building-links" id="' + campus + '-buildings">' +
    '<div class="building-links__campus-name">' +
    campuses[campus].name +
    '</div></div>'
);

A live demo at jsFiddle.

Thank you Teemu, I plugged this in and it worked great, and I plan to refactor a bit now that I understand what is going on.

Collectives™ on Stack Overflow

Bind click event to elements created in a nested for loop

3 Answers 3

6 Comments

Comments

1 Comment

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

6 Comments

Comments

1 Comment

Related