hasAttribute("Role") crashing web page

Question

I am creating a JavaScript Bookmarklet that checks each div element to see if it has a role attribute, as well as checks for "onkeypress", and "onclick" attributes. It highlights divs that don't have a role or has a "onclick", and lacking "onkeypress".

I am having an issue with my if statement for checking for role attributes. The code below leads to whatever page it is used on to crash.

var divs = document.getElementsByTagName("div");
for(var i = 0; i < divs.length; i++){
    if(divs[i].hasAttribute("role")){
        if(divs[i].hasAttribute("onclick" || "onClick")){
            if(!divs[i].hasAttribute("onkeypress")){
                divs[i].classList.add("highlight");
                var divAbove = document.createElement("div");
                divAbove.classList.add("text-box");
                divAbove.innerHTML = "Div";
                divs[i].appendChild(divAbove);
            }
        }
    }else{
        divs[i].classList.add("highlight");
        var divAbove = document.createElement("div");
        divAbove.classList.add("text-box");
        divAbove.innerHTML = "Div: Lacks Role";
        divs[i].appendChild(divAbove);
    }
}

But whenever I add the if statement for checking roles within the if statement checking for "onclick" it no longer crashes. But that removes the functionality for checking each div for a role.

I am unsure what the issue could be. I would appreciate any help. Thank you.

Answer 1

The problem here is that getElementsByTagName() returns a HTMLCollection.

An HTMLCollection in the HTML DOM is live; it is automatically updated when the underlying document is changed.

So the collections change when you create a new div. This causes an infinity iteration, because you iterate over the new div as well.

To avoid this, you can use querySelectorAll() which is not live.

const divs = document.querySelectorAll("div");
for(var i = 0; i < divs.length; i++){
    if(divs[i].hasAttribute("role")){
        if(divs[i].hasAttribute("onclick") || divs[i].hasAttribute("onClick")){
            if(!divs[i].hasAttribute("onkeypress")){
                divs[i].classList.add("highlight");
                var divAbove = document.createElement("div");
                divAbove.classList.add("text-box");
                divAbove.innerHTML = "Div";
                divs[i].appendChild(divAbove);
            }
        }
    }else{
        divs[i].classList.add("highlight");
        var divAbove = document.createElement("div");
        divAbove.classList.add("text-box");
        divAbove.innerHTML = "Div: Lacks Role";
        divs[i].appendChild(divAbove);
    }
}

<div></div>
<div></div>
<div></div>
<div></div>

.forEach() is not necessary here, but might be a cleaner way than the for loop.

const divs = document.querySelectorAll("div");
divs.forEach(div => {
  if (div.hasAttribute("role")) {
    if (div.hasAttribute("onclick") || div.hasAttribute("onClick")) {
      if (!div.hasAttribute("onkeypress")) {
        div.classList.add("highlight");
        var divAbove = document.createElement("div");
        divAbove.classList.add("text-box");
        divAbove.innerHTML = "Div";
        div.appendChild(divAbove);
      }
    }
  } else {
    div.classList.add("highlight");
    var divAbove = document.createElement("div");
    divAbove.classList.add("text-box");
    divAbove.innerHTML = "Div: Lacks Role";
    div.appendChild(divAbove);
  }
});

<div></div>
<div></div>
<div></div>
<div></div>

As already mentioned in the comments replace .hasAttribute("onclick" || "onClick") by divs[i].hasAttribute("onclick") || divs[i].hasAttribute("onClick").