0

I have following js

document.addEventListener("DOMContentLoaded", function() {
  document.querySelector("#pageRedirect").addEventListener("change", changeHandler);
});

function changeHandler() {
  var timer;
  if (pageRedirect.checked) {
    timer = setTimeout(function() {
      window.location.href = 'http://www.google.com';
    }, 3000);

    console.log(pageRedirect.checked);
  } else {
    //dont redirect
    clearTimeout(timer);

    console.log(pageRedirect.checked);
  }
}
<input type="checkbox" id="pageRedirect" name="pageRedirect" />

I dont redirect when checkbox uncheck but it is. Where's my fault ?

Thanks.

Jason Cust
  • 10,743
  • 2
  • 33
  • 45
Phatyh T.
  • 45
  • 5
  • what is `pageRedirect.checked`? you almost certainly want `document.getElementById("pageRedirect").checked` or `$('#pageRedirect').prop("checked")`. you could declare `var pageRedirect;` in line 1, and then set it to the element inside the DOMContentLoaded handler, to continue using it in the rest of the code – Plato Jul 14 '15 at 14:40
  • @Plato [See this question](http://stackoverflow.com/questions/3434278/do-dom-tree-elements-with-ids-become-global-variables) - many people use IDs and names with hyphens in them to prevent them becoming global variables – James Thorpe Jul 14 '15 at 14:45

2 Answers2

3

The issue is the location of your variable declaration for the timer ID. It's inside your function, so when you attempt to clear it, it's not got the ID you assigned in the setTimeout. You need to move the declaration outside of your function so it can keep the handle of the timer between invocations:

var timer;
function changeHandler(){
    // etc

This is compounded by the spec for clearTimeout which says:

If handle does not identify an entry in the list of active timeouts of the WindowTimers object on which the method was invoked, the method does nothing.

I.E. it will silently ignore any invalid timer handles, which in your case would be undefined.

James Thorpe
  • 31,411
  • 5
  • 72
  • 93
  • Oh, I get it, it's because you want to clear the timer from the FIRST click when the handler is run for the SECOND click – Plato Jul 14 '15 at 16:26
1

The issue is that the timer variable is redeclared every time the function executes, so even having closures, the viariable doesn't hold the timer.

You could store the timer somewhere else, like in the node. Notice in the following snippet I store the timer in this.timer

document.addEventListener("DOMContentLoaded", function () {
    document.querySelector("#pageRedirect").addEventListener("change", changeHandler);
});

function changeHandler(){
    if(pageRedirect.checked){
        this.timer = setTimeout(function(){
            document.getElementById('output').innerHTML = "done";
        }, 3000);
    } else {
        //dont redirect
        clearTimeout(this.timer);
    }
}
<input type="checkbox" id="pageRedirect" name="pageRedirect"/>

<div id="output"></div>
JSelser
  • 3,510
  • 1
  • 19
  • 40