Prototype add event listener

Question

Hey,

For some reason, when I try assigning a actionlisetner to the list elements the value does not stick. Here is what I mean:

Event.observe(window, 'load', function() {
    for(i = 1; i <= $$('ul#review_list li').length; i++) {
     $('cover_' + i).observe('click', function(event) {
      alert(i);
     }); 
    }

});

So there are 7 list elements in #review_list and for some reason whenever any of the li elements are click I get an alert with value 8 for every element clicked. I want each to alert its respective i value. What am I doing wrong here?

Thanks!

Answer 1

Try this:

Event.observe(window, 'load', function() {
    for(i = 1; i <= $$('ul#review_list li').length; i++) {

        (function (i) { // i is passed as an argument below

            $('cover_' + i).observe('click', function(event) {
                alert(i); // creates a closure around the argument i
            });

        })(i); // pass i as an argument

    }
});

The reason for which the first method does not work is because alert(i); creates a closure around the loop variable i, which gets incremented for each event assignment. At the time the first event will be fired, the value of i, which is common for all the events is 8, that's why you get 8 no matter where you click.

In the second method, the one I posted, alert(i) creates a closure around the argument i, which won't be shared with any other event listener.

Anyway, you should read this article on JavaScript closures to better understand them.

Answer 2

Try adding a var keyword to your for loop. Without var you are assigning a global variable i, which is then incremented on every loop iteration. So after the loop it will have the value 8 and your alert is refering to that value.

Event.observe(window, 'load', function() {
    for(var i = 1; i <= $$('ul#review_list li').length; i++) {
        $('cover_' + i).observe('click', function(event) {
                alert(i);
        });     
    }
});

Answer 3

As Ionut G. Stan says, the issue is the closure over 'i'. RaYell is right about your wanting to declare the var (but not about that solving the problem).

That loop also repeatedly re-executes the $$ call, which isn't really ideal, and completely unnecessarily calls $ to look up elements you've already looked up (via $$).

FWIW:

Event.observe(window, 'load', function() {

    $$('ul#review_list li').each(function(elm, index) {
        ++index; // Now it's 1-based
        elm.observe('click', function(event) {
            alert(index);
        });
    });
});

$$ looks up the elements, Enumerable#each then iterates through the result calling the given function with the element reference and its zero-based index in the array. The event handler is then a closure over several things, including the index parameter passed into the #each iterator.

*Edit: I'm sorry, I just realized I made a massive assumption below: That the cover_x elements are, in fact, the list items under review_list. If they're not, disregard the below and my apologies! - T.J.*

So that works, but it's also unnecessarily complex. Event delegation could be your friend here: Look for clicks on the list, rather than list items:

Event.observe(window, 'load', function() {
    $('review_list').observe('click', function(event) {
        var li;
        li = event.findElement('li');
        if (li) {
            // ...your code here to interact with the list item.
            // If you need the index, you can find it easily enough:
            // index = parseInt(li.id.substring(6));
            // ...since your IDs on the LI items is "cover_x" with x
            // being the index.
        }
    });
});