Click EventListener only running once

Question

I am still learning the basics of JavaScript and currently learning eventListeners. I am trying to make a button which on clicking changes the background color of the whole body to some randomly generated rgb code, every 100ms, and on clicking it again, the background color turns back to white and stops the color changing.

I have made a loop with setTimeout. When button is clicked, random rgb values are generated and applied to body background color. I have used a Boolean flag which is assigned false value when button is clicked again, which stops the loop on checking the if condition. The problem that I am facing is that the event Listener is not working for more than one click.

Here is the code:

const button = document.querySelector('#btn');

var flag = true;

button.addEventListener('click', function() {
  loop();
})

function makeRGB() {
  const r = Math.floor(Math.random() * 255);
  const g = Math.floor(Math.random() * 255);
  const b = Math.floor(Math.random() * 255);
  const colorID = `rgb(${r},${g},${b})`;
  document.body.style.backgroundColor = colorID;
}

function loop() {
  if (!flag) {
    return;
  }
  makeRGB();
  setTimeout(loop, 100);
  button.onclick = function stop() {
    flag = false;
    document.body.style.backgroundColor = 'white';
  }
}

h1 {
  text-align: center;
}

button {
  margin: auto;
  display: block;
}

<h1 id="heading">Welcome!</h1>
<button id="btn">Change Color! </button>

Answer 1

Instead of creating our own makeshift interval I used the builtin setInterval function. Both setInterval and setTimeout return a number that you can pass to either clearInterval or clearTimeout to stop the async code execution.

const button = document.querySelector('#btn');

let changeColorInterval;

button.addEventListener('click', function() {
    // if we currently have an interval running, e.g. != undefined
    if (changeColorInterval) {
        // remove the interval, e.g. stop the execution of makeRGB
        clearInterval(changeColorInterval);
        // set the variable back to undefined, otherwise next time
        // you click the button this branch of the if statement
        // will be executed, even though the interval is not
        // actually running anymore.
        changeColorInterval = undefined;
        // restore the background to white like you wanted.
        document.body.style.backgroundColor = "white";
        
      // If we don't have an interval, create one
    } else changeColorInterval = setInterval(makeRGB, 100);

})

function makeRGB() {
    const r = Math.floor(Math.random() * 256);
    const g = Math.floor(Math.random() * 256);
    const b = Math.floor(Math.random() * 256);
    const colorID = `rgb(${r},${g},${b})`;
    document.body.style.backgroundColor = colorID;
}

h1 {
    text-align: center;
}

button {
    margin: auto;
    display: block;
}

<h1 id="heading">Welcome!</h1>
  <button id="btn">Change Color! </button>

Why it seems like it's only called once

if (!flag) { return; }

The event listener itself is called every time you press the button. You can see that if you place a console.log("click") inside of your click callback, right before loop(). The issue is that you never assigned your variable back to true so it always exists the function.

Don't add event listeners inside event listeners... unless you really intend to

button.onclick = function stop() { flag = false; document.body.style.backgroundColor = 'white'; }

You assign an "old school style" event listener inside an event listener which doesn't seem like a good idea. See: addEventListener vs onclick

Why not use var

You most likely don't want to use var. You more likely than not never want to use var. Just use let to declare variables you intent to modify, and const for variables that should be constant. If you actually care as to why google something like "javascript var vs let vs const". But if you're just starting to learn javascript, it's probably best to just avoid var until you understand it.

Incorrect color generation

Your color generation is just a tad wrong. As described by mozilla

The Math.random() function returns a floating-point, pseudo-random number in the range 0 to less than 1 (inclusive of 0, but not 1)

So 0 <= Math.random() < 1. You will never get 1 as the upper bound is exclusive.

So let's say you get 9.99999999... and multiply that by 255. You will never get 255 but something smaller than that. And then you floor it. So the max number you will get is 254. To fix that I would just recommend multiplying by 256.

Write a comment for additional help

If you need any additional help understanding the code, please get back to me in the comments of this answer :)

Answer 2

1. Have your button handler return a closure that maintains your flag status. This way you don't have any global variables.

2. Use setTimeout, and only invoke it if a condition is met. That way you don't have to clear anything.

3. Have makeRGB return a value rather than set the color of the element directly.

const button = document.querySelector('#btn');

// When you call `handler` it returns a new function 
// that is called when the button is clicked
button.addEventListener('click', handler(), false);

function makeRGB() {
  const r = Math.floor(Math.random() * 255);
  const g = Math.floor(Math.random() * 255);
  const b = Math.floor(Math.random() * 255);
  return `rgb(${r},${g},${b})`;
}

// Initially set `flag` to false
function handler(flag = false) {

  // Cache the document body element
  const body = document.body;

  // Return the function that serves as
  // the click listener
  return function() {

    // Reset the flag when the button is clicked
    flag = !flag

    // Start the loop
    function loop() {

      // If `flag` is true set the new colour
      // and call `loop` again
      if (flag) {
        body.style.backgroundColor = makeRGB();
        setTimeout(loop, 100);

      // Otherwise set the background to white
      } else {
        body.style.backgroundColor = 'white';
      }
    }

    loop();

  }
}

h1 { text-align: center; }
button { margin: auto; display: block; }

<h1 id="heading">Welcome!</h1>
<button id="btn">Change Color! </button>

Answer 3

Use setInterval instead of setTimeout in order to continuously change the background color, until the button is clicked again

const button = document.querySelector('#btn');

var flag = false;

var startChange;

button.addEventListener('click', function() {
  flag = !flag;
  loop();
})

function makeRGB() {
  const r = Math.floor(Math.random() * 255);
  const g = Math.floor(Math.random() * 255);
  const b = Math.floor(Math.random() * 255);
  const colorID = `rgb(${r},${g},${b})`;
  document.body.style.backgroundColor = colorID;
}

function loop() {
  if (!flag) {
    clearInterval(startChange);
    document.body.style.backgroundColor = 'white';
    return;
  }
  startChange = setInterval(makeRGB, 100);
}

h1 {
  text-align: center;
}

button {
  margin: auto;
  display: block;
}

<h1 id="heading">Welcome!</h1>
<button id="btn">Change Color! </button>

Answer 4

Here's a reformat of your code. Could probably be cleaned up more, but should do what you want it to. Instead of overwriting the interval, here we remove it instead.

const randomColor = () => Array(3).fill(0).map(() => Math.floor(Math.random() * 256)).join();

let loop;

document.querySelector("#btn").addEventListener("click", (e) => {
    if(loop){
        clearInterval(loop);
        loop = undefined;
        document.body.style.backgroundColor = "white";
    } else {
        loop = setInterval(() => {
            document.body.style.backgroundColor = `rgb(${randomColor()})`;
        }, 100);
    }
})