0

I'm trying to create buttons dynamically with unique listeners and handlers by using a for loop, but unfortunately I must be doing something wrong because only the last button works. Even more surprising is the fact that when clicking the last button instead of "Button No.3" it returns "Button No.4"

Bellow is the code and here is a jsfiddle http://jsfiddle.net/y69JC/4/

HTML:

<body>
    <div id="ui">
    some text ...
    </div>
</body>

Javascript:

var uiDiv = document.getElementById('ui');
uiDiv.innerHTML = uiDiv.innerHTML + '<br>';

var results = ["Button one","Button two","Button three","Button four"];

for(var n=0;n<results.length;n++)
{
 uiDiv.innerHTML = uiDiv.innerHTML + '<button id="connect'+n+'">option'+n+':'+results[n]+'</button><br>';
 var tempId = document.getElementById('connect'+n);
 tempId.addEventListener('click', function(){console.log("Button No."+n)}, false);
}

Thanks!

cweiske
  • 30,033
  • 14
  • 133
  • 194
Manolis
  • 167
  • 1
  • 12

3 Answers3

2

It's a classic case when you need a closure: Change to:

var uiDiv = document.getElementById('ui');
uiDiv.innerHTML = uiDiv.innerHTML + '<br>';

var results = ["Button one", "Button two", "Button three", "Button four"];

for (var n = 0; n < results.length; n++) {
    // Note: do not use .innerHTML. Create new element programatically
    //       and then use .appendChild() to add it to the parent.
    var button = document.createElement('button');
    button.id = 'connect' + n;
    button.textContent = 'option' + n + ':' + results[n];

    uiDiv.appendChild(button);

    button.addEventListener('click', /* this is an inline function */ (function (n2) {
        // Here we'll use n2, which is equal to the current n value.
        // Value of n2 will not change for this block, because functions
        // in JS do create new scope.

        // Return the actual 'click' handler.
        return function () {
            console.log("Button No." + n2)
        };
    })(n)); // Invoke it immediately with the current n value
}

The reason for this is that a for loop does not create a scope, so "Button No. " + n was always evaluated with the n equal to the number of elements in results array.

What has to be done is to create an inline function accepting n as a parameter and call it immediately with the current value of n. This function will then return the actual handler for the click event. See this answer for a nice and simple example.

Edit: Your code is using innerHTML property to add new buttons in a loop. It is broken because every time you assign using uiDiv.innerHTML = ..., you are deleting all contents present previously in this div and re-creating them from scratch. This caused ripping off all event handlers previously attached. Schematically, it looked like this:

uiDiv.innerHTML = ""

// First iteration of for-loop
uiDiv.innerHTML === <button1 onclick="...">

// Second iteration of for-loop
// 'onclick' handler from button1 disappeared
uiDiv.innerHTML === <button1> <button2 onclick="...">

// Third iteration of for-loop
// 'onclick' handler from button2 disappeared
uiDiv.innerHTML === <button1> <button2> <button3 onclick="...">
Community
  • 1
  • 1
kamituel
  • 34,606
  • 6
  • 81
  • 98
  • Yes, you can accomplish this, although I think this may not work in IE < 9. For an example, see [this fiddle](http://jsfiddle.net/7pHkw/) at line 13. Basically, it involves using [bind()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind?redirectlocale=en-US&redirectslug=JavaScript%2FReference%2FGlobal_Objects%2FFunction%2Fbind). – kamituel Aug 27 '13 at 13:49
  • I tried your code on jsfiddle and it doesn't work... I modified it to make it work by changing the EventListener to this: tempId.addEventListener('click',(function (n2) {return function(){console.log("Button No."+n2)}})(n), false); but it works like my code... Any ideas ? [jsfiddle](http://jsfiddle.net/y69JC/5/) – Manolis Aug 27 '13 at 14:23
  • @Manolis - what browser are you using? BTW, my answer is still valid, it answers your original question - I don't see why you removed the 'accepted' flag. – kamituel Aug 27 '13 at 14:39
  • Sorry for the confusion, I removed my question before I saw that you replied. – Manolis Aug 27 '13 at 14:41
  • I'm using Chrome, regarding the flag, I removed it because I can't get the code to work (the original code not the code that answers my external handlers question) so I thought that your answer is incorrect. – Manolis Aug 27 '13 at 14:49
  • @Manolis - I updated the answer with the explanation why it was broken for you - you shouldn't rely on `innerHTML` for DOM manipulation. See bottom of my answer and extended code sample. See [fiddle](http://jsfiddle.net/GXJuB/) for a working, complete code. – kamituel Aug 27 '13 at 15:09
0

The event handler is bound to n, which has the value results.length by the time the event fires.

You have to close the value of n by making a copy, you can do this by calling a function. This construction is known as a closure.

for(var n=0;n<results.length;n++)
{
 uiDiv.innerHTML = uiDiv.innerHTML + '<button     id="connect'+n+'">option'+n+':'+results[n]+'</button><br>';
 var tempId = document.getElementById('connect'+n);
 tempId.addEventListener('click', (function(bound_n) {
   console.log("Button No."+ bound_n);
 })(n)), false); // <-- immediate invocation
}

If n were an object, this wouldn't work because objects (unlike scalars) get passed-by-reference.

A great way to avoid having to write closures, in all your for-loops, is to not use for-loops but a map function with a callback. In that case your callback is your closure so you get the expected behaviour for free:

function array_map(array, func) {
    var target = [], index, clone, len;
    clone = array.concat([]); // avoid issues with delete while iterate
    len = clone.length;
    for (index = 0; index < len; index += 1) {
        target.push(func(clone[index], index));
    }
    return target;
}

array_map(results, function (value, n) {
    uiDiv.innerHTML = uiDiv.innerHTML
      + '<button id="connect'+n+'">option'+n+':'+results[n]+'</button><br>';
    var tempId = document.getElementById('connect'+n);
    tempId.addEventListener('click', function(){console.log("Button No."+n)}, false);    
});
Halcyon
  • 57,230
  • 10
  • 89
  • 128
  • I tried your code but I'm getting a console error: `Uncaught SyntaxError: Unexpected token )` – Manolis Aug 27 '13 at 15:07
  • I don't get that error, but look at the line and character position, that will likely tell you where the error is. – Halcyon Aug 27 '13 at 15:09
0

you can write

onclick = 'myFunction(this.id):'

in the button then later:

function myFunction(idNum){console.log("Button No."+idNum);}

also here are some improvements you may want to implement:

uiDiv.innerHTML = uiDiv.innerHTML + ....;
uiDiv.innerHTML += ....;

you also want to declare "uiDivContent" before the loop then afterward write:

uiDiv.innerHTML = uiDivContent;
Math chiller
  • 4,123
  • 6
  • 28
  • 44