Adding events to elements inside loop - JScript/Closures

Question

I'm building a list of buttons and I want each one to trigger the addForm() function with the current members[member].id.

But it happens that only the last button will fire the event.

I know it has something to do with closures and as you can see I have adapted the function to use this pattern.

What am I doing wrong?

function displayConnections(connections) {
 /*(...)*/
 for (var member in members) {
 connectionsDiv.innerHTML += "<p>" + members[member].firstName + " " + members[member].lastName
 + " ID: " + members[member].id;
 btn = document.createElement("input");
 btn.setAttribute("type","button");
 btn.setAttribute("value","Send Message");
 btn.setAttribute("id",members[member].id);
 btn.onclick = function (id) {
 return function () {
 addForm(id);
 };
 }(members[member].id);
 connectionsDiv.appendChild(btn);
 } 
}

Thanks.

Answer 1

First, remember you are not writing C# or Java. The for (var ... in ...) structure does not iterate a collection. You should always check hasOwnProperty to see if the property name belongs to the object itself:

if (!members.hasOwnProperty(member)) continue;

Then check to make sure that the property value is an object and not a function etc.

Second, your variable btn is lacking a var declaration. You are creating a global variable called btn, not a variable local to your function.

Next, you have a typo mistake in your original code. Your original code actually is interpreted this way (thanks to JavaScript's auto-semicolon-insertion feature!):

btn.onclick = function (id) {
 return function () {
 addForm(id); <-- this id is now the click event's event object, not what you want
 };
};
(members[member].id); <-- this line will have no side effect

In order to run your program in your original style, you need to bracket the function definition:

btn.onclick = (function (id) {
 return function () {
 addForm(id);
 };
})(members[member].id);

• Alnitak: I think you're wrong about simplifying the closure - members[member].id will continue to change on each pass through the loop.
• Joao Heleno: This correction still gives me the last button being the one to fire the event.

Answer 2

Well, instead of those workaround you can also use:

element.setAtrribute('onClick', 'javascript:functionName(' + parameter + ');');

which worked for me and should work for you as well.

Answer 3

In this block of code:

btn.onclick = function (id) {
 return function () {
 addForm(id);
 };
}(members[member].id);

You have the syntax wrong - it should be:

btn.onclick = (function(id) {
 return function () {
 addForm(id);
 }
})(members[member].id);

What this does is automatically invoke a new anonymous closure, which has a locally bound copy of members[member].id in its id parameter, which itself then returns a closure which is what's actually bound to btn.onclick.

The automatic invokation only happens if you put the outer parenthesis around the closure declaration.

• Joao Heleno: With your correction only the last button will fire the event.

Answer 4

for (var i = 0; i < 10; i++) {
 // Won't work
 //$('div').eq(i)[0].onclick = function() { alert(i) }; // 10
 $('div').eq(i)[0].onclick = (function(id) {
 return function() {
 alert(id)
 };
 })(i);
}

jsFiddle.