Why is the onclick only being assigned to the last object?

Question

I am trying to assign an onclick function a certain type of item. So, within a for loop I have if(invType[i] != "empty") then within that if statement I have

if(invType[i] == "usable"){
    (function(num) {
       document.getElementById(num).onclick = function(){useItem(invName[num],num)};
    })(i);
}

only the last "usable" object is getting the onclick function. Everyone keeps linking me to this question, but that doesn't help me. I have tried everything and I can't figure it out. Can someone please give me a better explanation for what all that means and how I can get my code to work?

JSFiddle

Here's the whole code that adds all the buttons:

function backpack(update) {
    i = 0;
    if (update != 1) {
        /*
                    tempA = document.getElementById("scene").innerHTML;
                    tempB = document.getElementById("textbox").innerHTML;
                    */
    }
    /*
                document.getElementById("textbox").innerHTML = '<br><br><input type="button" value="Close Backpack" onclick="closeBackpack()">';
                document.getElementById("scene").innerHTML = '<div id="backpack" style="height:400px; width:400px; background-image:url(files/sugame/sprites/backpackBackground.png); border-style:solid; border-width:8px; position:absolute; top:92px; left:192px"></div>';
                */
    for (i = 0; i < 9; i++) {
        if (invType[i] != "empty") {
            switch (i) {
                case 0:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:4px; top:4px;">';
                    break;
                case 1:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:136px; top:4px;">';
                    break;
                case 2:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:268px; top:4px;">';
                    break;
                case 3:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:4px; top:136px;">';
                    break;
                case 4:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:136px; top:136px;">';
                    break;
                case 5:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:268px; top:136px;">';
                    break;
                case 6:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:4px; top:268px;">';
                    break;
                case 7:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:136px; top:268px;">';
                    break;
                case 8:
                    document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:268px; top:268px;">';
                    break;
            }
            if (invType[i] == "usable") {
                (function (num) {
                    document.getElementById(num).onclick = function () {
                        useItem(invName[num], num)
                    };
                })(i);
            }
            if (invType[i] == "equipment") {
                document.getElementById(i).onclick = function (i) {
                    return function () {
                        equipItem(invName[i], i)
                    };
                }(i);
            }
        }
    }
}

I don't know why you are using an IIFE in your script, btw can you post more of your code, a jsfiddle for example? — ale, Sep 23 '15 at 18:01
@WinterMute Because: [Javascript infamous Loop issue?](http://stackoverflow.com/questions/1451009/javascript-infamous-loop-issue) — Andreas, Sep 23 '15 at 18:03
ok, but I think that the problem could be an element with id = '1'; doesn't fit very well with a valid id — ale, Sep 23 '15 at 18:05
@WinterMute in html5 the only rules for the id is that it must be at least 1 character and cannot contain spaces — isaac tschampl, Sep 23 '15 at 18:07
@isaactschampl also start with number?, omg I've lost something — ale, Sep 23 '15 at 18:07
@WinterMute yes it can start with a number the id isn't the problem, its assigning totally find to the last "usable" object whose id would also have started with a number — isaac tschampl, Sep 23 '15 at 18:10
Can you make a jsfiddle that demonstrates the problem? The code you've posted looks OK, the problem is probably somewhere else. — Barmar, Sep 23 '15 at 18:11
@Barmar well the onclick is attached to an image not a button so I had to modify it a little bit, and then the buttons wouldnt show up on the jsfiddle, but at least you can look at the code a little bit more in detail there http://jsfiddle.net/s7d2ah1h/3/ Edit: Just kidding i forgot to call the function the buttons show up fine. — isaac tschampl, Sep 23 '15 at 18:28
probably because your elements are having the same ID, ID should be unique... — jpganz18, Sep 23 '15 at 18:36

score 2 · Accepted Answer · answered Sep 23 '15 at 18:50

2

The problem is with the way you're adding all the buttons to the DOM. You're doing:

document.getElementById("backpack").innerHTML += '<input type="button" value="' + invName[i] + '" id="' + i + '" style="position:absolute; left:4px; top:4px;">';

This re-parses the HTML of the entire DIV, so any event listeners on the old buttons are lost.

There are two simple ways to fix this:

Instead of concatenating HTML, use functions like createElement and appendChild to add to the DOM directly, without affecting previous elements.
Use two loops: one loop adds all the buttons, the second loop adds the event listeners to them.

Another thing I noticed, but it's not related to the problem, is that you forget to declare i as a local variable in your functions. This can cause problems if you call other functions inside the loop that do the same thing, because they'll affect the outer loop.

answered Sep 23 '15 at 18:50

Barmar

741,623
53
500
612

I may be misunderstanding that, but I want it to replace the old buttons and then add new event listeners every time the backpack function is called, because the inventory can change and then I need it to have the correct button in the inventory spot, and have that button do the appropriate action. In my actual code those commented out lines arent commented out, so that div only exist when I do the backpack function. – isaac tschampl Sep 23 '15 at 19:08
The problem is that you're replacing the entire DIV each time through the loop. So when you add button 1, you recreate button 0 as well. And when you add button 2, you recreate buttons 0 and 1. And so on. – Barmar Sep 23 '15 at 19:16
It's fine to remove the old HTML before the loop, the problem is that you do it during the loop as well. – Barmar Sep 23 '15 at 19:18
But that code is only runs if it matches the case for switch(i) so it should only create one button each time, right? or am I misunderstanding how switch / case works? – isaac tschampl Sep 23 '15 at 19:32
The way you create each button also recreates all the previous buttons. When you do `div.innerHTML += something`, it converts the contents of the DIV into an HTML string, concatenates `something` to the string, then parses that HTML string back into DOM elements and puts them into the DIV. – Barmar Sep 23 '15 at 19:40
Oh I didn't know that. Thank you so much! – isaac tschampl Sep 23 '15 at 19:44
1

Remember, `a += b` is just short for `a = a + b`. – Barmar Sep 23 '15 at 19:45

Why is the onclick only being assigned to the last object?

1 Answers1