Javascript eventhandler always being added to last object in array

Question

I am trying to perform two tasks on a "toggle collapse" button (which is actually an image):

1. If a section of the page is loaded collapsed (from cookie) then add the class "collapsed" to an ancestor tag.
2. Add an event handler to toggle the presence of the "collapsed" class when the button is clicked.

To do so, I have the following code:

function toggleCollapsedPlugin(sender, collapseState) {
    // do something
    // expects 1 OR 2 arguments
}

function initCollapsedPlugin() {
    var forumcats = document.getElementById('forums').getElementsByClassName('forumbit_nopost L1');
    var button;
    var buttonParent;
    for (var i = 0; i < forumcats.length; i++) {
        button = forumcats[i].getElementsByTagName('a')[1].getElementsByTagName('img')[0]; // will always exist
        buttonParent = button.parentNode.id; // will never be empty
        if (button.src.indexOf('_collapsed') >= 0) {
            toggleCollapsedPlugin(button.parentNode.id, true);
        }
        button.addEventListener('click', function () { toggleCollapsedPlugin(buttonParent); }, false);
    }
}

As I step through initCollapsedPlugin() for the first item in the Chrome DevTools, everything appears to be fine. When I later click one of the images to call toggleCollapsedPlugin(sender), sender is always equal to buttonParent of the last item in forumcats. Can someone explain why and how to fix this?

(If it helps anyone provide an answer, this is the vBulletin4 software.)

Answer 1

This is a classic 'pitfall' of JavaScript closures. Basically, your buttonParent variable is being overwritten in every iteration and the anonymous function you're passing to addEventListener references but does not copy this variable.

You'll probably need something similar to:

button.addEventListener('click', (function (parent) {
    return function() {
        toggleCollapsedPlugin(parent);
    };
})(buttonParent), false);

For a more detailed explanation, see How do JavaScript closures work?.

In your case however, you can simply get rid of the closed variable altogether and find the button's parent when the anonymous function is called. This works because this inside of the callback function refers to the element the event was registered on, being the element references by button in that iteration.

button.addEventListener('click', function() {
    return toggleCollapsedPlugin(this.parentNode.id);
}, false);

Answer 2

You're declaring buttonParent, assigning it to the event call, but then changing it in the loop until eventually it refers to the last item.

I think all the events will point to this same reference.

Replace

button.addEventListener('click', function () { toggleCollapsedPlugin(buttonParent); }, false);

with

button.addEventListener('click', function () { toggleCollapsedPlugin(this.parentNode.Id); }, false);

"this" inside the event on the button object will be the button itself

Answer 3

You get the last value for buttonParent because every event handler is referencing the same variable. The buttonParent is captured as part of the function's closure over local variables.

By the time the event handler is called, the value of buttonParent has been set to the last button's parent by the for loop.

This is why, generally, you shouldn't declare functions In a loop. The usual solution is to use a function invocation to pass values to the event handler. Instead of calling addEventListener directly, you invoke a function, passing the button you want to add a listener to. The act of calling the function copies the variable reference into a function argument, so there's a separate copy for each event listener.

function(b) {
  var buttonParent=b.parentNode.id;
  b.addEventListener(...)
} (button);