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.

side note: `.hasAttribute("onclick" || "onClick")` does not do what you expect. Probably you mean `divs[i].hasAttribute("onclick") || divs[i].hasAttribute("onClick")` — Sven Eberth, Jun 11 '21 at 18:07

Sven Eberth · Accepted Answer · 2021-06-11T18:29:07.610

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").

score 0 · Answer 2 · edited Jul 06 '21 at 19:49

As @Sven Eberth point your divs collection is live. But you can frozen it by making copy. What hits me in your code is your 3 level nested really ugly (sorry for that) condition statement. Please consider changing this condition on some helper method.

var divs = Array.from(document.getElementsByTagName("div"));

const elementAttribute = (attributes, element) => {
  return attributes.include.every(attr => element.hasAttribute(attr)) 
  && attributes.exclude.every(attr => !element.hasAttribute(attr))
}

const createElement = ({classList, html}) => {
   const divAbove = document.createElement("div");
   classList.forEach(className => divAbove.classList.add(className));
   divAbove.innerHTML = html;
   return divAbove;
}

const insertElement = ({parent, parentClassNames}, attrs) => {
   parent.classsList.add(parentClassNames);
   element = createElement(attrs);
   parent.appendChild(element);
}

for(var i = 0; i < divs.length; i++){
    if(elementAttribute({
      include: ["role", "onclick"], 
      exclude: ["onkeypress"]
      }, divs[i]){
        insertElement(
          {parent: divs[i], parentClassNames: "highlight"},
          {classNames: "text-box", html: "Div"}
        )
    } else{
        insertElement(
          {parent: divs[i], parentClassNames: "highlight"},
          {classNames: "text-box", html: "Div: Lacks Role"}
        )
    }
}

p.s. onclick vs onClick:

Html element attribute are case insensitve. Good practice to keep HTML markup lowercase. So you don't have to check if is present "onClick" or "onclick". What more onClick with capital letter is react internal handler. It is never added to html element as attribute.

Proof: No matter that you check "onclick" or "onClick" both are treated in the same way.

const divs = Array.from(document.getElementsByTagName("div"))
const divsWith_onClick = divs.filter(el => el.hasAttribute("onClick"));
const divsWith_onclick = divs.filter(el => el.hasAttribute("onclick"));
console.log(divsWith_onClick.length === divsWith_onclick.length)

<div onClick="()=>{}"/>
<div onclick="()=>{}"/>

hasAttribute("Role") crashing web page

2 Answers2