Advice needed for scopes in JavaScript

Question

I would like to connect this "JS" to Bugzilla (example: bugzilla.mozilla.org or landfill.bugzilla.org).

I started to learn JS language today and I would like to ask you:

1. How can I not do bad things in global scope?
2. How should I use functions (in var or not)?

quicksearch = document.getElementById('quicksearch_top');
comment = document.getElementById('comment');

severity = document.getElementById('bug_severity');
priority = document.getElementById('priority');

commit_top = document.getElementById('commit_top');
commit = document.getElementById('commit'); 

function focusonload() { 
// may be it should be
// in next function?

    function cursorfocus(s) {
        x = window.scrollX;
        y = window.scrollY;
        s.focus();
        window.scrollTo(x, y);
    }

    if (comment !== null) {
        cursorfocus(comment)
    } else {
        quicksearch.focus();
    }
}

function navigation(keypressed) { 

    keypressed = keypressed || window.event;

    function selectelement(w, select) {
        w.value = select;
    }

    keyCode = keypressed.keyCode || keypressed.which,
    kn = {
        enter: 13, 
        save: 83, 
        down: 40, 
        up: 38, 
        p1: 49, 
        p2: 50, 
        p3: 51, 
        p4: 52, 
        p5: 53, 
    };

    if (keypressed.altKey) {
        if (keyCode == nk.save && commit_top!==null){
            commit_top.click();
        }

        var key_str;
        if (priority!==null){
            switch (keyCode) {
                case kn.p1: key_str = "P1"; break;
                case kn.p2: key_str = "P2"; break;
                case kn.p3: key_str = "P3"; break;
                case kn.p4: key_str = "P4"; break;
                case kn.p5: key_str = "P5"; break;
            }
        }
        selectelement(priority, key_str);
        severity.focus();

    } else if (keypressed.ctrlKey) {
        switch (keyCode) {
            case kn.enter:
                if (commit!==null&&comment.value!=="") {
                    commit.click();
                }
                break;
            case kn.up:
                if (quicksearch!==null) {
                    quicksearch.focus();
                }
                break;
            case kn.down:
                if (comment!==null) {
                    comment.focus();
                }
                break;
        }
    }
}

focusonload();
document.onkeydown = navigation;
// how to call this functions when page
// would be loaded at all (with window.onload)

Answer 1

How can I not mess up the global scope?

You can use an IIFE ( Immediately Invoked Function Expression ) to surround your code and then assign all your variables with var <- Very important.

(function(){

  var quicksearch = document.getElementById('quicksearch_top');
  var comment = document.getElementById('comment');

  var severity = document.getElementById('bug_severity');
  var priority = document.getElementById('priority');

  var commit_top = document.getElementById('commit_top');
  var commit = document.getElementById('commit'); 

  //the rest of your code here

}()); 

How should I use functions (in var or not)?

It really is up to you, at this point I prefer function.

Other than that:

You should use lowerCamelCasing, so

• commit_top -> commitTop
• quicksearch -> quickSearch
• selectelement -> selectElement

etc.etc

Your keycode map is very nice, keep it that way!

One last comment is that you should not use document.onkeydown = navigation;, there is a good chance that you will break something this way.

Try document.addEventListener("keydown", navigation, true); instead.

Answer 2

To add a couple of points to konijns answer;

You can remove the need for verbose switch statements with a dict:

var kn = {
    49: 'P1', 
    50: 'P2', 
    51: 'P3', 
    52: 'P4', 
    53: 'P5', 
};

...

var keyStr = kn[keyCode];

You can call a function when the page loads using the onload event:

window.onload = focusOnLoad;

Since it's only executed once function cursorfocus(s) may as well be inline, rather than a function declaration. The same with selectElement().

I also like to put the larger block of code in my else statement. This makes it easier to read, and the flow feels better. Also avoid unnecessary variables, it's quite clear what this code means without x and y.

if (!comment) {
  quicksearch.focus();
} else {
    s.focus();
    window.scrollTo(window.scrollX, window.scrollY);
}

Finally, don't forget those var statements!