Your problem here is that the variable i won't have the value you think it'd have. That's due to how closures work.
Try this simple example:
for(var i = 0; i < 5; i++) {
console.log("Before timeout: " + i);
setTimeout(function() {
console.log("After timeout: " + i);
}, 1000);
}
What would you expect when you run this? Probably that after one second, you see this in the console?
Before timeout: 0
Before timeout: 1
Before timeout: 2
Before timeout: 3
Before timeout: 4
After timeout: 0
After timeout: 1
After timeout: 2
After timeout: 3
After timeout: 4
But guess what, you'll see this:
Before timeout: 0
Before timeout: 1
Before timeout: 2
Before timeout: 3
Before timeout: 4
After timeout: 5
After timeout: 5
After timeout: 5
After timeout: 5
After timeout: 5
Why 5? The for statement basically sets i to 0, then counts it up every iteration and checks every time whether it is still below 5, and if not, breaks the loop. So, after the loop, i has value 5, like this:
for(var i = 0; i < 5; i++) {
/* ... */
}
console.log("After loop: " + i);
...gives:
After loop: 5
Okay but why is the value used which i has after the loop? That's because your function callback you are passing to setTimeout accesses the i variable in the outside scope directly! And because the function will only run after one second (after the loop was long completed), the value of i at this time will be 5, as shown above!
So the solution would be to create a local copy of this variable which is local to each iteration. Now this is not as trivial as it seems because in JavaScript, control blocks like for don't create a scope (unless you use ES6 and let). You have to use another anonymous function around it to create a local copy which is then different each time:
for(var i = 0; i < 5; i++) {
// This is a so-called "immediately executed function expression"
// It's an anonymous function which is then immediately called due
// to the `()` at the end.
(function() {
var j = i; // Now we have a local `j`
console.log("Before timeout: " + j);
setTimeout(function() {
console.log("After timeout: " + j);
}, 1000);
})();
}
Another way to due this, which is a bit shorter, is this trick:
for(var i = 0; i < 5; i++) {
// See how this anonymous function now takes a parameter `i`?
// And we pass this parameter when calling this function below,
// which basically creates an inner copy which we can now also call
// `i`.
(function(i) {
console.log("Before timeout: " + i);
setTimeout(function() {
console.log("After timeout: " + i);
}, 1000);
})(i);
}
This is why I recommend to use things like forEach when iterating over arrays because they take a function callback as parameter so you automatically have a local scope in your code in there.
Now in your case, you could either use one of the anonymous-function-solutions from above, or something like forEach, but there is a gotcha: The element variable is no normal JavaScript array, it's a list of DOM elements, which doesn't have forEach. But you can still make it work using the Array.prototype.forEach.call trick:
function recordData(){
var element = document.getElementsByClassName("bubble");
Array.prototype.forEach.call(element, function(i) {
element[i].addEventListener("click", function(){
var id = element[i].attributes.id.value;
var x_cordinate = element[i].children[2].attributes.x.value;
var y_cordinate = element[i].children[2].attributes.y.value;
var keyword = element[i].children[0].textContent;
clicked_elements.push({
id: id,
x_cordinate: x_cordinate,
y_cordinate: y_cordinate,
keyword: keyword
})
}, false);
});
}
Before, it didn't work because you were effectively using element[element.length].attributes (because i was already one beyond your upper bound of the array), so element[i] is was undefined.
There you go!
However, an even better way would be using this because inside an event listener, this refers to the target of the event. So instead of element[i], you can just use this everywhere inside your listener!
