views:

1700

answers:

2

I'm working with a bit of html and Javascript code that I've taken over from someone else. The page reloads a table of data (via an asynchronous request) every ten seconds, and then re-builds the table using some DOM code. The code in question looks something like this:

var blah = xmlres.getElementsByTagName('blah');
for(var i = 0; i < blah.length; i++) {
    var td = document.createElement('td');
    var select = document.createElement('select');
    select.setAttribute("...", "...");
    select.onchange = function() {
        onStatusChanged(select, callid, anotherid);
    };
    td.appendChild(select);
}

When the onchange event is fired for a <select> element however, it seems like the same values are being passed to the onStatusChanged() method for every <select> in the table (I've verified that in each iteration of the loop, callid and anotherid are being given new, distinct values).

I suspect this is occuring because of the nature of how I am setting the event handler, with the select.onchange = function() syntax. If I understand how this is working correctly, this syntax sets a closure for the onchange event to be a function which refers to these two references, which end up having a final value of whatever they are set to on the last iteration of the loop. When the event fires, the value referenced by callid and anotherid is the value set in the last iteration, not the value set at the individual iteration.

Is there a way that I can copy the value of the parameters I am passing to onStatusChanged()?

I've changed the title to better reflect the question and the accepted answer.

+10  A: 

You do, indeed, need to implement a closure here. This should work (let me know - I didn't test it)

var blah = xmlres.getElementsByTagName('blah');
for(var i = 0; i < blah.length; i++) {
    var td = document.createElement('td');
    var select = document.createElement('select');
    select.setAttribute("...", "...");
    select.onchange = function(s,c,a)
    {
        return function()
        {
            onStatusChanged(s,c,a);
        }
    }(select, callid, anotherid);
    td.appendChild(select);
}
Peter Bailey
this does indeed work - thanks!
matt b
Also another solution I discovered - a workaround, really - would be to store the callid and anotherid as attributes in the DOM element, and inside the eventhandler just reference those via this.getAttribute()
matt b
I wouldn't call it a workaround. What I would expect is to have the handler set once (not every time through the loop) and have it discover the variables itself.
le dorfier
A: 

This is the right answer. Had a similar issue and the closure solution was the ticket.

Adrian