OnClick works only for last appended child

Question

I have such code:

var pageCount = 5; //for example, doesn't really matter
var paginationList = document.createElement("ul");
paginationList.className = "pagination";
for(var i = 1; i <= pageCount; i++){
  var paginationNode = document.createElement("li");
  var paginationLink = document.createElement("a");
  paginationLink.innerHTML = i;
  paginationLink.href = "#";
  paginationLink.onclick = function(){ console.log("yay"); }; //removed loadProperties here
  paginationNode.appendChild(paginationLink);
  paginationList.appendChild(paginationNode);
}
divxml.innerHTML = "";
divxml.appendChild(paginationList);
//code replaced by this comment inserts a lot of content to divxml
//for this bug or something to work, you need next line
divxml.innerHTML += "<br>";
divxml.appendChild(paginationList);

As you can see, I'm doing pagination here. The problem is that first pagination buttons don't work, I can't see yay in console when I click on them, but the second and last ones do work (I see yay in console when I click on them). What's wrong, How do I fix that?

the click event has to be instantiated after it is added to DOM. Are you using jQuery? — happymacarts, Dec 22 '16 at 17:46
This is just the classic closure loop problem. you're defining the onclick function inside of the loop, so it will only work for the very last element in the loop — Pabs123, Dec 22 '16 at 17:48
Where are `xhttp` and `pageSize` defined? I believe without a self-contained example there is not much we can do. Please see [mcve]. — Felix Kling, Dec 22 '16 at 17:52
@Pabs123: That's not correct (in general). Functions inside a loops are only a problem if the function access loop variables. That's not the case in the current version of the code. — Felix Kling, Dec 22 '16 at 17:53
@Zakaria: I don't think it's a duplicate of that question, at least it's too early to make that call. It especially doesn't explain why there is no log output for the first element but for the others. — Felix Kling, Dec 22 '16 at 17:55
I just tested the code (Adapting it to work in my environment) and provided that `pageCount`, `divxml` are defined and `loadProperties(xhttp, pageSize, Number(this.innerHTML));` doesn't throw errors, the code works. Are you sure you didn't deleted an important piece of code? — Desaroll, Dec 22 '16 at 18:17
@happymacarts if it has to be instantiated after it is added to DOM, why then the second buttons work? — Krigga, Dec 22 '16 at 21:38
@FelixKling edited code so xhttp and pageSize do not matter now — Krigga, Dec 22 '16 at 21:39
http://jsbin.com/lijisu/1/edit?html,js,console,output — I can't reproduce the problem. You need to provide a real [mcve] — Quentin, Dec 22 '16 at 21:42
@Desaroll I don't know how this works for you, but I have just removed loadProperties function and it's still same result (first buttons don't work, second ones do) — Krigga, Dec 22 '16 at 21:44

Quentin · Answer 1 · 2016-12-23T09:20:16.673

0

divxml.innerHTML += "<br>";

Reading from innerHTML converts the DOM into HTML. The HTML does not have the event handlers that were attached to the DOM.

Writing the HTML back to the innerHTML (after appending <br> to it) converts the HTML to DOM and overwrites the DOM that was there before.

You have now destroyed the event handlers.

Don't use innerHTML.

edited Dec 23 '16 at 09:20

answered Dec 22 '16 at 21:54

Quentin

914,110
126
1,211
1,335

How do I add a new line then? – Krigga Dec 22 '16 at 21:55
The same way you add a list element. `createElement` and `appendChild` (although a line break doesn't really make sense here, a CSS margin would be more appropriate). – Quentin Dec 22 '16 at 21:56
But if I add new line like this, then second group of pagination buttons doesn't appear, how to add them? – Krigga Dec 22 '16 at 21:57
An element can't exist in multiple parts of the DOM at once. If you `appendChild` the same thing in multiple places then you move it. If you want two sets of elements, then create two sets of elements. – Quentin Dec 22 '16 at 21:58
Is there a way to duplicate node, or I need to add another `for`? – Krigga Dec 22 '16 at 21:59
This won't fix the issue. Even if he doesn't use `innerHTML`, he's inserting the same nodes into the DOM twice, so both sets won't be rendered. – SimpleJ Dec 22 '16 at 22:06
@SimpleJ — Did you read the other comments? That's been addressed. – Quentin Dec 22 '16 at 23:27
@Quentin I see that now. I was just commenting to say that this answer doesn't fully solve the problem OP is describing in his question (event handlers not working). – SimpleJ Dec 23 '16 at 19:04

SimpleJ · Accepted Answer · 2016-12-23T19:09:49.703

You will have to create two list elements and two sets of list item elements for this to work:

var pageCount = 5; //for example, doesn't really matter
var paginationList1 = document.createElement("ul");
var paginationList2 = document.createElement("ul");
paginationList1.className = paginationList2.className = "pagination";

for(var i = 1; i <= pageCount; i++){
  paginationList1.appendChild(createPaginationLink(i));
  paginationList2.appendChild(createPaginationLink(i));
}

document.body.appendChild(paginationList1);
document.body.appendChild(document.createElement("br"));
document.body.appendChild(paginationList2);

function createPaginationLink(text) {
  var paginationNode = document.createElement("li");
  var paginationLink = document.createElement("a");
  paginationLink.innerText = text;
  paginationLink.href = "#";
  paginationLink.addEventListener("click", function(){ console.log("yay"); }); //removed loadProperties here
  paginationNode.appendChild(paginationLink);
  return paginationNode;
}

And as stated in the other answer, mutating innerHTML will cause your elements to be re-created without their event listeners, so instead create and append your <br/> element using createElement and appendChild.

Codepen

OnClick works only for last appended child

2 Answers2

Linked

Related