OnClick only works once

[jamescott]

I've written some simple js to change visibility of some layers and the classes of some links, but the javascript only works the first time it's run... and help would be much appreciated.

The second time I click the link I get 'object doesn't support this property or method' on the line that calls the function?

Link

<li><a href="#" onclick="nextLayer();return false">>></a></li>

Function

function nextLayer(){
TextLayerDisplay = document.getElementById('Text').style.display
Img1LayerDisplay = document.getElementById('Img1').style.display
Img2LayerDisplay = document.getElementById('Img2').style.display
Img3LayerDisplay = document.getElementById('Img3').style.display
Img4LayerDisplay = document.getElementById('Img4').style.display
Img5LayerDisplay = document.getElementById('Img5').style.display
Img6LayerDisplay = document.getElementById('Img6').style.display

if (TextLayerDisplay=='block'){
currentLayer="Text"
nextLayer="Img1"
}
if (Img1LayerDisplay=='block'){
currentLayer="Img1"
nextLayer="Img2"
}
if (Img2LayerDisplay=='block'){
currentLayer="Img2"
nextLayer="Img3"
}
if (Img3LayerDisplay=='block'){
currentLayer="Img3"
nextLayer="Img4"
}
if (Img4LayerDisplay=='block'){
currentLayer="Img4"
nextLayer="Img5"
}
if (Img5LayerDisplay=='block'){
currentLayer="Img5"
nextLayer="Img6"
}
if (Img6LayerDisplay=='block'){
currentLayer="Img6"
nextLayer="Text"
}

document.getElementById(currentLayer).style.display = 'none'
document.getElementById(nextLayer).style.display = 'block'

if (nextLayer == 'Text'){
document.getElementById('LinkText').className = 'current'
document.getElementById('Link' + currentLayer.substring(3)).className = 'none'
} else {

document.getElementById('Link' + nextLayer.substring(3)).className = 'current'

if (currentLayer == 'Text'){
document.getElementById('LinkText').className = 'none'
} else {
document.getElementById('Link' + currentLayer.substring(3)).className = 'none'
}
}

}

[nathandelane]

Is your HTML available also, so that I can run your code in my browser and see what it's doing? Or is your page available online somewhere that I could look at it and test it out in order to see if I can find the problem?

[jamescott]

Thanks for your response, the page can be seen here, http://85.237.201.35/montage/project...bert-docks.asp

It's the >> link in the images menu that I'm having issues with, you can use all of the other links fine (1,2,3,4,5,6,info) and click the >> once, at any point, but the second time it's clicked is when I'm having the problem.

All that it's meant to do is change the layer (e.g. if Img1 display = block then Img1 display = none; Img2 display = block) & change the link class (If Img1 display = block then Link1 class=none; Link2 class=current)

[nathandelane]

Thanks for the link. First off, let me tell you that the more and more I look at the swapLayer function you have there, the more I am concerned that it isn't written properly. So let me ask you about how the content of that page is laid out. Are all of the images already a part of the page, and just some of them are hidden by the style display: none;. If that is not the case, can you tell me how the images are loaded? Just brainstorm with me for a minute.

Let's assume that all of your images are basically placed on your page like this:

<style type="text/css">
.hidden
{
	display: none;
}
</style>
.
.
.
<div id="image1" class="">
	<img src=".."/>
</div>
<div id="image2" class="hidden">
	<img src=".."/>
</div>
<div id="image3" class="hidden">
	<img src=".."/>
</div>

Then what I would do is place a page state variable somewhere, say next to my function, displayNextImage. And then I'll have my function displayNextImage. Now I'll also know how many images I have on the page, in this case I have 3. So:

var currentImage = 1;
var numberOfImages = 3;

function displayNextImage() {
	if(currentImage == numberOfImages) {
		currentImage = 1;
	} else {
		currentImage++;
	}

	document.getElementById("image" + currentImage).className = "";
}

Now of course I want to also hide the previous images, so I create a method named hidePreviousImage like this:

function hidePreviousImage() {
	var previousImage = 0;
	if(currentImage == 1) {
		previousImage = numberOfImages;
	} else {
		previousImage = currentImage - 1;
	}

	document.getElementById("image" + previousImage).className = "hidden";
}

Then tuck a call to that function in your displayNextImage function like this:

function displayNextImage() {
	if(currentImage == numberOfImages) {
		currentImage = 1;
	} else {
		currentImage++;
	}

	document.getElementById("image" + currentImage).className = "";
	hidePreviousImage();
}

Does that help simplify the process a little? Now if this is not your layout then all is not lost, you still want a global variable identifying the total number of images, one identifying the current image, and a function to display the next image and one to hide the previous image.

[jamescott]

All sorted, many thanks...

The issue was that the project had ballooned, and I had just added on to my previous script rather than re-write it from scratch.

Due to the fact that I needed links to the individual layers as well as the next link, I amended your code to the following...

var currentLayer = 1;
var numberOfLayers = 7;

function displayLayer(newLayer) {
	
	if (newLayer == 'next'){
		displayNextLayer()
	} else {
		newLayerNumber = newLayer.substring(4)
		if (newLayerNumber == currentLayer){
			
		} else {
			document.getElementById("Layer" + newLayerNumber).className = "";
			document.getElementById("Link" + newLayerNumber).className = "current";
			document.getElementById("Layer" + currentLayer).className = "hidden";
			document.getElementById("Link" + currentLayer).className = "";
			currentLayer = newLayerNumber;
		}
	}
}

function displayNextLayer() {
	if(currentLayer == numberOfLayers) {
		currentLayer = 1;
	} else {
		currentLayer++;
	}
	document.getElementById("Layer" + currentLayer).className = "";
	document.getElementById("Link" + currentLayer).className = "current";
	hidePreviousLayer();
}

function hidePreviousLayer() {
	var previousLayer = 6;
	if(currentLayer == 1) {
		previousLayer = numberOfLayers;
	} else {
		previousLayer = currentLayer - 1;
	}
	document.getElementById("Layer" + previousLayer).className = "hidden";
	document.getElementById("Link" + previousLayer).className = "";
}

then changed my links to

<li><a id="Link2" onclick="displayLayer(this.id)">1</a></li>
            	<li><a id="Link3" onclick="displayLayer(this.id)">2</a></li>
            	<li><a id="Link4" onclick="displayLayer(this.id)">3</a></li>
            	<li><a id="Link5" onclick="displayLayer(this.id)">4</a></li>
            	<li><a id="Link6" onclick="displayLayer(this.id)">5</a></li>
            	<li><a id="Link7" onclick="displayLayer(this.id)">6</a></li>
            	<li><a href="#" onclick="displayLayer('next');return false">>></a></li>
            	<li><a class="current" id="Link1" onclick="displayLayer(this.id)">info</a></li>

and my html to

<div id="Layer1">
	<h1>Title</h1>
	<p>Lorem ipsum dolor sit amet...</p>
</div>

 <img src="/graphics/Img1.jpg" width="960" height="500" id="Layer2" class="hidden" />
 <img src="/graphics/Img2.jpg" width="960" height="500" id="Layer3" class="hidden" />
 <img src="/graphics/Img3.jpg" width="960" height="500" id="Layer4" class="hidden" />
 <img src="/graphics/Img4.jpg" width="960" height="500" id="Layer5" class="hidden" />
 <img src="/graphics/Img5.jpg" width="960" height="500" id="Layer6" class="hidden" />
 <img src="/graphics/Img6.jpg" width="960" height="500" id="Layer7" class="hidden" />

[nathandelane]

I'm glad I could help you out. You have a very nice site by the way.