How to add on-click event to button in javascript?

Question

<html>
<body>
<input type="text" id="number"/>
<input type="button" value="create button" onclick="createbtn()"/> 
<br> 
<script>
function createbtn() 
{
  var n=document.getElementById("number").value;

  for(i=1;i<=n;i++)
   {
    var x = document.createElement("INPUT");
    x.setAttribute("type", "button");
    x.setAttribute("value", i);
    x.setAttribute("id","x"+i);
    document.body.appendChild(x);
    document.getElementById("x"+i).onclick=function(){rotate(i)};            
   }
}

function rotate(p)
  { 
   var n=document.getElementById("number").value;
   var j=n;
   var k=0; 

   for(i=n;i>p;i--)
    {
     document.getElementById("x"+i).value=i-p;
    }
   for(i=1;i<=p;i++)
    {
     document.getElementById("x"+i).value=(j-(p-1))+k; 
     k++;
    }
  }

</script>
</body>
</html>

In the above code what I am trying to do is create number of buttons equal to number inputted in text field and then add an on-click event to those buttons so that when they are clicked the value of each button is rotated. The method I have used for getting an onclick event is not working properly. Help me!!

This is a classic mistake in using closures in loop. Read developer.mozilla.org/en-US/docs/Web/JavaScript/Closures about your code. — Alex Kudryashev
– Alex Kudryashev, Commented Jul 20, 2016 at 18:11

Mitya · Accepted Answer · 2016-07-20 18:13:52Z

5

Several issues here.

1) Use meaningful variable names. x and p are hardly indicative and will make life difficult when you come back to the code one day and wonder what they do. (They also hinder people trying to help you on Stack Overflow, who have to first interpret them.)

2) You don't need to do document.getElementById("x"+i).onclick - you already have a reference to the button in x so you can just do x.onclick.

3) As a rule, don't use DOM-0 events (e.g. .onclick). These allow you only one of each type and are less manageable. Instead, use .addEventListener().

4) As per @Alex Kudryashev's comment, you are making the common beginner mistake of trying to reference an iterative variable inside an asynchronous closure. In other words, by the time rotate() runs, p will always evaluate to the last value of i, because it happens after the loop has finished. Instead, you need to pass a run-time copy of i in at the point of binding the event handler. This can be done with the help of an immediately-executed function which returns the event handler. (This can be a little daunting if you're new to JS and is thus outside the scope of this answer. Look it up, though.)

5) Rather than binding separately to each and every button, look into event delegation. This makes your code more efficient and can help performance when you're dealing with an appreciably high number of elements and events.

All in all, try this:

x.addEventListener('click', (function(i_local_copy) { return function() {
    rotate(i_local_copy);
}; })(i));

answered Jul 20, 2016 at 18:13

Mitya

34.7k15 gold badges70 silver badges125 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

sagar Over a year ago

Thank you!! it worked well.But i have certain modification in my requirement.If i wanted to add same function to every button and the value of local copy must increment every time any of the button is clicked. Can you help me with this?

Mitya Over a year ago

I'm unsure what you mean. You should probably ask a separate question for this, since it's a departure from your original question.

Collectives™ on Stack Overflow

How to add on-click event to button in javascript?

1 Answer 1

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related