Javascript code not running inside onload function

Question

I am wondering why this code does not work. When I put it inside the onload function, it does not work, and I am not sure why. I have a lot of other code in the onload function, but I have deleted it for this example. Anyway the other code works fine, only this part does not.

window.onload = function() {
var i, timer, divide;
i = 0;
divide = 100;

function start() {
    timer = self.setInterval("increment()", (1000 / divide)); 
}

function increment() {
    i++;
    document.getElementById("timer_out").innerHTML = (i / divide);
}

function stop() {
    clearInterval(timer);
    timer = null;
}

function reset() {
    stop();
    i = 0;
    document.getElementById("timer_out").innerHTML = (i / divide);
}

}

<div>
    <span id="timer_out">0</span>
    </br>
    <input type="button" value="Start" onclick="start()"/>
    <input type="button" value="Stop" onclick="stop()"/>
    <input type="button" value="Reset" onclick="reset()"/>
</div>

Have you checked your console? (Hit F12 to open it). Your answer lies in there. Basically, your functions need to be global to access them in your onclick handlers. A better approach would be to use .addEventListener. — Mike Cluck
– Mike Cluck, Commented Feb 16, 2016 at 16:50
Basically what @MikeC trying to say is, your every buttons' onclick event is unable to call its function since you wrap their function inside onload event. The thing is, your buttons load before its function declared. — choz
– choz, Commented Feb 16, 2016 at 16:53

millerbr · Accepted Answer · 2016-02-16 17:14:20Z

10

You need to define your functions and variables outside the onload scope, otherwise they aren't accessible.

var i, timer, divide;
function start() {
    if(!timer) {
        timer = setInterval("increment()", (1000 / divide)); 
    }
}

function increment() {
    i++;
    document.getElementById("timer_out").innerHTML = (i / divide);
}

function stop() {
    clearInterval(timer);
    timer = null;
}

function reset() {
    stop();
    i = 0;
    document.getElementById("timer_out").innerHTML = (i / divide);
}
window.onload = function() {
    i = 0;
    divide = 100;
}

As mentioned in comments, you also have a bug that will make it impossible to stop the timer if you click start more than once. This is because you are creating two (or more) timers but only have a reference to the last one. To fix this, a simple if statement checking if a timer exists should suffice. Code above has been updated to show this, and here is an example of it running: http://jsfiddle.net/qPvT2/40/

edited Feb 16, 2016 at 17:14

answered Feb 16, 2016 at 16:54

millerbr

2,9411 gold badge16 silver badges25 bronze badges

Sign up to request clarification or add additional context in comments.

14 Comments

T.J. Crowder Over a year ago

@Blindeagle: You still have them inside your onload in that fiddle. (Hint: Consistent indentation makes it easier to see things like that.)

Mike Cluck Over a year ago

@Blindeagle The code you linked to doesn't contain the fix. Also, you'll want to remove self. since you don't declare self. Finally, avoid using strings with setInterval. Instead, just write setInterval(increment, 1000 / divide)

millerbr Over a year ago

Good eye on the self catch, removed that from my answer

Blindeagle Over a year ago

@MikeC ur a beautiful man! Thank you so much for all the help really happy x

millerbr Over a year ago

As mentioned by @choz you had a bug regarding the start button being pressed more than once. I've updated my answer with a fix for this too.

|

Community · Accepted Answer · 2017-05-23 11:52:50Z

Problems:

Functions referened from onxyz attributes must be globals. Your functions aren't globals, because they're declared inside your onload handler. Some people will tell you to make them globals, but there's a better answer.
onload is almost certainly much, much later than you'd really like this code to run. It waits until all images have fully-downloaded, for instance.
Using strings with setTimeout/setInterval is usually not a good idea.
</br> is invalid HTML. In HTML, a br element is written <br>. You can also write it <br/> or <br /> if you like, but the / is meaningless in HTML. (It has meaning in XHTML.)
There's no need to prefix setTiemout/setInterval with self.. It works, because there's a default self global, but it's unnecessary.
If you click start twice, you'll overwrite the previous timer's handle and not be able to stop it. We need to call stop before starting.

See comments for how I've addressed those below.

Without jQuery (because you tagged it, but don't seem to be using it)

// This script is at the end of the HTML, just beofre the closing
// </body> tag, ands o all the elements defined by the HTML above it
// will be there and ready for us to handle. No need for `onload`.
// But we'll use a scoping function to avoid creating globals.
(function() {
  var i, timer, divide;

  // Hook up our buttons.
  // For a very simple thing like this, we can just assign to 
  // their onclick properties, but for anything more complicated
  // I'd use addEventListener (falling back to attachEvent if
  // the browser doesn't have addEventListener, to support obsolete
  // IE like IE8), but that's overkill for this small example.
  document.querySelector("input[value=Start]").onclick = start;
  document.querySelector("input[value=Stop]").onclick = stop;
  document.querySelector("input[value=Reset]").onclick = reset;
  
  i = 0;
  divide = 100;
  timer = 0; // A value we can safely pass to clearInterval
             // if we never set anything else

  function start() {
    // Make sure to stop any previous timer
    stop();
    
    // Note no quotes or (), we're passing a reference to the
    // increment function into `setInterval`.
    // Also no need for `self.`
    timer = setInterval(increment, (1000 / divide));
  }

  function increment() {
    i++;
    document.getElementById("timer_out").innerHTML = (i / divide);
  }

  function stop() {
    clearInterval(timer);
    timer = null;
  }

  function reset() {
    stop();
    i = 0;
    document.getElementById("timer_out").innerHTML = (i / divide);
  }

})();

<div>
  <span id="timer_out">0</span>
  <br>
  <input type="button" value="Start" />
  <input type="button" value="Stop" />
  <input type="button" value="Reset" />
</div>

For more about addEventListener/attachEvent and a function you can use if you need to support obsolete browsers, see this answer.

With jQuery (because you did tag it)

// This script is at the end of the HTML, just beofre the closing
// </body> tag, ands o all the elements defined by the HTML above it
// will be there and ready for us to handle. No need for `onload`.
// But we'll use a scoping function to avoid creating globals.
(function() {
  var i, timer, divide;

  // Hook up our buttons.
  $("input[value=Start]").on("click", start);
  $("input[value=Stop]").on("click", stop);
  $("input[value=Reset]").on("click", reset);
  
  i = 0;
  divide = 100;
  timer = 0; // A value we can safely pass to clearInterval
             // if we never set anything else

  function start() {
    // Make sure to stop any previous timer
    stop();
    
    // Note no quotes or (), we're passing a reference to the
    // increment function into `setInterval`.
    // Also no need for `self.`
    timer = setInterval(increment, (1000 / divide));
  }

  function increment() {
    i++;
    $("#timer_out").text(i / divide);
  }

  function stop() {
    clearInterval(timer);
    timer = null;
  }

  function reset() {
    stop();
    i = 0;
    $("#timer_out").text(i / divide);
  }

})();

<div>
  <span id="timer_out">0</span>
  <br>
  <input type="button" value="Start" />
  <input type="button" value="Stop" />
  <input type="button" value="Reset" />
</div>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>

Same thing as miller's answer, click start twice and won't be able to stop it.
Ah did not see this problem , if u knoy any way to fix it, please let me know
@Blindeagle: It's fixed in the answer now (hit refresh just in case).

Milind Shinde · Accepted Answer · 2016-02-16 17:38:22Z

3

Instead of writing your functions in window.onload.. Just write them outside of it... As you are writing them inside the window.onload function, they are not accessible...just write them outside and your code will run properly

answered Feb 16, 2016 at 17:38

Milind Shinde

312 bronze badges

Collectives™ on Stack Overflow

Javascript code not running inside onload function

3 Answers 3

14 Comments

Without jQuery (because you tagged it, but don't seem to be using it)

With jQuery (because you did tag it)

5 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

14 Comments

Without jQuery (because you tagged it, but don't seem to be using it)

With jQuery (because you did tag it)

5 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related