CODEWITHSUNDEEP
javascriptonclick
YsoL8
YsoL8

Reputation: 2214

JS onclick not firing correctly

I have the following function. The problem is that instead of waiting for the user to click the image as expected, the function immediately fires the imgReplace function for each element in the images array.

Have I done something wrong? Could the fact I'm using a separate Javascript routine based on Jquery be relevant here? 

function setup () {
    var images = document.getElementById("mycarousel");
    images = images.getElementsByTagName("img");

    for (var i = 0; i< images.length; i++) {
        images[i].onclick = imgReplace (images[i]);
    }

}

Upvotes: 1

Views: 397

Answers (3)

Pointy
Pointy

Reputation: 413702

Wow I just fixed this embarrassing bug in some of my own code. Everybody else has gotten it wrong:

images[i].onclick = function() {imgReplace(images[i]);};

won't work. Instead, it should be:

images[i].onclick = (function(i) { return function() { imgReplace(images[i]); }; })(i);

Paul Alexander's answer is on the right track, but you can't fix the problem by introducing another local variable like that. JavaScript blocks (like the {} block in the "for" loop) don't create new scopes, which is a significant (and non-obvious) difference from Java or C++. Only functions create scope (setting aside some new ES5 features), so that's why another function is introduced above. The "i" variable from the loop is passed in as a parameter to an anonymous function. That function returns the actual event handler function, but now the "i" it references will be the distinct parameter of the outer function's scope. Each loop iteration will therefore create a new scope devoted to that single value of "i".

Upvotes: 4

jcane86
jcane86

Reputation: 681

What you want to do here is:

images[i].onclick = function() {imgReplace(images[i]);}

try that.

Cheers

Upvotes: -1

Paul Alexander
Paul Alexander

Reputation: 32367

Your assigning the result of the call to imageReplace to the onclick handler. Instead wrap the call to imageReplace in it's own function

images[i].click = function(){ imgReplace( images[i] ) }

However, doing so will always replace the last image. You need to create a new variable to enclose the index

for (var i = 0; i< images.length; i++) {
    var imageIndex = i;
    images[i].onclick = function(){ imgReplace (images[imageIndex]); }
}

Upvotes: -1

Related Questions