Assigning the Onclick to the HTML dom using javascript

This site is best viewed in a modern browser with JavaScript enabled.

Assigning the Onclick to the HTML dom using javascript

B_vidler

Hi all,

I have the following code snippet that i am having some trouble with.

<script>

var holding = document.getElementById("chooseframe"); // Contains outer div element //

var imageholders = [];
for(var i=0;i<images.length;i++)   // loops through all images in the images array //
{
imageholders[i] = document.createElement("IMG");
imageholders[i].src=images[i];
imageholders[i].className = "tabs";
imageholders[i].onclick = function()  {
	document.slideshow1.src=images[i]; //makes the slideshow image the image of the current IMG being clicked //
	document.slideshow.src=images[i];  //multilayer slideshow so does the same thing as the above code  //
	window.clearInterval(pauseme);      // again, clears variables used in the slideshow //
	window.clearInterval(runfront);
	window.clearInterval(runback);
	};

holding.appendChild(imageholders[i]);  //displays the IMG elements created as children of the first variable //
}

var nodes = document.getElementById('chooseframe').children; //this part of the code is irrelevant really as all this is doing is sizing the elements depending on how many elements there are in the imageholders array //
var amount = 100/nodes.length;
for(w=0;w<nodes.length;w++)
{
nodes[w].style.height=amount+"%";
}

</script>

The problem here is the

imageholders[i].onclick = function()  {
	document.slideshow1.src=images[i]; //makes the slideshow image the image of the current IMG being clicked //
	document.slideshow.src=images[i];  //multilayer slideshow so does the same thing as the above code  //
	window.clearInterval(pauseme);      // again, clears variables used in the slideshow //
	window.clearInterval(runfront);
	window.clearInterval(runback);
	};

I assumed at first that the onclick would get the dynamic value of i as everything else does. However, I started to get that the function was not working like that but was instead seeing i when then function was actually being called (so i = 4). I have tried so many ways to fix this, looked up quite a few old posts aswell but cannot seem to figure out a way to fix this for my code.

Can anyone help to resolve this and if so please explain how the "fixed" code works.

Thanks to anyone and everyone who helps

ps: whole code can be seen on my test site here http://englishseadragon.appspot.com/ (please ignore the pictures, )

bionoid

Untested, but try this (changes are in red):

<script>

var holding = document.getElementById("chooseframe"); // Contains outer div element //

var imageholders = [];

[COLOR="#FF0000"]function imageHolderClick(i)
{
	return function()
	{
		document.slideshow1.src=images[i]; //makes the slideshow image the image of the current IMG being clicked //
		document.slideshow.src=images[i];  //multilayer slideshow so does the same thing as the above code  //
		window.clearInterval(pauseme);      // again, clears variables used in the slideshow //
		window.clearInterval(runfront);
		window.clearInterval(runback);
	};
}[/COLOR]
for(var i=0;i<images.length;i++)   // loops through all images in the images array //
{
imageholders[i] = document.createElement("IMG");
imageholders[i].src=images[i];
imageholders[i].className = "tabs";
imageholders[i].onclick = [COLOR="#FF0000"]imageHolderClick(i);[/COLOR]

holding.appendChild(imageholders[i]);  //displays the IMG elements created as children of the first variable //
}

var nodes = document.getElementById('chooseframe').children; //this part of the code is irrelevant really as all this is doing is sizing the elements depending on how many elements there are in the imageholders array //
var amount = 100/nodes.length;
for(w=0;w<nodes.length;w++)
{
nodes[w].style.height=amount+"%";
}

</script>

B_vidler

Hi bionoid,

I inputted your code and it works perfectly, the only problem now (and, in a way, the biggest problem) is that i don't understand HOW that works. could you or someone else explain the code?

how i currently see that code:

imageHolderClick(i); is called so the value or 1,2,3 or 4 is given to the functions operator (simple stuff)
return function(); I have never come across this "return function()" code before today, and for what i have seen this is used to store the value of the "wrapped" function. so it returns the function with the correct i value (in this case) 4 times and the i operator simply makes the images array correct. Assuming i am correct there, the one thing i still dont understand is how the return value is held each time instead of being overwritten by the next returned value and therefore ending up like my code was before (only showing the end loop).

bionoid

Hi, I assume you haven't uploaded the changes to the live site yet. Love the wallpapers BTW.

When I return a function from within another function, it is called a closure. It basically creates a new scope.
Because "i" is a Number, it is not passed by reference, so the newly declared "i" within the imageHolderClick function will remain unchanged (unless the members within the closure change it of course). Remember that it is not the same "i" between calls, and therefore do not overwrite each others value.

We could have done the same thing with an anonymous function call, but it's not really considered good practise to create functions within a loop, even though this is almost the same thing

Some reading if I suck at explaining things: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Closures

007Julien

Short explanation : With your script the variable i is unknown when the click event occurs !
With the closure, Bionoîd build a function for each variable i !

B_vidler

I kinda understand, it would be a lie to say i fully understand how closures work even after studying that link quite hard.

How bionoid's function works however, i understand now so thanks .

Sadly, I cannot update my site as it is located on a google drive with settings i had on my old computer and im having some problems getting google apps to work on my new one.
(if this changes i will post back here)

Thanks guys

B_vidler

Hi all,

Iv been shaping up some on my old code lately (after about a year of practice)

This code is one of those pieces but it seems this never wants to work without some little glitch .

Here is the new code

Old code is this post

<script>
function slideshow()
{
	var images = [
		'Images/PokemonImage1.jpg',
		'Images/PokemonImage2.jpg',
		'Images/PokemonImage3.jpg',
		'Images/PokemonImage4.jpg',
		'Images/PokemonImage5.jpg',
	 ];

var run = FadeImages(0,1);

function FadeImages(Image1Number,Image2Number)
{
  var FadeAmount = 0;
  var StartFade = window.setInterval(function()
  {
	FadeAmount = FadeAmount + 0.005
	var pausevalue = document.getElementById('pause').value;
	if(pausevalue == 1)
	{
	  SwitchImg(Image1Number,Image2Number,false)
	}
	else
	{
	  var FadeOut = function(){
	    document.getElementById('image1').style.opacity = (1-FadeAmount).toFixed(2);
	  }
	  FadeOut();
	  var FadeIn = function(){
	    document.getElementById('image2').style.opacity = FadeAmount.toFixed(2);
	  }
	  FadeIn();
	  if(FadeAmount >= 1)
	  {
	    window.clearInterval(StartFade);
	    SwitchImg(Image1Number,Image2Number,true)
	  }
	}
  },10);
}

function SwitchImg(Image1Number,Image2Number,run)
{
  var NewImage2Number;
  var img1 = document.getElementById('image1') 
  var img2 = document.getElementById('image2') 
  if(Image2Number == images.length-1)
  {
	NewImage2Number = 0;
  } 
  else
  {
	NewImage2Number = Image2Number + 1;
  } 
  img1.src=images[Image2Number]; // Change so that both images = the one being shown//
  img1.style.opacity = 1; // Change so that the new image is now on top (so to speak) //
  img2.style.opacity = 0; //Change so that the new image (when switched in) will be un-visible //
  img2.src=images[NewImage2Number]; // switch in new image //
  if(run == true)
  {
  FadeImages(Image2Number,NewImage2Number);
  }
}
}

slideshow();

function ChangeValue(elm,value,elmInnerHtml)
{
  elm.value=value;
  elm.innerHTML = elmInnerHtml;
}

</script>
<body>
<div onclick="ChangeValue(this,1,'Paused')" ondblclick="ChangeValue(this,0,'Running..')" id='pause' value='0'>
PauseMe
</div>
<img src='Images/PokemonImage1.jpg' style='position:absolute;top:0xp;left:0px;width:300px;height:300px;' id='image1'>

<img src='Images/PokemonImage2.jpg' style='position:absolute;top:0xp;left:0px;width:300px;height:300px;' id='image2'>


</body>

Hopefully you will agree this is a lot better than my old code but if you run this (you will need to pop some images in) you may notice a small glitch.

Can't see it? well then you are one of 4 things:

Using Google Chome and are blind :S
Using IE
Using Firefox and haven't looked hard enough
to lazy to put images in ...

In case you don't have google chrome (i mean really?). The problem is such: When the switch function is called ever now and again (for what appears to me to be random ( i know its code, that does not happen ) the img1 will have its opacity changed BEFORE the new image is switched over. This results in the old image flashing quickly onto the screen.

In IE this simply doesn't happen (IE is better? wait ... what?)

In FireFox on the 3 image there is a slight judder on the image but it works correctly (thinking this is the clue here).

Anyone wanna have a stab at whats going on here?

Iv commented the important lines of code with how I see the logic

	  img1.src=images[Image2Number]; // Change so that both images = the one being shown//
	  img1.style.opacity = 1; // Change so that the new image is now on top (so to speak) //
	  img2.style.opacity = 0; //Change so that the new image (when switched in) will be un-visible //
	  img2.src=images[NewImage2Number]; // switch in new image //

its almost as if chrome is getting ahead of itself and tripping over.

Anyway, i'm rambling. Any improvements, as always, would be great and if you think you know what the problem is ... well your a genius.

O! and if you put an alert in this middle of the opacity changing, it works correctly .... yer ....

Thanks!

deathshadow

Well, I see a typo... you have xp instead of px...

Really though, using the STYLE attribute in the markup is sloppy poor coding; get that out into a stylesheet where it belongs. (I had my way STYLE as a tag would be obsoleted, as an attribute it would be deprecated)

Of course most of what you're doing there is now CSS3's job.

... but I think you forgot to actually link to / include your 'new code'.

B_vidler

Hi Deathshadow,

The typos and the style tag are both irrelevant as both are in a style sheet in the actual code. I simply put that in for anyone who wanted to load it here.

I haven't looked into CSS3 anywhere near as much as maybe i should, however, this was more to see how much i had improved.

Finally the new code was in my last post? Not sure how you couldn’t see that Although, admittedly i have posted this in the wrong (old) post. I posted two posts back when this was uploaded and i mistook this one with the other.

That aside however, I have managed to "fix" this problem by wrapping the 3 lines after the img 1 image is set to the img 2 image in a window.setTimeout function. (with a tiny 1 million second delay)

Doing this seems to give google chrome the breathing space it needs to get its feet .

deathshadow

I think I misread your post. I have a good deal of difficulty deciphering broken engrish. You said "this is the new code", but then had a double break and then text saying what followed was the old code... So.. that's' the NEW code?

Really, if that's the new code you seem to be changing out img elements off a list instead of just loading the images normally in the markup, having static scripting only elements (the pause/play) in the markup, and so forth...

Want a laugh? Check out the code for mine:
http://www.elementalsjs.com/demos/flipperTest.html

A bit more complex since it handles multiple animation types; uses CSS3 to do all the heavy lifting of fades and slide in/out. Only thing the JS handles is the delay between slides, play/pause, selection buttons and the timer status updates... to make a slide change, it just replaces classes on the elements -- the elements to be animated simply being pulled from the DOM instead of by the script.

Has the advantage that if scripting is disabled, all the images are just loaded and shown.

deathshadow

Oops, forgot the explanation page:
http://www.elementalsjs.com/example.eFlipper