3

I have a setup function that runs onload to add some behaviours to elements. The setup function passes arguments to the mouseover event but those arguments are being changed during the for loop because they are local references.

function setupAreas( image, map, lots ) {
    // obj is a simple wrapper for doc.getElementById
    var image = obj(image); // image for imagemap
    var map = obj(map); // imagemap element

    var areas = map.getElementsByTagName('area');
    for (var i in areas) {
        var area = areas[i]; // imagemap area element
        area.id = area.alt;
    }

    for (var lot_id in lots) {
        if (lot_id != 'Lot No' && lot_id != '') {
            var area = document.getElementById(lot_id);
            if (!area || !area.coords) {
                alert('no map coords for lot '+lot_id);
            } else {
                var coords = area.coords.split(",");
                //alert('tag: '+area.tagName+' id: '+lot_id+' area: '+area);
                var details = lots[lot_id];
                if (details) {
                    // setup mouseover call with complete details of area
                    area.onmouseover = function(){ showLot(lot_id, area, coords, details, image, map, areas, lots) };
... snip ...

The problem is that because of the for loop the references lot_id and area are changed on each iteration. The result is the mouseover event for any element gives the lot_id and area of the last area only.

I don't want or need jQuery for this. A simple JS solution that doesn't pollute the global namespace is preferred.

SpliFF
  • 38,186
  • 16
  • 91
  • 120

3 Answers3

2

Try surrounding the contents of your for loop in a closure:

for (var lot_id in lots) {
    (function(lid){
        //contents of for loop - use lid instead of lot_id    
    })(lot_id);
}

let me know how that works out

EDIT: You don't have to surround the whole loop actually, you could just surround the line that attaches the event:

(function(lid){
    area.onmouseover = function(){ showLot(lid, area, coords, details, image, map, areas, lots) };
})(lot_id);

However surrounding the whole loop may prevent future bugs arising :)

Darko
  • 38,310
  • 15
  • 80
  • 107
2

You need to create a closure around your function. Something like this might help:

function makeShowLot(lot_id, area, coords, details, image, map, areas, lots) {
  return function () { 
      showLot(lot_id, area, coords, details, image, map, areas, lots);
    };
}

Then, do this instead:

area.onmouseover = makeShowLot(lot_id, area, coords, details, image, map, areas, lots);

makeShowLot is a function that returns a function. That function that is returned takes no arguments; all of the arguments needed for showLot are enclosed in this anonymous function.

pkaeding
  • 36,513
  • 30
  • 103
  • 141
0

As you correctly observed because of closure, 'lot_id' is captured and it's the same for all mouseover events. Fixing the problem is simple, before you assign onmouseover, store the lot_id in another local var, say lotIdForMouseOver, and pass that to the mouseover function. The new local var thing will work in C#, not in JavaScript. At work, I do lot of C# and hence the confusion!

Like pkaeding suggested, create a helper function and you should be good.

On a side note, if you 'inverse' your 'if' checks, you can get rid of the nested ifs. IMHO, nested if's are very difficult to follow.

Here's how I would do it.

function setupAreas(image, map, lots)
{
    // existing code

    for(var lot_id in lots)
    {
        if(lot_id == 'Lot No' || lot_id == '')
            continue;

        var area = document.getElementById(lot_id);

        if(!area || ! area.coords)
        {
            alert('no maps for coords for lot ' + lot_id);
            continue;
        }

        var coords = area.coords.split(",");
        var details = lots[lot_id];

        if(! details)
            continue;

        //makeMouseOver function takes 'n' arguments and returns a function which
        //will call showLot with those same 'n' arguments.

        //This is the same suggestion as pkaeding, only that I have exploited 'arguments'
        //property to make it simpler. 
        var makeMouseOver = function()
        {
            var creationArgs = arguments;
            return function() { showLot.apply(null, creationArgs); };
        }

        area.onmouseover = makeMouseOver(lot_id, area, coords, details, image, map, area, lots);

        // more code.
    }
}
SolutionYogi
  • 31,807
  • 12
  • 70
  • 78