Font and color checker chrome extension in JavaScript

Question

I have written this code for a color and font checker on a webpage.

The code is current and not so good. I would really like if I can get comments, reviews or any better approach for my PoC.

PoC: I want to highlight all the colors or fonts which should not be used on the webpage (the domain is defined by me as of now). I am traversing the DOM and checking the color and font styles and then highlight them if there are values out of my domain.

The current approach is very naive and not optimal because I am traversing and storing element styles, which is bad for duplicated DOM elements.

Any comments are appreciated, please let me know if I should add any more details.

inject.js

var brandColor = [
 "rgb(255, 255, 255)",
 "rgb(0, 0, 0)",
 "rgb(0, 163, 224)",
 "rgb(0, 118, 168)",
 "rgb(1, 33, 105)"
];
var brandFont = ["Open Sans", "Verdana"];
var fontNotMatch;
// generic walkDOM function - iterate DOM
function walkDom(start_element) {
 var arr = []; // we can gather elements here
 var loop = function(element) {
 do {
 // we can do something with element
 if (element.nodeType == 1) {
 // do not include text nodes
 arr.push(element);
 }
 if (element.hasChildNodes()) loop(element.firstChild);
 } while ((element = element.nextSibling));
 };
 loop(start_element);
 //loop(start_elem.firstChild); // do not include siblings of start element
 return arr;
}
var arr = walkDom(document.body);
var stylesContainer = {};
var fontChecker = {};
// iterate through DOM array
for (elem in arr) {
 // get all colors - [need better approach for this as well]
 stylesContainer[arr[elem].tagName] = getComputedStyle(
 arr[elem]
 ).getPropertyValue("color");
 // get all fonts
 fontChecker[arr[elem].tagName] = getComputedStyle(arr[elem]).getPropertyValue(
 "font-family"
 );
}
//console.log(stylesContainer);
// iterate styles
Object.keys(stylesContainer).forEach(element => {
 if (brandColor.indexOf(stylesContainer[element]) > -1) {
 //pass
 } else if (document.querySelector(element).childNodes.length == 1) {
 document.getElementsByTagName(element)[0].style.border = "2px solid red";
 var path =
 "https://example.com/icons/favicon.ico";
 var img = document.createElement("img");
 img.setAttribute("src", path);
 img.setAttribute("width", "20");
 document.querySelector(element).appendChild(img);
 }
 // console.log(stylesContainer[element]);
});
// iterate font family
Object.keys(fontChecker).forEach(element => {
 // Loop through each element and check the font family
 // console.log(fontChecker[element]);
 if (fontChecker[element].indexOf("Open Sans") != 1) {
 fontNotMatch = true;
 }
});
if (fontNotMatch == true) {
 // alert("Incorrect font");
 var f = document.createElement("h3");
 f.classList.add("error");
 f.innerHTML = "hey bhansa";
 document.querySelector("BODY").appendChild(f);
 var pathCSS = chrome.runtime.getURL("demo.css");
 var head = document.getElementsByTagName("head")[0];
 var link = document.createElement("link");
 link.rel = "stylesheet";
 link.type = "text/css";
 link.href = pathCSS;
 head.appendChild(link);
}

• Bhansa: Nah, it works but it's not in the very optimized form.
• Sᴀᴍ Onᴇᴌᴀ: Why find elements with one and only one child element (i.e. if (document.querySelector(element).childNodes.length == 1)) to set the border color and insert an image? why not elements with 0 or 2+ child elements?
• Bhansa: @SᴀᴍOnᴇᴌᴀ I want to give border only to leaf nodes otherwise, it will result in nested borders. I just want to highlight that element that it is not compliant.

Answer 1

Just some random comments from a quick read through:

1) Does this work if someone defines their colors using hex or name style (#fff, #ffffff, white, rgba(255,255,255,1)?

2) Walking the DOM and creating a huge array is probably not a good idea for anything but a small web page. After that you have another 3 separate loops. I would suggest that you create a function that walks the dom and calls a function for each node (function walkDom(start_element, callback) { .... Then you only need to store nodes that fail the test. Also combine ll those loops into one test.

3) Rather than comparing the nodetype with 1 I would use if (element.nodeType === Node.ELEMENT_NODE) ...

4) Rather than a do...while loop and nextSibling it is probably simpler just to get the childNodes and loop through them.

5) Or even better look into the TreeWalker api https://developer.mozilla.org/en-US/docs/Web/API/TreeWalker

6) This loop doesn't make sense:

for (elem in arr) {
 stylesContainer[arr[elem].tagName] = ...;
}

If you have two <div>'s in your tree the second one will over-write the first one.

7) A few things here:

if (brandColor.indexOf(stylesContainer[element]) > -1) {
 //pass
} else if (document.querySelector(element).childNodes.length == 1) {
 document.getElementsByTagName(element)[0].style.border = "2px solid red";
 ...

}

Firstly don't use an if with an empty block, use a negated if :

if (brandColor.indexOf(stylesContainer[element]) < 0 && document.querySelector(element).childNodes.length == 1) {

Also why only highlight elements with a single child and not those with two children or no children? Also consider using includes instead of indexOf And again this:

document.getElementsByTagName(element)

will return the first tag of that type in the document. It might not be the one with the issue.

8) Don't mix styles. You use for ... in in some places and forEach in others. If you're using ES6 use let and const instead of var.

9) It is unnecessary to compare to true. if (fontNotMatch == true) { can be written as just if (fontNotMatch) {

10) If you check color you might want to check background-color and the various border-color's too.

• Bhansa: Thanka @Marc, really helpful comments. Few clarifications: 1) It returns the rgb value so it should be fine. 6) any solution for this even if I want to store element whic are not correct. why only highlight elements with a single child: I want to highlight only single child otherwise it will highlight in a nested way. 10) Right now I am only checking for color value.
• Marc Rohloff: For (6) I would check elements using a callback and either (a) highlight them immediately or (b) push the element onto a non-associative array. You could also add logic at this point to disable recursing into elements that have an error.

Answer 2

General feedback

It is difficult to tell exactly what this code is supposed to do, mostly because there are no samples of HTML where the code will and will not perform different tasks. I tried running it on the HTML of your github page (though without the CSS initially). I see the border added to two elements: a subscribe link and a <time> element. You replied to my question in a comment, saying:

I want to give border only to leaf nodes otherwise, it will result in nested borders. I just want to highlight that element that it is not compliant.

But the code will only add the border and image to the first element of each tag that have only one child element. Should it do so to all elements of each tag that have only one element? If so, document.querySelector() won't work for that.

As was mentioned already, the code can likely be optimized to use fewer loops. And DOM lookups can be minimized by caching them in variables (see third bullet below). And instead of always adding every single element node to arr in loop(), would it work to only add node elements that have one and only one child element?

More specific feedback

• ES-6: Arrow functions are used, which are a feature of EcmaScript-2015 (A.K.A. es-6). Because of this, other es-6 features can be used like const for any variable that doesn't need to be re-assigned and let for block scope variables.
• Unused variable: var brandFont does not appear to be used in the code after it is declared. Was it supposed to be used when setting fontNotMatch?

• Adding borders and adding images:

  } if (document.querySelector(element).childNodes.length == 1) {
   document.getElementsByTagName(element)[0].style.border = "2px solid red";
  

  The conditional checks if the first element matched by element has one single child node, then the next line sets the border of the first element with that tag name. Then later on, the first element matched by element is used again to insert the image into. This could be optimized by storing the DOM reference to the first element with the tag name in a (const) variable at the beginning and then using that variable later instead of querying the DOM later.

• Checking for a substring in a string This code:

  if (fontChecker[element].indexOf("Open Sans") != 1) {
  

  checks that Open Sans appears in the 2^nd position (i.e. index 1). While that works for a string that starts with "Open Sans" that text could come later in the string, which would break the condition. A better check would be that the substring has an index greater than -1 (i.e. found):

  if (fontChecker[element].indexOf("Open Sans") > -1) {
  

• Empty Conditional block: As Marc mentioned, the block below can lead to a code smell:

  if (brandColor.indexOf(stylesContainer[element]) > -1) {
   //pass
  } else if (document.querySelector(element).childNodes.length == 1) {
  

  For more tips on cleaning up code, check out this video of a presentation Rafael Dohms talk about cleaning up code (or see the slides here).