5
\$\begingroup\$

I'm building a Mozilla Firefox extension to hide link's formatting and show them as regular text. It consists of a script that runs on web-pages (content.js) that's activated by clicking on the extension (handled with background.js). On activation, a CSS rule is injected to hide link's formatting. Activating it again will shut changes off.

background.js additionally updates the extension's icon (based on links being currently hidden or not) every time the active tab is changed.

I'm new to JavaScript. I'd really appreciate your comments and observations to improve it (best practices, code structure, etc).

Script that runs on web pages (content.js):

'use strict';

// Links formatting state variable
let hideLinks = false;
const hideLinksRule = `
a,
a:hover,
a:focus,
a:active,
a:visited {
    text-decoration: none !important;
    color: inherit !important;
    background-color: inherit !important;
    border-bottom: initial !important;
}`;


let styleSheet = (function () {
    let style = document.createElement('style');
    document.head.appendChild(style);
    return style.sheet;
})();


function handleRequest(request) {
    
    // Do not modify if request is only a query
    if (request.isQuery) {
        return Promise.resolve({
            hideLinks
        });
    }

    // Toggle hide/show link formatting
    if (hideLinks) {
        styleSheet.deleteRule(0);
        hideLinks = !hideLinks;
    } else {
        styleSheet.insertRule(hideLinksRule, 0);
        hideLinks = !hideLinks;
    }

    // Return links state
    return Promise.resolve({
        hideLinks
    });

}


browser.runtime.onMessage.addListener(handleRequest);

Script to handle the extension on the browser (background.js):

// Icons paths
const showLinksIconPath = {
    path: 'icons/link-icon.png'
};
const hideLinksIconPath = {
    path: 'icons/broken-link-icon.png'
};


function onError(error) {
    browser.browserAction.setIcon(showLinksIconPath);
}


function updateIcon(response) {
    if (response.hideLinks) {
        browser.browserAction.setIcon(hideLinksIconPath);
    } else {
        browser.browserAction.setIcon(showLinksIconPath);
    }
}


function verifyTabStatus(activeInfo) {
    browser.tabs.sendMessage(
        activeInfo.tabId, {
            isQuery: true
        }
    ).then(updateIcon).catch(onError);
}


function activeTabLinkToggle() {
    browser.tabs.query({
        currentWindow: true,
        active: true
    }).then(requestLinkFormatToggle).catch(onError);                
}


function requestLinkFormatToggle(tabs) {
    
    if (!(tabs === undefined || tabs.length == 0)) {
        const activeTab = tabs[0];
        browser.tabs.sendMessage(
            activeTab.id, {
                isQuery: false
            }
        ).then(updateIcon).catch(onError);
    }

}


// Update Icon on tab change
browser.tabs.onActivated.addListener(verifyTabStatus);


// Request toggle link formatting on click
browser.browserAction.onClicked.addListener(activeTabLinkToggle);

Configuration file to put it together (manifest.json):

{

    "manifest_version": 2,
    "name": "No-link",
    "version": "1.0",

    "description": "Hide link formatting.",

    "icons": {
        "48": "icons/link-icon.png"
    },


    "permissions": [
        "tabs"
    ],


    "browser_action": {
        "default_icon": "icons/link-icon.png",
        "default_title": "No-link"
    },


    "background": {
        "scripts": ["background.js"]
    },
    
    "content_scripts": [
        {
            "matches": ["<all_urls>"],
            "js": ["content.js"]
        }
    ]

}

You can also get the source code here. Thank you very much for your time.

\$\endgroup\$
2
\$\begingroup\$

Always prefer const when declaring variables - only use let when you have to reassign a variable. const requires less cognitive overhead, when you know at a glance that something's never going to be reassigned.

Style toggling You're currently toggling the style with styleSheet.insertRule(rule, 0) and styleSheet.deleteRule(0). I think it would be a little bit better to take a different approach.

  • Who knows what insertRule(rule, 0) and deleteRule(0) do at a glance? Probably not a whole lot of people; it's not a very common API that's used.
  • What if you need to expand the extension to add additional rules? Then using multiple rule indicies could become repetitive.

One alternative would be to, on toggle, either append or remove the style tag from the <head>. Another alternative would be to assign or clear its textContent. See below for example.

hideLinks is not an altogether precise name. At first glance, someone seeing a variable named hideLinks would probably think it sounds like a function that, when called, hides links. Maybe call it something else, like linksAreHidden. You can also make its toggling more DRY:

if (hideLinks) {
    styleSheet.deleteRule(0);
    hideLinks = !hideLinks;
} else {
    styleSheet.insertRule(hideLinksRule, 0);
    hideLinks = !hideLinks;
}

can be

if (hideLinks) {
    styleSheet.deleteRule(0);
} else {
    styleSheet.insertRule(hideLinksRule, 0);
}
hideLinks = !hideLinks;

Or, even better, rather than having a persistent outside variable, you could simply check to see whether the style element is attached or has content. See below for example. This is what I'd prefer - let the style node (which must exist regardless) be the source of truth, rather than introducing another variable that needs to stay in sync.

Request object You currently have

if (request.isQuery) {
    return Promise.resolve({
        hideLinks
    });
}
// otherwise, insert or remove style rule

I think it'd be better to be more explicit. What if you add a 3rd request command, or a 4th request command? Instead of having an implicit "Else, if this isn't a query, then do THIS instead", I'd prefer to always send a property indicating what the command should be.

Perhaps like this, using a isToggle property:

const hideLinksStyleText = `
a,
a:hover,
a:focus,
a:active,
a:visited {
text-decoration: none !important;
color: inherit !important;
background-color: inherit !important;
border-bottom: initial !important;
}`;
const styleTag = document.head.appendChild(document.createElement('style'));

function handleRequest(request) {
    const linksOriginallyHidden = Boolean(styleTag.textContent);
    // Do not modify if request is only a query
    if (request.isQuery) {
        return Promise.resolve({
            linksAreHidden: linksOriginallyHidden
        });
    }
    if (request.isToggle) {
        styleTag.textContent = linksOriginallyHidden
            ? hideLinksStyleText
            : '';
        return Promise.resolve({
            linksAreHidden: !linksOriginallyHidden
        });
    }
}

The conditional operator can be used to make conditional expressions tersely. This:

const showLinksIconPath = {
    path: 'icons/link-icon.png'
};
const hideLinksIconPath = {
    path: 'icons/broken-link-icon.png'
};
function updateIcon(response) {
    if (response.hideLinks) {
        browser.browserAction.setIcon(hideLinksIconPath);
    } else {
        browser.browserAction.setIcon(showLinksIconPath);
    }
}

can turn into

function updateIcon(response) {
    browser.browserAction.setIcon({
        path: `icons/${response.hideLinks ? 'broken-' : ''}link-icon.png`
    });
}

Or, if you don't like the interpolation:

function updateIcon(response) {
    browser.browserAction.setIcon({
        path: response.hideLinks
            ? 'icons/broken-link-icon.png'
            : 'icons/link-icon.png'
    });
}

Tab checking You have:

if (!(tabs === undefined || tabs.length == 0)) {
  const activeTab = tabs[0];

The negations are slightly tougher to parse logically at a glance than they can be. Consider instead:

if (tabs && tabs.length) {

In a larger project, you could use optional chaining and Babel:

if (tabs?.length) {

But optional chaining is only supported in very new versions of FF, and Babel isn't worth it just for this. (Babel is a good idea if you want to write a reasonably sized script and want to permit older browser versions to be able to understand the JS syntax you use. For a FF extension, it's less important, because almost all users will be on reasonably up-to-date browsers anyway - no need to transpile down to ES5 for those crummy IE11 users)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your recommendations. One question, the ! operator here const linksOriginallyHidden = !styleTag.textContent; is to verify if the string is empty? \$\endgroup\$ – fabrizzio_gz Sep 24 at 21:47
  • \$\begingroup\$ Yep, that inverts the truthyness of the following expression. !someText evaluates to true if someText contains the empty string. Otherwise, it'll evaluate to false if someText contains any other string. \$\endgroup\$ – CertainPerformance Sep 24 at 21:50
  • \$\begingroup\$ Right. Since links are hidden when the textContent isn't empty, I believe the condition should be inverted then: const linksOriginallyHidden = !!styleTag.textContent;. However, I ran across the formatting not changing anymore when I used a double !!. I don't know if I have a bug somewhere else or if it has anything to do with the style tag being an object and casting its properties to a boolean. The funny thing is that when I use a single ! as in your script, it works but the icons are inverted (it shows a hidden-links icon when they aren't and vice-versa). \$\endgroup\$ – fabrizzio_gz Sep 24 at 22:06
  • \$\begingroup\$ Oops, linksOriginallyHidden should be assigned the truthyness of the original textContent, not the other way around. Make sure that the object property names match up, eg linksAreHidden in both the content script and background script. \$\endgroup\$ – CertainPerformance Sep 24 at 22:11
  • \$\begingroup\$ Yes, I found the problem. This assignment needed to be inverted as well: styleTag.textContent = linksOriginallyHidden ? '', hideLinksStyleText';. Thanks again for all of your observations! I'm working on implementing them. It's a nice improvement and I learned new things. \$\endgroup\$ – fabrizzio_gz Sep 24 at 22:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.