1

I'm trying to remove all hidden classes from elements on click with JavaScript. Here is the dummy code I use to try doing this:

    <style>
    .hidden {display:none;}
    </style>
    

    <div>Value 1</div>
    <div class="hidden">Value 2</div>
    <div class="hidden">Value 3</div>
    <div class="hidden">Value 4</div>
    
    
    <button onclick="removeHidden()">Show All</button>
    
    
    <script>
    function removeHidden()
    {
        var hidden = document.getElementsByClassName("hidden");
        for(var i=0; i<hidden.length; i++)
        {
            hidden[i].classList.remove("hidden");
        }
    }
    </script>

When clicking the button, I would expect all classes 'hidden' to be removed but oddly, it removes the hidden class from the second div and from the fourth but skips the third.

The result I get is:

Value 1
Value 2
Value 4

Any idea why that is because I really don't understand this?

I also tried this code but with the same result:

var els = document.getElementsByClassName("hidden");

Array.prototype.forEach.call(els, function(el) {
    el.ClassList.remove("hidden");
});
Scott Marcus
  • 64,069
  • 6
  • 49
  • 71
sharkmountain
  • 55
  • 2
  • 7
  • Possible duplicate of [JS: iterating over result of getElementsByClassName using Array.forEach](https://stackoverflow.com/questions/3871547/js-iterating-over-result-of-getelementsbyclassname-using-array-foreach) – Dave Everitt Feb 19 '19 at 20:59

4 Answers4

6

The issue is that getElementsByClassName() returns a "live" node list, which is a list that is updated anytime you reference the list. This ensures that you get the most up to date element references at all times. This is an expensive construct and is really only for rare use cases when it is needed.

Every time your code references the hidden variable, the DOM is re-scanned for elements that have the hidden class and after you begin removing that class, the length of the list shrinks by one. It's because of this changing length that one item gets skipped.

To use getElementsByClassName() correctly here, remove the class from the last element first and work your way backward to the first. This ensures that as the length of the node list shrinks, you are not skipping over any nodes.

<style>
    .hidden {display:none;}
</style>
    
    <div>Value 1</div>
    <div class="hidden">Value 2</div>
    <div class="hidden">Value 3</div>
    <div class="hidden">Value 4</div>
    
    
    <button onclick="removeHidden()">Show All</button>
          
    <script>
    function removeHidden()
    {
        var hidden = document.getElementsByClassName("hidden");
        for(var i = hidden.length-1; i > -1; i--)
        {
            hidden[i].classList.remove("hidden");
        }
    }
    </script>

But, because live node lists cause a performance hit, as a general rule, don't use them. Instead, use a static node list, which you get with the more modern and more flexible .querySelectorAll(). Also, if we convert the static node list returned by .querySelectorAll() into an Array, we can use the Array API to iterate it with .forEach(), which eliminates the need for an indexer.

<style>
    .hidden {display:none;}
</style>
    

    <div>Value 1</div>
    <div class="hidden">Value 2</div>
    <div class="hidden">Value 3</div>
    <div class="hidden">Value 4</div>        
    
    <button onclick="removeHidden()">Show All</button>
            
    <script>
    function removeHidden()
    {
        // Get all the elements that match the selector into an Array
        var hidden = Array.prototype.slice.call(document.querySelectorAll(".hidden"));
        
        // Now we can loop using the Array API
        hidden.forEach(function(item){
            item.classList.remove("hidden");
        });
    }
    </script>
Scott Marcus
  • 64,069
  • 6
  • 49
  • 71
  • 1
    Thanks so much, Scott! This makes a lot of sense now. I wasn't aware of the live/static node list difference. – sharkmountain Feb 19 '19 at 20:24
  • Your code samples appear to be the wrong way around? You describe removing the class from the last element first and working backward, then show sample code for using querySelectorAll. Then you describe using querySelectorAll but show a code sample for the aforementioned method of working backwards. Great answer other than that! :) – Andy Feb 19 '19 at 20:53
  • @Andy Thanks. Must have gotten them mixed up while editing. Corrected now. – Scott Marcus Feb 19 '19 at 20:58
2

You can use querySelectorAll. The problem with getElementsByClassName is that the list it generates is dynamic. It means that if some change is made to the DOM it is instantly reflected in the list, because whenever the list is accessed the DOM is scanned to provide the list. So when in the loop the classes are removed one by one the length of the list also grew shorter which was used in the loop(i<hidden.length). While querySelectorAll provides a static list thus gives the correct output.

function removeHidden() {
  var hidden = document.querySelectorAll(".hidden");
  for (var i = 0; i < hidden.length; i++) {
    hidden[i].classList.remove("hidden");
  }

}
.hidden {
  display: none;
}
<div>Value 1</div>
<div class="hidden">Value 2</div>
<div class="hidden">Value 3</div>
<div class="hidden">Value 4</div>


<button onclick="removeHidden()">Show All</button>
ellipsis
  • 12,049
  • 2
  • 17
  • 33
  • *It means that if some change is made to the DOM it is instantly reflected in the list* Well, no. Actually, there is no stored list at all. It means that every time you try to access the list, the entire DOM is scanned again and a new list is generated. – Scott Marcus Feb 19 '19 at 20:25
1

The reason is that the list you're iterating is a "live list". This means that it reflects the current state of the DOM at all times. As such, when you remove an element from the DOM with that class, it also gets removed from the list.

After its removal, the list is re-indexed from that point forward, meaning that the current iteration is now pointing to what had been the next element. Upon incrementing i++, you've skipped over that element, and moved on to the next, which had previously been two elements ahead. This continues on as you iterate the list.

To solve it, either iterate from the end of the list to the start, or use a non-live-list for iteration.

ziggy wiggy
  • 1,039
  • 6
  • 6
0

use var hidden = document.querySelectorAll(".hidden") instead

EDIT: as Ziggy Wiggy explained this is because your iterating over a list of DOM elements, once you remove one the rest of the elements are essentially "shifted" or re-indexed down one position. So when you iterate through value 2 and remove it, value 3 becomes the first element in the list, and you already iterated over it so the loop skips value 3 and goes to value 4. To avoid this, querySelector kind of provides a sort of snapshot of DOM Elements.

working snippet:

<style>
  .hidden {
    display: none;
  }
</style>


<div>Value 1</div>
<div class="hidden">Value 2</div>
<div class="hidden">Value 3</div>
<div class="hidden">Value 4</div>


<button onclick="removeHidden()">Show All</button>


<script>
  function removeHidden() {
    var hidden = document.querySelectorAll(".hidden");
    for (var i = 0; i < hidden.length; i++) {
      hidden[i].classList.remove("hidden");
    }
  }
</script>
cb64
  • 825
  • 8
  • 16