Problem with dynamically set onclick

[abs]

Hi everyone.

Please, check my test code here: http://skocz.pl/jstest . The trouble is
that no matter which span element I click, it alerts '3' and I'm wondering
why not '1' for the first span, '2' for second and '3' for third ? Anybody
has any idea ?

Best regards,
ABS

[RobG]

abs wrote:

Hi everyone.

Please, check my test code here: http://skocz.pl/jstest . The trouble is
that no matter which span element I click, it alerts '3' and I'm wondering
why not '1' for the first span, '2' for second and '3' for third ?

The values should be 0, 1 and 2.

Anybody has any idea ?

You have discovered closures - the code of interest is:

function setOnclicks() {
spans = document.body.getElementsByTagName('span')
for(i=0; i<spans.length; i++) {
span = spans[i]
span.onclick = function() {testfunction(i)}
}
}

i here forms a closure back to i in the function (which you've created
as a global, but even if you'd made it local you'd still get the same
result). When the loop finishes, the value of i is 3, so all the
onclicks show 3.

Always declare variables with var, particularly for counters like i
inside functions. Some candidates for a solution are:
function setOnclicks() {
var spans = document.body.getElementsByTagName('span')
for(var i=0; i<spans.length; i++) {
span = spans[i]
span.onclick = new Function('testfunction(' + i + ')');
}
}
But new Function is considered only marginally better than using eval.
Another way to modify the scope chain is:

function setOnclicks() {
var spans = document.body.getElementsByTagName('span')
for(var i=0; i<spans.length; i++) {
span = spans[i]
span.onclick = (function(x){
return function(){testfunction(x)};
})(i);
}
}
However I find that exacerbates IE's memory leak problem with closures
involving DOM elements - it may not be a problem for you in this case.

You can also modify the scope chain by using a function to attach the
handler:

function setOnclicks() {
var spans = document.body.getElementsByTagName('span')
for(var i=0; i<spans.length; i++) {
addOnclick(spans[i], i);
}
}

function addOnclick(el, i){
el.onclick = function(){testfunction(i);};
}
A novel suggestion by Lasse Nielsen[1] uses with:

function setOnclicks() {
var spans = document.body.getElementsByTagName('span')
for(var i=0; i<spans.length; i++) {
span = spans[i]
with ({temp : i}) {
span.onclick = function(){testfunction(temp)};
}
}
}
1. <URL:
http://groups.google.com.au/group/co...0caf593c683e27

>

There are probably other solutions, search the archives for 'onclick
closure'
--
Rob

[bart.lateur]

abs wrote:

Please, check my test code here: http://skocz.pl/jstest . The trouble is
that no matter which span element I click, it alerts '3' and I'm wondering
why not '1' for the first span, '2' for second and '3' for third ? Anybody
has any idea ?

The reason is because all the functions you use in your assignment, all
reference the same variable i. After your assigment, the i changes, and
these functions reflect the change. If one of the functions changed the
value for i, the other would see the same change.

Here's one way to solve this:

function set(i) { return function() {testfunction(i)} };
for(i=0; i<spans.length; i++)
{
span = spans[i]
span.onclick = set(i);
}

This creates a new closure for every invocation, so each sees a
different i.

A somwhat more complex way with the same result, is replacing the inner
function with one that is defined and called in one go:

for(i=0; i<spans.length; i++)
{
span = spans[i]
span.onclick = (function (i) { return function() {testfunction(i)}
})(i);
}

--
Bart.

[abs]

"RobG" <rg***@iinet.net.auwrote in message
news:45**********************@per-qv1-newsreader-01.iinet.net.au...

The values should be 0, 1 and 2.

Ofcourse :) Sorry.

You have discovered closures - the code of interest is: [...]

Thank you very much for such interesting lesson and for make me conscious
that closures exist :)

Best regards,
ABS

[abs]

<ba*********@pandora.bewrote in message
news:11**********************@n51g2000cwc.googlegr oups.com...

Here's one way to solve this:

Big thanks for help.

Best regards,
ABS

[Bill W]

abs wrote:

Hi everyone.

Please, check my test code here: http://skocz.pl/jstest . The trouble is
that no matter which span element I click, it alerts '3' and I'm wondering
why not '1' for the first span, '2' for second and '3' for third ? Anybody
has any idea ?

Best regards,
ABS

The value of i, which is a global variable, at then end of the for loop
is 3. The invocation, testfunction(i) in the onclick event uses the
current value of i, not the value when the onclick event was
established.
As a side note, the values you want will never be displayed in the
alert box, because the variable i has a range of 0-2; at the first
span, i is 0, then 1, and finally 2. The loop terminates when i

>=spans.length, which in this case, is 3.

There are a number of ways to fix the code to do what you want. Here is
one way:

function setOnclicks()
{
spans = document.body.getElementsByTagName('span')
for(i=0; i<spans.length; i++)
{
span = spans[i]
span.onclick = function () {
var x;
x = i + 1;
return function() {testfunction(x)}
}();
}
}
This code calls a function that returns a function using the current
value of i as the value of a local variable, x. The value of x is bound
at the time the function is executed. When the onclick occurs, the
value of x, created at the time the function was returned, is used in
the invocation of testfunction. This is called a closure, you can find
out more about these things at:

http://jibbering.com/faq/faq_notes/closures.html

Hope this helps.

[Evertjan.]

Bill W wrote on 02 jan 2007 in comp.lang.javascript:

There are a number of ways to fix the code to do what you want. Here is
one way:

function setOnclicks()
{
spans = document.body.getElementsByTagName('span')
for(i=0; i<spans.length; i++)
{
span = spans[i]
span.onclick = function () {
var x;
x = i + 1;
return function() {testfunction(x)}
}();
}
}

Why not make the count an attribute of the span?

function setOnclicks() {
spans = document.body.getElementsByTagName('span');
for(i=0; i<spans.length; i++) {
span = spans[i];
span.nmbr = i + 1;
span.onclick = function() {alert(this.nmbr)};
};
};

Tested in IE7.
--
Evertjan.
The Netherlands.
(Please change the x'es to dots in my emailaddress)

[Richard Cornford]

RobG wrote:
<snip>

function setOnclicks() {
var spans = document.body.getElementsByTagName('span')
for(var i=0; i<spans.length; i++) {
span = spans[i]
span.onclick = new Function('testfunction(' + i + ')');
}
}
But new Function is considered only marginally better than using eval.

That would be a matter of opinion. The - eval - function is commonly
abused for tasks that are better done in other ways. The Function
constructor can be similarly abused, but constructing function objects
cannot be an abuse of the Function constructor.

Another way to modify the scope chain is:

function setOnclicks() {
var spans = document.body.getElementsByTagName('span')
for(var i=0; i<spans.length; i++) {
span = spans[i]

Shouldn't - span - be declared as a local variable?

span.onclick = (function(x){
return function(){testfunction(x)};
})(i);
}
}

However I find that exacerbates IE's memory leak problem with closures
involving DOM elements -

<snip>

Closures involving DOM elements are not an issue in IE. Circular chains
of reference including DOM elements provoke the IE memory leak issue.
Closures are just one way if creating such chains, and not always in a
way that is clearly evident.

In this case, after the loop, nulling - span - and - spans - would
ensure that no object on the inner function's scope chain referred to
the elements to which the event handlers had been assigned, and so no
_circular_ chain of references would exist.

Richard.