having trouble with adding an event "onmouseover" to a span

Question

Can someone tell me what I am doing wrong? I've spent an entire day troubleshooting this but I am getting nowhere... I want to add the event "onmouseover" to my span elements. However when I implement the code below, nothing happens. I did a bit of googling and I think it might be a variable scope problem?? Im not too sure... Any help is appreciated!

<!DOCTYPE html>
<html>
    <head>
        <title>Fixing bugs in JS</title>
        <script src="question1.js" type="text/javascript"></script>
    <head>
    <body>
        <div id="output"></div>
    </body>
<html>

var NUMBERS = 100;

function go() 
{
  var out = document.getElementById("output");
  for (var i = 1; i < NUMBERS+1; i++) {
    var span_one = document.createElement("span");
      span_one.id = "span" + i;
      span_one.innerHTML = "" + i;
      out.appendChild(span_one);

    if (isPrime(i) === true) { // where i is a prime number (3, 5, 7..etc)
      span_one.style.backgroundColor = "red";
      span_one.onmouseover = function() {
        hover("span"+i, "yellow", "150%")
      };
      span_one.onmouseout = function() { 
        hover("span"+i, "red", "100%") // whatever color in this line always overrides previous set color...
      };
}

function hover(id, color, size) {
    var span = document.getElementById(id);
    span.style.backgroundColor = color;
    span.style.fontSize = size;
}


function etc() {
    ...
}

window.onload=go;

The problem is that when called, `i` no longer holds what it needs to hold. (The value of i inside onmouseover is not the same as when it was used as a param to isPrime) — enhzflep, Nov 27 '16 at 03:06
@enhzflep I checked via console.log(span_one.id) and it holds the value its suppose to have... — aeml, Nov 27 '16 at 03:10
That's not the same thing. `span_one.id` _should_ hold the same value as `i`, but it doesn't. — enhzflep, Nov 27 '16 at 03:14

score 1 · Answer 1 · answered Nov 27 '16 at 03:18

1

There's really no need to (a) give the elements an id (b) to use the i counter for anything other than the loop of creating them. Here's an alternative.

function newEl(tag){return document.createElement(tag)}
function byId(id){return document.getElementById(id)}

window.addEventListener('load', onDocLoaded, false);

function onDocLoaded(evt)
{
 var i, n = 100;
 var outputContainer = byId('output');
 
 for (i=1; i<=n; i++)
 {
  var span = newEl('span');
  //span.id = 'span_' + i;
  span.textContent = i;
  outputContainer.appendChild(span);
  
  if ( i%2 == 1) // isOdd
  {
   span.addEventListener('mouseover', onSpanMouseOver, false);
   span.addEventListener('mouseout', onSpanMouseOut, false);
  }
 }
}

function onSpanMouseOver(evt)
{
 this.style.backgroundColor = 'yellow';
 this.style.fontSize = '150%';
}
function onSpanMouseOut(evt)
{
 this.style.backgroundColor = 'red';
 this.style.fontSize = '100%';
}

<div id='output'></div>

answered Nov 27 '16 at 03:18

enhzflep

12,927
2
32
51

Your code doesn't color code anything. – Scott Marcus Nov 27 '16 at 03:22
@ScottMarcus - nonsense. The demo works right here in the page. Did you not try it before posting? (I did) Odd numbers turn yellow when hovered and turn red when the mouse leaves them. – enhzflep Nov 27 '16 at 03:34
Primes should be red automatically. Then they turn yellow when moused over and then back to red when mousedout. – Scott Marcus Nov 27 '16 at 03:37
Yeah, no kidding. You want an award for useless pedantry? You wrote, and I responded to "Your code doesn't color code anything" - mate, buzz off. – enhzflep Nov 27 '16 at 03:39
Whoa! Easy there. When I clicked "run" nothing was color coded. And, now that I am mousing over and out, I see that you are not leaving the primes red (also what the OP wants). Sorry for the confusion. – Scott Marcus Nov 27 '16 at 03:41
Just try engaging the brain _before_ you engage the mouth or the fingers next time. Concepts/behaviour unimportant to the realisation of a solution were deliberately omitted. Apology accepted, also, regret expressed for my hostile response. ;) – enhzflep Nov 27 '16 at 03:48
Hmmm. Your code doesn't do what the OP wants and when that is pointed out, you insult me, apologize and then insult me again? Uh ok. – Scott Marcus Nov 27 '16 at 03:53
No, we can't be friends.The OP wanted to attach event listeners for `mouseover` and `mouseout` to spans. The fact that I dealt with odd numbers rather than primes and that I didn't bother to colour items during page init are, as I asserted earlier, irrelevant. Sorry for assuming the readership here to be capable of the nous required to discern between relevant and irrelevant differences. Did my avatar provide no clues as to what your experience dealing with me may be like? Again, perhaps I assume too much of people.. The Dunning Kruger effect bites people at both ends of the bell-curve. ;) – enhzflep Nov 27 '16 at 04:02
I'm not asking to be your friend and "No" your profile doesn't indicate that you've had a bad day and are looking to jump all over someone. In my experience, code that does the polar opposite of the desired effect is usually flagged. Have a nice day. ;) – Scott Marcus Nov 27 '16 at 04:08

score 0 · Answer 2 · answered Nov 27 '16 at 03:12

Here's a working example:

http://jsbin.com/zixeno/edit?js,console,output

The problem is exactly what enhzflep said. One solution is to to move the "addSpan" logic out of the for loop and into a function.

var NUMBERS = 100;

function go() {
  var out = document.getElementById("output");
  for (var i = 1; i < NUMBERS+1; i++) {
    addSpan(i);
  }

  function hover(id, color, size) {
    var span = document.getElementById(id);
    span.style.backgroundColor = color;
    span.style.fontSize = size;
  }

  function addSpan(i) {
    var span_one = document.createElement("span");
    span_one.id = "span" + i;
    span_one.innerHTML = "" + i;
    out.appendChild(span_one);

    if (isPrime(i) === true) {
      span_one.style.backgroundColor = "red";
      span_one.onmouseover = function() {
        hover("span"+i, "yellow", "150%")
      };
      span_one.onmouseout = function() { 
        hover("span"+i, "red", "100%");
      };
    }
  }
}

Sumit Surana · Answer 3 · 2016-11-27T03:23:08.397

The problem is with the variable i, its a common issue with closures. For more information you can have a look at MDN closures and go to the section Creating closures in loops: A common mistake. To come over this issue, change the var in for loop to let. This will help you retain the scope and thus fixing the issue.

var NUMBERS = 100;

function go() {
  var out = document.getElementById("output");
  for (let i = 1; i < NUMBERS+1; i++) {
      let span_one = document.createElement("span");
      span_one.id = "span" + i;
      span_one.innerHTML = "" + i;
      out.appendChild(span_one);

    if (isPrime(i) === true) { // if a number is a prime then run this func
      span_one.style.backgroundColor = "red";
      span_one.onmouseover = function() {
        hover("span"+i, "yellow", "150%")
      };
      span_one.onmouseout = function() { 
        hover("span"+i, "red", "100%") // whatever color in this line always overrides previous set color...
      };
    }
    
    function hover(id, color, size) {
        var span = document.getElementById(id);
        span.style.backgroundColor = color;
        span.style.fontSize = size;
    }
    
    //Added my custom function as it was not provided
    function isPrime(i){
      return i%2 != 0;
    }
  }
}

window.onload = go;

<div id="output"></div>

Your result is the revers of what the OP wants. Primes should be red. — Scott Marcus, Nov 27 '16 at 03:21

score 0 · Accepted Answer · edited May 23 '17 at 12:24

Your issue is that you have closures around your i variable.

Closures occur whenever you nest a function within another function. Where the code runs unpredictably is when the nested function uses a variable from an ancestor function and the nested function has a longer lifetime than the ancestor in question.

Here, your mouseover and mouseout functions rely on i from the parent function go. Since the mouseover and mouseout functions are being attached to DOM elements and those DOM elements are going to remain in memory until the page is unloaded, those functions will have a longer lifetime than go. This means that the i variable that go declared can't go out of scope when go completes and that both of the mouse functions will SHARE the same value of i. The value that i has by the time a human comes along and moves the mouse is the LAST value it had when the loop ended.

Closures can be challenging at first, but you can read a bit more about them here.

Changing var i to let i on your loop solves that because let introduces block scope for each iteration of the loop.

Also, I saw that you were missing two closing curly braces that were causing errors. I added my own isPrime() function. See comments for locations:

window.onload=go;

const NUMBERS = 100;

function go(){

    
  var out = document.getElementById("output");
  
  // Using let instead of var avoids a closure by making sure
  // that each looping number exists in the block scope of the
  // loop and upon each iteration a new variable is created.
  for (let i = 1; i < NUMBERS+1; i++) {
    
    var span = document.createElement("span");
    span.id = "span" + i;

    span.innerHTML = i + "<br>"; 
    out.appendChild(span);

    if (isPrime(i)) { // where i is a prime number (2, 3, 5, 7..etc)
      span.style.backgroundColor = "red";
      
      // If you use the i variable in nested functions, you will create a
      // closure around it and both the mouseover and mouseout functions will
      // share the last known value of i. Each function must get its own copy
      // of i.
      span.onmouseover = function() {
          hover("span" + i, "yellow", "150%")
      };
      
      span.onmouseout = function() { 
        // whatever color in this line always overrides previous set color...
        hover("span" + i, "red", "100%") 
      };
      
    }  // <-- Missing
  }    // <-- Missing
  
}
  
function isPrime(value) {
    for(var i = 2; i < value; i++) {
        if(value % i === 0) {
            return false;
        }
    }
    return value > 1;
}

function hover(id, color, size) {
    var span = document.getElementById(id);
    span.style.backgroundColor = color;
    span.style.fontSize = size;
    console.log(id, span);
}

<div id="output"></div>

Hi @Scott Marcus, how can I break those numbers so that each has their own line? When I add out.innerHTML += "
";, it breaks the code. I no longer get the onmouseover effect. — aeml, Nov 27 '16 at 04:53
@aeml I've updated my answer to do that. You just needed to change this: `span.textContent= i;` to this: `span.innerHTML = i + "
";` — Scott Marcus, Nov 27 '16 at 04:55
@aeml No problem. Don't forget to upvote the answer. Good luck! — Scott Marcus, Nov 27 '16 at 05:06

having trouble with adding an event "onmouseover" to a span

4 Answers4