Javascript increment variable to update table cells onclick

Question

I have a page where the user inputs a color and I call the onClick method to change the color of the individual cells of the table. However, when I click any cell, only the last cell (cell3 in this case) will change color. What am I doing wrong?

I get the error:

Message: 'document.getElementById(...)' is null or not an object
Line: 24
Char: 4
Code: 0

My code is:

    <html>

    <body>
    <input type='text' id='userInput' value='yellow' />

    <table border="1"> 
        <tr>
            <td id="1">cell1
            </td>
        </tr>
        <tr>
            <td id="2">cell2
            </td>
        </tr>
        <tr>
            <td id="3">cell3
            </td>
        </tr>

    </table>

    <script type="text/javascript">
    for(var i = 1; i <= 3; i++){
        document.getElementById(i).onclick = function(){
        var newColor = document.getElementById('userInput').value;
            document.getElementById(i).style.backgroundColor = newColor;
        }
    }
    </script> 
    </body>

    </html>

bunbun · Accepted Answer · 2012-09-20 08:40:28Z

3

Change your HTML to this: An ID must start with an an alpha character. It is not valid to start with a number.

<table border="1"> 
    <tr>
        <td id="td1">cell1
        </td>
    </tr>
    <tr>
        <td id="td2">cell2
        </td>
    </tr>
    <tr>
        <td id="td3">cell3
        </td>
    </tr>

</table>

This is a very common Javascript issue: All the code shares the value of i which is 3 at the end of the loop. You can solve it by using another helper function like this:

function changeIt(i) {
  // This inner function now has its own private copy of 'i'
  return function() {
    var newColor = document.getElementById('userInput').value;
      document.getElementById("td" + i).style.backgroundColor = newColor;
  }
}

for(var i = 1; i <= 3; i++){
    document.getElementById(i).onclick = changeIt(i);
}

It can also be written using an anonymous function, but those are harder to read.

edited Sep 20, 2012 at 8:40

bunbun

2,6764 gold badges36 silver badges54 bronze badges

answered Sep 20, 2012 at 8:26

Jeremy J Starcher

24k6 gold badges56 silver badges75 bronze badges

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

4 Comments

Stuart Wakefield Over a year ago

I was about to upvote, but your code does not solve the problem, you must replace document.getElementById(i).onclick = function() { changeIt(i); } with document.getElementById(i).onclick = changeIt(i);

Eric Lennartsson Over a year ago

Good point with the variable scope and alpha character id's. I'd prefer just using this, though.

Jeremy J Starcher Over a year ago

@StuartWakefield -- Yeah, I did slightly screw that up. Thanks for the catch.

Jeremy J Starcher Over a year ago

@EricLennartsson -- this would be a better choice, but then we'd have lost the chance to head off future questions.

Eric Lennartsson · Accepted Answer · 2012-09-20 08:25:38Z

1

First of all, your for loop is wrong. Try:

for(var i = 1; i <= 3; i++) {
   //code
}

Second, instead of retrieving the element each time in your loop, you could use this:

this.style.backgroundColor = document.getElementById('userInput').value;

edited Sep 20, 2012 at 8:25

answered Sep 20, 2012 at 8:17

Eric Lennartsson

5893 silver badges11 bronze badges

1 Comment

Eric Lennartsson Over a year ago

Updated to use the this keyword, which should make your code work. Also, like Jeremy pointed out, you shouldn't start an element id with a number.

Stuart Wakefield · Accepted Answer · 2012-09-20 08:44:08Z

Jeremy's answer is close but there is still a problem in that changeIt is not being called until the element is clicked, by which time the value of i is still three. Using Jeremy's update to the HTML the correct script can be written as...

function createChangeColorHandler(n) {
  return function() {
    var newColor = document.getElementById('userInput').value;
    document.getElementById("td" + n).style.backgroundColor = newColor;
  }
}

for(var i = 1; i <= 3; i++) {
  // We pass i to the function createChangeColorHandler by value
  // at the time of this pass of the loop rather than referencing 
  // the variable directly after the loop has finished
  document.getElementById(i).onclick = createChangeColorHandler(i);
}

As an anonymous function...

for(var i = 1; i <= 3; i++) {
  // We pass i to the function createChangeColorHandler by value
  // at the time of this pass of the loop rather than referencing 
  // the variable directly after the loop has finished
  document.getElementById(i).onclick = (function(n) {
    return function() {
      var newColor = document.getElementById('userInput').value;
      document.getElementById("td" + n).style.backgroundColor = newColor;
    }
  })(i);
}

EDIT Jeremy's answer is now correct

Collectives™ on Stack Overflow

Javascript increment variable to update table cells onclick

3 Answers 3

4 Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related