0

I have been trying to add a mouseover listener in javascript as follows:

for (var i = 0; i < classes.length; i++)
{
    var elements = document.getElementsByClassName("class" + i.toString());
    for (var j = 0; j < elements.length; j++) {
        elements[j].addEventListener("mouseover", function(e) {
            showDiv(i, e.pageX, e.pageY);
        });
    }
}

and showDiv is:

function showTranslationDiv(idx, x, y){
    const notification = document.getElementsById('div0');
    notification.innerHTML = "You are at: " + x.toString() + " " + y.toString();
    notification.style.top = y.toString() + 'px';
    notification.style.left = x.toString() + 'px';

    notification.style.display = 'flex';
    setTimeout(function () {

        notification.style.display = 'none';
    }, 5000);
}

classes is a list of 3 different class names (the real name is not relevant).

I encountered two problems here:

  1. When I use style.top, as you see above, it doesn't work and div0 didn't appear, but if I write a hard-coded top value (style.top = '400px' for example) it's working fine.
  2. the value of idx is always 3 for any class, it should be different for different class indices.

What is wrong with my code?

demo
  • 6,038
  • 19
  • 75
  • 149

1 Answers1

0

Your notification isn't appearing because it looks like you have a typo when calling getElementById.

This:

const notification = document.getElementsById('div0');

Should be:

const notification = document.getElementById('div0');

Your idx is always 3 becuase, as @phuzi mentioned in the comments, your event handler forms a closure around the value of i. The linked answer has more information.

When this:

showDiv(i, e.pageX, e.pageY);

Accesses i from here:

for (var i = 0; i < classes.length; i++)

It always refers to the same instance of i that was declared as a var.


To solve these problems, correct the typo and scope i so that its unique to each iteration of the loop creating your event handlers.

for (let i = 0; i < classes.length; i++)
{
    var elements = document.getElementsByClassName("class" + i.toString());
    for (var j = 0; j < elements.length; j++) {
        elements[j].addEventListener("mouseover", function(e) {
            showDiv(i, e.pageX, e.pageY);
        });
    }
}
function showTranslationDiv(idx, x, y){
    const notification = document.getElementById('div0');
    notification.innerHTML = "You are at: " + x.toString() + " " + y.toString();
    notification.style.top = y.toString() + 'px';
    notification.style.left = x.toString() + 'px';

    notification.style.display = 'flex';
    setTimeout(function () {

        notification.style.display = 'none';
    }, 5000);
}

As a side note, you may want to use innerText instead of innerHTML since you're only setting text value. innerHTML is a potential vector for cross-site scripting (XSS). Even though you're likely safe here, it's good practice to avoid it when possible.

notification.innerText = `You are at: ${x} ${y}`; 

Also, you may want to prefer querySelectorAll over getElementsByClassName because the latter is rechecked every time you interact with it.

var elements = document.querySelectorAll(`.class${i}`);
D M
  • 5,769
  • 4
  • 12
  • 27
  • Thanks! it's work now. And thanks for the other tips. Do you have any idea what is the solution for the style.top that didn't work? – hareldo Mar 17 '21 at 21:09
  • Was the `getElementById` typo not the issue? You're using [`style.top`](https://www.w3schools.com/jsref/prop_style_top.asp) correctly. What are the values of `x` and `y` when it fails? – D M Mar 17 '21 at 21:32
  • it wasn't the issue. the value was correct according to mouse position but the assignment wasn't work – hareldo Mar 17 '21 at 21:58