Append onclick method using for loop

Question

I'm appending onclick events to elements that I'm creating dynamically. I'm using the code below, this is the important part only.

Test.prototype.Show= function (contents) {
    for (i = 0; i <= contents.length - 1; i++) {
         var menulink = document.createElement('a');
         menulink.href = "javascript:;";
         menulink.onclick = function () { return that.ClickContent.apply(that, [contents[i]]); };
    }
}

First it says that it's undefined. Then I changed and added:

var content = content[i];
menulink.onclick = function () { return that.ClickContent.apply(that, [content]); };

What is happening now is that it always append the last element to all onclick events( aka elements). What I'm doing wrong here?

possible duplicate of [Javascript infamous Loop problem?](http://stackoverflow.com/questions/1451009/javascript-infamous-loop-problem) — epascarello, Jan 06 '14 at 16:04
indeed, thanks! with that title it's less likely to be found, but its useful :) — nhenrique, Jan 06 '14 at 16:11
Also note your code is falling prey to [*The Horror of Implicit Globals*](http://blog.niftysnippets.org/2008/03/horror-of-implicit-globals.html): You need to declare `i`. — T.J. Crowder, Jan 06 '14 at 16:12
Rather than `i <= contents.length - 1`, you can use `i < contents.length` — Denys Séguret, Jan 06 '14 at 16:16

Denys Séguret · Accepted Answer · 2014-01-06T16:16:53.963

It's a classical problem. When the callback is called, the loop is finished so the value of i is content.length.

Use this for example :

Test.prototype.Show= function (contents) {
    for (var i = 0; i < contents.length; i++) { // no need to have <= and -1
         (function(i){ // creates a new variable i
           var menulink = document.createElement('a');
           menulink.href = "javascript:;";
           menulink.onclick = function () { return that.ClickContent.apply(that, [contents[i]]); };
         })(i);
    }
}

This immediately called function creates a scope for a new variable i, whose value is thus protected.

Better still, separate the code making the handler into a function, both for clarity and to avoid creating and throwing away builder functions unnecessarily:

Test.prototype.Show = function (contents) {
    for (var i = 0; i <= contents.length - 1; i++) {
        var menulink = document.createElement('a');
        menulink.href = "javascript:;";
        menulink.onclick = makeHandler(i);
    }

    function makeHandler(index) {
        return function () {
            return that.ClickContent.apply(that, [contents[index]]);
        };
    }
};

A way to avoid this problem altogether, if you don't need compatibility with IE8, is to introduce a scope with forEach, instead of using a for loop:

Test.prototype.Show = function (contents) {
  contents.forEach(function(content) {
    var menulink = document.createElement('a');
    menulink.href = "javascript:;";
    menulink.onclick = function() {
      return that.ClickContent.call(that, content);
    };
  });
}

I don't like the `menulink.href = "javascript:;";` part. If the purpose is to have a pointer cursor, then it can be done with css in a way that makes the purpose clear. — Denys Séguret, Jan 06 '14 at 16:11
Great answer! Thank you very much. As you said, a classical problem that I had no idea. Thank you! — nhenrique, Jan 06 '14 at 16:11
that part it's related with the preventdefault. I was having strange behaviors sometimes with chrome. After changing that part everything works fine, but maybe there's a better solution — nhenrique, Jan 06 '14 at 16:14
This should do AFAIK: `menu.onclick = function(e){ e.preventDefault(); ... }` — elclanrs, Jan 06 '14 at 16:15

Append onclick method using for loop

1 Answers1

Linked

Related