JavaScript: for loop in dynamic rollover function not working

Question

I'm trying to create a dynamic function using a for-loop in javascript, which will fire rollovers. I am using JS vs. CSS as the amount of images which will be rollovers is growing, and I figure a function is easier to maintain than x number of selectors.

This is creating a on method similar to jQuery.

var on = function(event, elem, callback, capture) {
  if (typeof elem === 'function') {
    capture = callback;
    callback = elem;
    elem = window;
  }
  capture = capture ? true : false;
  elem = typeof elem === 'string' ? document.querySelector(elem) : elem;
  if (!elem) return;
  elem.addEventListener(event, callback, capture);
};

These are my rollOver and rollOut functions:

function rollOver(elem) {
  document.getElementById(elem).src = `/images/home-page/desktop/EYES_ON_YOU_desktop_HP_HOVER_${elem.slice(length-1)}.jpg?$staticlink$`
}

function rollOut(elem) {
  document.getElementById(elem).src = `/images/home-page/desktop/EYES_ON_YOU_desktop_HP_NO_HOVER_${elem.slice(length-1)}.jpg?$staticlink$`
}

And this is where my for-loop lives:

  document.addEventListener("DOMContentLoaded", function(event) {
      var rollOverCollectionA = document.getElementById('roll-over-collection-a').querySelectorAll('img');
          rollOverCollectionA = Array.prototype.slice.apply(rollOverCollectionA);

          for (var i = 0; i < rollOverCollectionA.length; i++) {
           on('mouseover', rollOverCollectionA[i].id, function(){
             console.log( rollOverCollectionA[i].id)
             rollOver(rollOverCollectionA[i].id);
           });

           on('mouseout', rollOverCollectionA[i].id, function(){
            rollOut(rollOverCollectionA[i].id);
           }); 
          }

    });

In ${elem.slice(length-1)}, where that length come from ? Maybe you needs elem.slice(elem.length-1) — Charles Cavalcante, Nov 02 '18 at 22:02
Will document.querySelector work on an id if you don't prepend '#'? — Jim B., Nov 02 '18 at 22:47

score 1 · Answer 1 · 2018-11-03T15:49:45.267

The main problems I saw were:

elem.slice(length - 1); should be elem.slice(elem.length - 1) otherwise you're subtracting 1 from undefined
elem.slice should be replaced with elem.substr(elem.lastIndexOf('-') + 1) otherwise any images over 9 will start back at 0 because you would only get the last character of the id.
When you pass a string as elem to on, it uses document.querySelector, but you pass the id without the hash symbol (#). You don't need this anyway as you already have a reference to the image element, you can just pass that.

I also tidied it up and modernized it a little bit.

The glaring problem I neglected to mention was the use of var and the for(;;) loop. Thanks to @tranktor53 for pointing that out. I always instinctively replace for(;;) loops with for...in or for...of loops where I see them, and var with let or const, I didn't even notice that it was part of the problem.

function on({ type, element = window, callback, capture = false }) {
  if (typeof element === 'string') element = document.querySelector(element);
  if (!element) return;
  element.addEventListener(type, callback, capture);
};
function rollOver({ element, id }) {
  element.src = `https://via.placeholder.com/200x100?text=${ id }+hover`;
}
function rollOut({ element, id }) {
  element.src = `https://via.placeholder.com/200x100?text=${ id }+no+hover`;
}
document.addEventListener("DOMContentLoaded", _ => {
  const elements = document.querySelectorAll('#roll-over-collection-a img');
  for(let element of elements) {
    const id = element.id.substr(element.id.lastIndexOf('-') + 1);
    on({ type: 'mouseover', element, callback: _ => rollOver({ element, id }) });
    on({ type: 'mouseout' , element, callback: _ => rollOut({ element, id }) });
  }
});

<div id="roll-over-collection-a">
  <img id="roll-over-1" src="https://via.placeholder.com/200x100?text=1+no+hover">
  <img id="roll-over-2" src="https://via.placeholder.com/200x100?text=2+no+hover">
  <img id="roll-over-3" src="https://via.placeholder.com/200x100?text=3+no+hover">
  <img id="roll-over-4" src="https://via.placeholder.com/200x100?text=4+no+hover">
  <img id="roll-over-5" src="https://via.placeholder.com/200x100?text=5+no+hover">
  <img id="roll-over-6" src="https://via.placeholder.com/200x100?text=6+no+hover">
</div>

traktor · Answer 2 · 2018-11-04T02:18:31.880

The main problem is using var in the for loop and assuming that the value of i seen in the event handler will match that of i when th call back function was created. This is incorrect, since i will have the value it reached when the for loop completed, at a later time when the handler gets executed.

In current browsers the solution is to use let instead of var and hence starting the loop as

for (let i = 0; i < rollOverCollectionA.length; i++) ...

For discussion and older solutions see JavaScript closure inside loops – simple practical example

In regards the image source string calculation

/images/home-page/desktop/EYES_ON_YOU_desktop_HP_HOVER_${elem.slice(length-1)}.jpg?$staticlink$

please review what is needed - if you need to copy the entire value of elem as a string, don't include the call to slice. If a part of the string is required, the posted code may be correct. Slicing from a start index of elem.length-1 will copy the last letter of the element id (of course) and may also be correct.

Update: The need to capture the value of i during loop iteration can be eliminated in at least two ways:

In the mouseover and mouseout event handlers replace rollOverCollectionA[i]with this. There is no need for reverse lookup of an HTML collection based on a captured index value to determine the element an event handler is attached to.

Use event delegation with a single listener attached to on the images' DOM container. Using the same on function, and elem.id.slice(length-1) as a possible image source suffix, similar to:

document.addEventListener("DOMContentLoaded", function(event) {
    var rollOverCollectionA = document.getElementById('roll-over-collection-a');
    on('mouseover', rollOverCollectionA, function( event){
        var elem = event.target;
        if( elem && elem.id && elem.tagName === 'IMG') {
             elem.src = `/images/home-page/desktop/EYES_ON_YOU_desktop_HP_HOVER_${elem.id.slice(length-1)}.jpg?$staticlink$`;
    }
    });
    on('mouseout', rollOverCollectlonA, function(event) {
        var elem = event.target;
        if( elem && elem.id && elem.tagName === 'IMG') {
             elem.src = `/images/home-page/desktop/EYES_ON_YOU_desktop_HP_NO_HOVER_${elem.id.slice(length-1)}.jpg?$staticlink$`;
        }
    });

});

JavaScript: for loop in dynamic rollover function not working

2 Answers2