CSS - js rendering links useless

Question

Ok, well, thanks to people here, I figured out the portion of my js code that's rendering my links useless is what's pasted below... Still trying to figure out why, but if someone has any insight, that'd be great...

function initJumpMenus() {
    // Turns all <select> elements with the 'jumpmenu' class into jump menus
    var selectElements = document.getElementsByTagName("select");
    for( i = 0; i < selectElements.length; i++ ) {
        // Check for the class and make sure the element has an ID
        if( selectElements[i].className == "jumpmenu" &&
            document.getElementById(selectElements[i].id) != ""
        ) {
            jumpmenu = document.getElementById(selectElements[i].id);
            jumpmenu.onchange = function() {
                if( this.options[this.selectedIndex].value != '' ) {
                    // Redirect
            location.href=this.options[this.selectedIndex].value;
                }
            }
        }
    }
}

window.onload = function() {
    initJumpMenus();
}

Answer 1

This:

document.getElementById(selectElements[i].id) != ""

is wrong. You want to check if the element has an id, so simply do:

selectElements[i].id != ""

I'd like to point out that you can improve your code here and there:

1 Don't write for loops that get the length property for each iteration,

instead do:

for( i = 0, num = selectElements.length; i < num; i++ ) {
    ...
}

Only if you expect selectElements to grow or shrink it would make sense to requery the property value, but in that case, you probably should not write a for loop anyway.

2: Don't write nodelist[index]

For nodelists, such as returned by getElementsByTagName() you should not write nodelist[index] (even though most browser support that). The standard DOM method is item, so write nodelist.item(index) instead.

3 fetch items from the list only once

If you need an item from the list more than once, store it in a local variable. your loop would become:

for( i = 0, selectElement; i < selectElements.length; i++ ) {
    selectElement = selectElements.item(i);
    ...more code using selectElement...
}

Note the declaration of the selectElement variable in the for loop. since you don't use it outside the loop, declaring it there avoids clutter and ensures that if you move the loop, you move the declaration.

4. cheapest comparisons first

You wrote:

selectElement.className == "jumpmenu" &&
selectElement.id != ""

this could be slightly improved if you reverse the legs:

selectElement.id != "" &&
selectElement.className == "jumpmenu"

This will be faster, since it is less work to check if a string is empty, and for those cases where the string is empty, we won't even check the className

5 Don't use document.getElementById() if you already have the element

Inside the loop you have this:

jumpmenu = document.getElementById(selectElements[i].id);

You are basically getting the id from the selectElement and use that to query the document to find ....the element having and id equal to that of the current selectElement. Becuase id's are unique within the document (or should be), you are basically writing a completely unecessary sentence. jumpmenu and selectElement refer to one and the same object.

6. onchange handler improvements

inside the loop you assign an onchange handler. you do so by creating a new function for each loop iteration. This is the handler code:

function() {
    if( this.options[this.selectedIndex].value != '' ) {
        // Redirect
        location.href=this.options[this.selectedIndex].value;
    }
}

Three things are of note here: First, the onchange handler code contains this location.href = ... that should probably be document.location.href = ....

Second, you refer twice to this.options[this.selectedIndex].value. Again, put this in a local variable.

Third, the this refers to the element that experienced the onchange event by the time this function is executing. Other than this and properties of this, there are no variables in this handler that originate from the loop or the outer initJumpMenus function. You should simply create it once, outside the loop, and assign it every iteration:

var onchange_handler = function() {
    if( this.options[this.selectedIndex].value != "" ) {
        // Redirect
        location.href=this.options[this.selectedIndex].value;
    }
}
for (...) {
    if (...) {
        selectElement.onchange = onchange_handler;
    }
}

7 Summary

Putting it all toghether, this is how I would write it:

function initJumpMenus() {
    var handler = function() {
        var value = this.options.item(this.selectedIndex).value;
        if( value != "" ) {
            // Redirect
            document.location.href = value;
        }
    }
    var selectElements = document.getElementsByTagName("select");
    for(var i = 0, num = selectElements.length, selectElement; i < num; i++ ) {
        selectElement = selectElements.item(i);
        // Check for the class and make sure the element has an ID
        if( selectElement.id != "" &&
            selectElement.className == "jumpmenu"
        ) {
            selectElement.onchange = handler;
        }
    }
}

Answer 2

Check the error console; any exception in the JS will stop the links form reacting to a click.