1

I am building a rails application and am having a problem setting the onclick function for each table row. The table gets built dynamically and I would like each row to return an id field for further processing. The problem I am having is each row returns the last id.

Pardon my example at http://jsfiddle.net/vz7beLkg/1/ since it looks like crap. I had to make numerous modifications to get it to work in the fiddle. But it demonstrates the problem I am having. If you click a row in the top section you always get 11. I use a loop to assign counters and it appears a new row gets created because all the counters appear on screen.

for (r = 0; r < 11; r++) {
    var newRow = table.insertRow(table.rows.length);
    newRow.onclick = function () { gatherImageData(r); };
    var newCell  = newRow.insertCell(0);
    var newText  = document.createTextNode(r);
    newCell.appendChild(newText);
    var newCell  = newRow.insertCell(1);
    var newText  = document.createTextNode('r' + r);
    newCell.appendChild(newText);
}

Why does it behave in the way? Without knowing why I may make the same mistake again.

EDIT: The gatherImageData function will create a table in the lower portion of the page based on the id. There will be a list of ids that can change in the upper portion.

EDIT: I tried using the approach offered by Abdul but it produces the same error; .http://jsfiddle.net/vz7beLkg/5/
I added an assignment to the loop variable after the loop; r = 99. Now all the rows use this value.

How do I use a value in the loop to pass to each rows click event? It seems the click events are being modified after assignment. If I make it loop once the onclick returns 1. If I make it loop twice all rows return 2 (looping 0 then 1 then setting the loop var to 2). So it seems the onclick event is being modified AFTER it is being set with

newRow.onclick = function () { gatherImageData(r); };

How do I stop if from changing OR How do I properly assign it so it does not change AFTER I set it OR Why are all click events the same when a new row object appears to be created?

It seems either a new row object is being created sionce there are new rows in the table. Unless new rows are being inserted but the newRow object is pointing to all of them.

Joe
  • 379
  • 1
  • 6
  • 21
  • possible duplicate of [JavaScript closure inside loops – simple practical example](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) – Teepeemm Jul 29 '15 at 15:38
  • All the rows use the 99 value because you forgot to comment out `newRow.onclick = function () { gatherImageData(r); };` right after you inserted `setRow(newRow, r);` – Honinbo Shusaku Jul 29 '15 at 16:05

2 Answers2

1

I'm not entirely sure if I understood your problem, but I changed several things in your JSFiddle, and it seems to alert with the correct number: http://jsfiddle.net/vz7beLkg/2/

Your loop:

for (r = 0; r < 11; r++) {
                var newRow = table.insertRow(table.rows.length);
                gatherImageData(r, newRow); //changed line
                var newCell  = newRow.insertCell(0);
                var newText  = document.createTextNode(r);
                newCell.appendChild(newText);
                var newCell  = newRow.insertCell(1);
                var newText  = document.createTextNode('r' + r);
                newCell.appendChild(newText);
            }

gatherImageData:

function gatherImageData(id, row) {
    //changed from just alert("build table for " + id);
    row.onclick = function () { alert("build table for " + id); };

}

I changed newRow.onclick = function () { gatherImageData(r); }; to gatherImageData(r, newRow); injecting the current row object and the current index r. This got rid of attaching the onclick event, which i attach in the gatherImage function.

I changed gatherImageData function to attach the alert() action to the row node injected into it.

Honinbo Shusaku
  • 1,411
  • 2
  • 27
  • 45
  • Good answer, it works with an alert but I don't think it will work in production. The gatherImageData function builds a table based on the id passed in. The id gets created in the javascript that I posted, it is not static which I believe you approach uses since my example was static. I'll update the question. – Joe Jul 29 '15 at 15:46
  • @Joe "list of ids", couldn't you just pass the list into the function, and then instead of just `r` or `id`, you could just reference the list,using the index, inside the function? I've used an array with random numbers, and the numbers change everytime the function is called, and it seems to be working for me, because I pass it in – Honinbo Shusaku Jul 29 '15 at 16:01
  • I may try it but I really want to know whats wrong with my logic. One problem is the list changes. Another is I would have identical copies of data in memory. Also I would have to do a lookup on the click event when it appeared more efficient to just pass the know value. Plus I need to do the same with the lower table which contains much more data. Then I will have two tables with two identical lists in memory doing lookups that may be unnecessary. Working inefficiently, however is better than not working at all. – Joe Jul 29 '15 at 16:13
0

Based on Abdul's answer and the post JavaScript closure inside loops – simple practical example I was able to fix my error: http://jsfiddle.net/vz7beLkg/7/

With

function createWholeNumberClick(wholenumber) {
    return function() { gatherImageData(wholenumber); };
}

and

for (r = 0; r < 11; r++) {
    var newRow = table.insertRow(table.rows.length);
    newRow.onclick = createWholeNumberClick(r);
    var newCell  = newRow.insertCell(0);
    var newText  = document.createTextNode(r);
    newCell.appendChild(newText);
    var newCell  = newRow.insertCell(1);
    var newText  = document.createTextNode('r' + r);
    newCell.appendChild(newText);
}

Thanks for all who helped!!

Community
  • 1
  • 1
Joe
  • 379
  • 1
  • 6
  • 21