CODEWITHSUNDEEP
javascriptaddeventlistener
asimes
asimes

Reputation: 5894

JavaScript addEventListener() not working as expected

I have never used addEventListener(), but I cannot write the HTML equivalent I would like for each <div> I am treating as a button because of the way I am generating content. The equivalent would be:

<div onmousedown="jsItems[someId].toggleImage(someGallery, someIndex);"></div>

What I've been trying is this:

JsTree.prototype.addGalleries = function(inElements) {
    // ...unrelated code here removed for StackOverflow...

    for (var i = 0; i < this.jsGalleries.length; i++) {
        for (var j = 0; j < this.jsGalleries[i].buttons.length; j++) {
            var self = this;
            this.jsGalleries[i].buttons[j].addEventListener("mousedown", function() {
                self.toggleImage(i, j);
            });
        }
    }
}

Where i counts from 0 to 1 and j counts from 0 to 2 (for both i in this case), i represents someGallery, j represents someIndex, and I could access someId with this.id inside the code above (or with self.id inside addEventListener's function).

The problem is that although clicking on one of these "buttons" (<div>s) does trigger:

JsTree.prototype.toggleImage = function(inGallery, inIndex) {
    alert(this.id+", "+inGallery+", "+inIndex);
}

that it always alerts "8, 2, 3" regardless of which button is clicked. The "8" is correct but I have no idea why "2" or "3" are alerted. They seem to just be 1 more than what i and j count to (verified by trying j < this.jsGalleries[i].buttons.length-1 which alerts "8, 2, 2").

Edit: someId, someGallery, and someIndex are not real variables, they are junk I made up to try to explain the problem.

Upvotes: 0

Views: 321

Answers (2)

THEtheChad
THEtheChad

Reputation: 2432

It's not a problem with addEventListener. This is a common mistake. In order to understand what's going on, I have to explain how closures work.

When you have a loop and a function inside of it:

var i = 5;
while(i--){
  setTimeout(function(){
    console.log(i);
  }, 100);
}

Each function is given a reference to the variable i. That means that they don't retain the value of i at the time you defined them. Again, I'll restate, each function has a reference to the same variable i, not to the value that it had at the time the function was declared. In my example above, all of the setTimeout's are defined asynchronously. The anonymous functions all fire at 100 milliseconds and each one logs the value that's in i at the time that the function was run. In my example, that value would be -1 for all the functions.

There are 2 ways to solve this. I'll show you the easy one first:

for (var i = 0; i < this.jsGalleries.length; i++) {
    for (var j = 0; j < this.jsGalleries[i].buttons.length; j++) {
        var self = this;
        self.gallery = {i: i, j: j};
        this.jsGalleries[i].buttons[j].addEventListener("mousedown", function() {
            self.toggleImage(self.gallery.i, self.gallery.j);
        });
    }
}

Here, you're storing the values on the actual DOM element. These values are equivalent to the values at the time that the loop was run, so the event listener grabs the correct value. Notice I nested the value in an object called gallery. I did this to kind of namespace it. It's not a good idea to store values on elements in the DOM, just in case browsers end up implementing a property with the same name. I feel like gallery is safe enough.

The other option, and probably the best practice, for fixing this is to use closures to your advantage.

for (var i = 0; i < this.jsGalleries.length; i++) {
    for (var j = 0; j < this.jsGalleries[i].buttons.length; j++) {
        var self = this;
        this.jsGalleries[i].buttons[j].addEventListener("mousedown", (function closure(self, i, j){
            return function actualListener(){
                self.toggleImage(i, j);
            }
        })(self, i, j));
    }
}

In this case, we create a self executing function (called closure in my example) which runs immediately when we're creating the listener. Let me state it again, this function runs the moment the listener is being added, NOT when it's run. The reason we do this is so we can pass in the values we want to save for later, in this case, self, i, and j. Then, when the event occurs, the function that ACTUALLY gets run is the inner function (called actualListener). actualListener has a copy of all the values stored in its closure at the time that the closure function was run.

Upvotes: 1

loganfsmyth
loganfsmyth

Reputation: 161447

This is a classic JS mistake. The problem is that the values of i and j are not captured in any function scope, and your event handlers are asynchronous. That means that when your event handler runs, both for loops have run to completion, thus i == this.jsGalleries.length and j === this.jsGalleries[this.jsGalleries.length - 1].buttons.length.

Try out one of these:

JsTree.prototype.addGalleries = function(inElements) {
  // ...unrelated code here removed for StackOverflow...

  for (var i = 0; i < this.jsGalleries.length; i++) {
    for (var j = 0; j < this.jsGalleries[i].buttons.length; j++) {
      (function(self, innerI, innerJ){
        var galleryEl = self.jsGalleries[innerI].buttons[innerJ];
        galleryEl.addEventListener("mousedown", function() {
          self.toggleImage(innerI, innerJ);
        });
      })(this, i, j);
    }
  }
}

Or possibly clearer:

JsTree.prototype.addGalleries = function(inElements) {
  // ...unrelated code here removed for StackOverflow...

  var addHandler = function(self, i, j){
    self.jsGalleries[i].buttons[j].addEventListener("mousedown", function() {
      self.toggleImage(i, j);
    });
  };

  for (var i = 0; i < this.jsGalleries.length; i++) {
    for (var j = 0; j < this.jsGalleries[i].buttons.length; j++) {
      addHandler(this, i, j);
    }
  }
}

Upvotes: 3

Related Questions