1

I have this code:

document.getElementById('img'+i).onclick = function(){popup_show('popup',array_msg[i]+'|||'+date('Y-m-d',strtotime(lec_date))+'-==-'+str_view_forConflict, 'AddEditSchedule;;popup_drag2;;EditSched;;'+String(array_msg_id[3])+';;view', 'popup_drag', 'popup_exit', 'screen-center', 0, 0);};

...but when I click on the image, the data of array_msg[i] is the last data of the loop, meaning the index is the length of the loop. I use IE for this.

Please give me an idea on how to do this. In FF, it works fine because I use setAttribute.

@bobince

document.getElementById('img'+i).onclick= popup_show.bind(window, 'popup',array_msg[i]+'|||'+date('Y-m-d',strtotime(lec_date))+'-==-'+str_view_forConflict,'AddEditSchedule;;popup_drag2;;EditSched;;'+array_msg_id[3]+';;view','popup_drag', 'popup_exit', 'screen-center', 0, 0    ); 
                if (!('bind' in Function.prototype)) {
                    Function.prototype.bind= function(owner) {
                        var that= this;
                        var args= Array.prototype.slice.call(arguments, 1);
                        return function() {
                            return that.apply(owner,
                                args.length===0? arguments : arguments.length===0? args :
                                args.concat(Array.prototype.slice.call(arguments, 0))
                            );
                        };
                    };
                }
Treby
  • 1,328
  • 6
  • 18
  • 26

5 Answers5

2

You need to use a closure. It would help if you provided the loop code as well as the code that is executed in the loop, but assuming you have a standard for loop iterating through an array, the following code should work:

for (var i = 0, l = array.length; i < l; i++)
{
    (function(i) {
        document.getElementById("img" + i).addEventListener("click", function() {
            popup_show("popup", array_msg[i] + "|||" + date("Y-m-d", strtotime(lec_date)) + "-==-" + str_view_forConflict, "AddEditSchedule;;popup_drag2;;EditSched;;" + String(array_msg_id[3]) + ";;view", "popup_drag", "popup_exit", "screen-center", 0, 0);
        }, false);
    })(i);
}

Also, you shouldn't be using setAttribute in Firefox. Instead, use element.onclick or, preferably, element.addEventListener, which allows you to add multiple functions to be called when an event fires and thus this plays nicely with other code (if two bits of code assign a function to, say, a click event in the form element.onclick = function() { ..., then the second assignment overrides the first—not good). I've used element.addEventListener in my code example above.

Steve Harrison
  • 121,227
  • 16
  • 87
  • 72
  • what do you suggest to use then? .onclick?? – Treby Jan 08 '10 at 06:17
  • Yes. `onclick` is cross-browser compatible going back over a decade. It's actually IE, not Firefox, that gets `setAttribute` wrong; either way you should generally never use `setAttribute` in an HTML document. (It still has uses in XML.) – bobince Jan 08 '10 at 06:20
  • 1
    I would definitely recommend using `onclick` rather than `setAttribute`, but these days, `onclick` is also a bit dated. I would recommend using `addEventListener` instead. More information about `addEventListener` and its IE equivalent (`attachEvent`) can be found here: https://developer.mozilla.org/en/DOM/element.addEventListener. – Steve Harrison Jan 08 '10 at 06:45
  • it says: Webpage error details Timestamp: Fri, 8 Jan 2010 06:49:44 UTC Message: Object doesn't support this property or method Line: 348 Char: 9 Code: 0 – Treby Jan 08 '10 at 06:50
  • 1
    @Treby: Which browser are you testing it in? – Steve Harrison Jan 08 '10 at 06:56
  • Internet Explorer and you loop is kinda weird. "l = array.length; i < l" i don't get this? – Treby Jan 08 '10 at 07:16
  • 1
    @Treby: It is a performance best-practice. It's very inefficient to constantly look up the `length` property of an array every loop iteration, so we cache the length of the array in a variable, `l`, and use this whenever we need to access the array's length during the loop. – Steve Harrison Jan 08 '10 at 07:58
  • 1
    @Treby: The above code doesn't take IE into account at all—try testing it in Safari, Chrome, Firefox, or another standards-compliant browser... – Steve Harrison Jan 08 '10 at 08:01
  • +1 for closure, -1 for browser dependency. `element.onclick` may be a bit dated, but it works in all browsers, and it's shorter. The only problem with it is it only allows for one callback per event type. However, this isn't always a problem, and at least not in this case. See http://www.quirksmode.org/js/events_tradmod.html – Justin Johnson Jan 08 '10 at 10:30
2

You've hit the loop-variable-closure problem. This is a very common gotcha in C-style languages with closures, such as JavaScript and Python. See the accepted answer of this question for a solution involving binding the loop variables in a second closure.

A slightly less nested solution is to use function.bind():

for (var i= 0; i<something.length; i++) {
    document.getElementById('img'+i).onclick= popup_show.bind(window, 'popup',
        array_msg[i]+'|||'+date('Y-m-d',strtotime(lec_date))+'-==-'+str_view_forConflict,
        'AddEditSchedule;;popup_drag2;;EditSched;;'+array_msg_id[3]+';;view',
        'popup_drag', 'popup_exit', 'screen-center', 0, 0
    );
}

however since this method is an ECMAScript Fifth Edition feature not supported by most browsers yet it needs a little help — see the bottom of this answer for a backwards-compatible implementation.

Community
  • 1
  • 1
bobince
  • 528,062
  • 107
  • 651
  • 834
  • i have an error error details Message: 'null' is null or not an object Line: 261 Char: 3 Code: 0 line 261 pertains to document.getElementById(;popup_drag;)['target'] = id; – Treby Jan 08 '10 at 06:43
  • Impossible to say without the code of that function really, but it's looking for an element with a given ID and not finding it. I don't know what the semicolons are doing there; if there were really like that in your source code that would be a syntax error and wouldn't run at all. – bobince Jan 08 '10 at 07:03
  • check out my question: this what i did – Treby Jan 08 '10 at 07:28
  • I mean that function where it is going wrong — the error is occurring inside another function, presumably `popup_show`. Anyway, you will need the code that defines `function.bind` to go *before* the loop that uses it. – bobince Jan 08 '10 at 08:45
2

Treby, this is cleaned up version of your answer. The additional anonymous anonymous function that you added isn't necessary.

for (var i = 0, l = array.length; i < l; i++ ) {
  document.getElementById(i + '05').onclick = (function(tmp) {
    return function() { 
      popup_show(
        "popup", 
        array_msg[tmp] + '|||' + date('Y-m-d', strtotime(lec_date)) + '-==-' + str_view_forConflict, 
        "AddEditSchedule;;popup_drag2;;EditSched;;" + String(array_msg_id[3]) + ";;view", 
        "popup_drag", "popup_exit", "screen-center", 0, 0
      );
    };
  })(i);
}

Edited to fix closure issue

Justin Johnson
  • 30,978
  • 7
  • 65
  • 89
1

Closures. You need to use JavaScript Closures. See if answers to this question help.

Community
  • 1
  • 1
Salman A
  • 262,204
  • 82
  • 430
  • 521
-1

Working Answer:

var closures = [];
for (var i = 0; i < array.length; i++){
  closures[i] = (function(tmp) {
        return function() {
        document.getElementById(tmp + '05').onclick = function(){popup_show("popup", array_msg[tmp]+'|||'+date('Y-m-d',strtotime(lec_date))+'-==-'+str_view_forConflict, "AddEditSchedule;;popup_drag2;;EditSched;;"+ String(array_msg_id[3]) +";;view", "popup_drag", "popup_exit", "screen-center", 0, 0)};
        };
})(i);

  closures[i]();
}

Thanks to Steve Harrison Answer. I got an idea to wrap around it

Community
  • 1
  • 1
Treby
  • 1,328
  • 6
  • 18
  • 26
  • If the question is answered, mark it as such. A note on the code: there's likely not a reason to save references to each closure. See my answer for a revised version of what you have. – Justin Johnson Jan 08 '10 at 10:39