Skip to content

refactor badge.displayBrowserActionBadge to be used in messageHandler fixes #973 & refactoring pop.js to fixes #1513 #1523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
31 changes: 22 additions & 9 deletions src/js/background/badge.js
Original file line number Diff line number Diff line change
@@ -1,25 +1,38 @@
const MAJOR_VERSIONS = ["2.3.0", "2.4.0"];
const badge = {
async init() {
const currentWindow = await browser.windows.getCurrent();
this.displayBrowserActionBadge(currentWindow.incognito);
const showVersionIndicator = await browser.windows.getCurrent();
this.displayBrowserActionBadge(showVersionIndicator);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to pass the string "showVersionIndicator" here, and you don't need the currentWindow variable anymore, because it was only used to pass the incognito property value, which isn't needed anymore. So you can just replace these 2 lines with:

this.displayBrowserActionBadge("showVersionIndicator");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have done this. I opened a new PR #1561 that fixes #973 this issue only (and all the changes you requested in this PR have been implemented there).

I plan to submit a new PR that addresses #1513

},

disableAddon(tabId) {
browser.browserAction.disable(tabId);
browser.browserAction.setTitle({ tabId, title: "Containers disabled in Private Browsing Mode" });
},

async displayBrowserActionBadge() {
async displayBrowserActionBadge(action) {
const extensionInfo = await backgroundLogic.getExtensionInfo();
const storage = await browser.storage.local.get({browserActionBadgesClicked: []});

if (MAJOR_VERSIONS.indexOf(extensionInfo.version) > -1 &&
storage.browserActionBadgesClicked.indexOf(extensionInfo.version) < 0) {
browser.browserAction.setBadgeBackgroundColor({color: "rgba(0,217,0,255)"});
browser.browserAction.setBadgeText({text: "NEW"});
function changeBadgeColorText(color, text){
browser.browserAction.setBadgeBackgroundColor({color: color});
browser.browserAction.setBadgeText({text: text});
}
if(action==="showVersionIndicator") {
const ActionBadgesClickedStorage = await browser.storage.local.get({browserActionBadgesClicked: []});
if (MAJOR_VERSIONS.indexOf(extensionInfo.version) > -1 &&
ActionBadgesClickedStorage.browserActionBadgesClicked.indexOf(extensionInfo.version) < 0) {
changeBadgeColorText("rgba(0,217,0,255)", "NEW");
}
}
else if (action==="showAchievement") {
const achievementsStorage = await browser.storage.local.get({achievements: []});
achievementsStorage.achievements.push({"name": "manyContainersOpened", "done": false});
// use set and spread to create a unique array
const achievements = [...new Set(achievementsStorage.achievements)];
browser.storage.local.set({achievements});
changeBadgeColorText("rgba(0,217,0,255)", "NEW");
}
}

};

badge.init();
13 changes: 1 addition & 12 deletions src/js/background/messageHandler.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,23 +171,12 @@ const messageHandler = {

// When the user opens their _ tab, give them the achievement
if (countOfContainerTabsOpened === 100) {
const storage = await browser.storage.local.get({achievements: []});
storage.achievements.push({"name": "manyContainersOpened", "done": false});
// use set and spread to create a unique array
const achievements = [...new Set(storage.achievements)];
browser.storage.local.set({achievements});
browser.browserAction.setBadgeBackgroundColor({color: "rgba(0,217,0,255)"});
browser.browserAction.setBadgeText({text: "NEW"});
badge.displayBrowserActionBadge("showAchievement");
}
},

async onFocusChangedCallback(windowId) {
assignManager.removeContextMenu();
// browserAction loses background color in new windows ...
// https://bugzil.la/1314674
// https://github.com/mozilla/testpilot-containers/issues/608
// ... so re-call displayBrowserActionBadge on window changes
badge.displayBrowserActionBadge();
browser.tabs.query({active: true, windowId}).then((tabs) => {
if (tabs && tabs[0]) {
assignManager.calculateContextMenu(tabs[0]);
Expand Down
70 changes: 60 additions & 10 deletions src/js/popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const Logic = {
async init() {
// Remove browserAction "upgraded" badge when opening panel
this.clearBrowserActionBadge();

// Retrieve the list of identities.
const identitiesPromise = this.refreshIdentities();

Expand Down Expand Up @@ -610,7 +610,15 @@ Logic.registerPanel(P_CONTAINERS_LIST, {
const siteSettings = await Logic.getAssignment(currentTab);
this.setupAssignmentCheckbox(siteSettings, currentTabUserContextId);
const currentPage = document.getElementById("current-page");
currentPage.innerHTML = escaped`<span class="page-title truncate-text">${currentTab.title}</span>`;
//using DOMParser to modify innerHTML
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonathanKingston - is DOMParser the best replacement for innerHTML assignment?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I create a separate PR for the issues, I thought I could submit them both but since you are still assessing whether DOMParser is the best replacement what would you advise that I do?
I also think that it would make them count as 2 separate contributions on Outreachy.
Thank you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't ever be used, it's not any safer than innerHTML in fact it breaks the linting which captures when we use escaped. I don't think this is a good idea at all.

const htmlText = escaped`<span class="page-title truncate-text">${currentTab.title}</span>`;
const parser = new DOMParser();
const parsed = parser.parseFromString(htmlText, "text/html");
const tags = parsed.getElementsByTagName("body");
for (const tag of tags) {
currentPage.appendChild(tag);
}

const favIconElement = Utils.createFavIconElement(currentTab.favIconUrl || "");
currentPage.prepend(favIconElement);

Expand Down Expand Up @@ -640,14 +648,21 @@ Logic.registerPanel(P_CONTAINERS_LIST, {
manage.title = escaped`View ${identity.name} container`;
context.setAttribute("tabindex", "0");
context.title = escaped`Create ${identity.name} tab`;
context.innerHTML = escaped`
const htmlText = escaped`
<div class="userContext-icon-wrapper open-newtab">
<div class="usercontext-icon"
data-identity-icon="${identity.icon}"
data-identity-color="${identity.color}">
</div>
</div>
<div class="container-name truncate-text"></div>`;
const parser = new DOMParser();
const parsed = parser.parseFromString(htmlText, "text/html");
const tags = parsed.getElementsByTagName("body");
for (const tag of tags) {
context.appendChild(tag);
}

context.querySelector(".container-name").textContent = identity.name;
manage.innerHTML = "<img src='/img/container-arrow.svg' class='show-tabs pop-button-image-small' />";

Expand Down Expand Up @@ -803,9 +818,16 @@ Logic.registerPanel(P_CONTAINER_INFO, {
const tr = document.createElement("tr");
fragment.appendChild(tr);
tr.classList.add("container-info-tab-row");
tr.innerHTML = escaped`
const htmlText= escaped`
<td></td>
<td class="container-info-tab-title truncate-text" title="${tab.url}" ><div class="container-tab-title">${tab.title}</div></td>`;
const parser = new DOMParser();
const parsed = parser.parseFromString(htmlText, "text/html");
const tags = parsed.getElementsByTagName("body");
for (const tag of tags) {
tr.appendChild(tag);
}

tr.querySelector("td").appendChild(Utils.createFavIconElement(tab.favIconUrl));
document.getElementById("container-info-table").appendChild(fragment);

Expand Down Expand Up @@ -866,7 +888,7 @@ Logic.registerPanel(P_CONTAINERS_EDIT, {
const tr = document.createElement("tr");
fragment.appendChild(tr);
tr.classList.add("container-panel-row");
tr.innerHTML = escaped`
const htmlText = escaped`
<td class="userContext-wrapper">
<div class="userContext-icon-wrapper">
<div class="usercontext-icon"
Expand All @@ -887,6 +909,13 @@ Logic.registerPanel(P_CONTAINERS_EDIT, {
src="/img/container-delete.svg"
/>
</td>`;
const parser = new DOMParser();
const parsed = parser.parseFromString(htmlText, "text/html");
const tags = parsed.getElementsByTagName("body");
for (const tag of tags) {
tr.appendChild(tag);
}

tr.querySelector(".container-name").textContent = identity.name;
tr.querySelector(".edit-container").setAttribute("title", `Edit ${identity.name} container`);
tr.querySelector(".remove-container").setAttribute("title", `Remove ${identity.name} container`);
Expand Down Expand Up @@ -986,7 +1015,7 @@ Logic.registerPanel(P_CONTAINER_EDIT, {
/* As we don't have the full or correct path the best we can assume is the path is HTTPS and then replace with a broken icon later if it doesn't load.
This is pending a better solution for favicons from web extensions */
const assumedUrl = `https://${site.hostname}/favicon.ico`;
trElement.innerHTML = escaped`
const htmlText = escaped`
<div class="favicon"></div>
<div title="${site.hostname}" class="truncate-text hostname">
${site.hostname}
Expand All @@ -995,6 +1024,13 @@ Logic.registerPanel(P_CONTAINER_EDIT, {
class="pop-button-image delete-assignment"
src="/img/container-delete.svg"
/>`;
const parser = new DOMParser();
const parsed = parser.parseFromString(htmlText, "text/html");
const tags = parsed.getElementsByTagName("body");
for (const tag of tags) {
trElement.appendChild(tag);
}

trElement.getElementsByClassName("favicon")[0].appendChild(Utils.createFavIconElement(assumedUrl));
const deleteButton = trElement.querySelector(".delete-assignment");
const that = this;
Expand Down Expand Up @@ -1023,8 +1059,15 @@ Logic.registerPanel(P_CONTAINER_EDIT, {
colors.forEach((containerColor) => {
const templateInstance = document.createElement("div");
templateInstance.classList.add("radio-container");
// eslint-disable-next-line no-unsanitized/property
templateInstance.innerHTML = colorRadioTemplate(containerColor);
// eslint-disable-next-line no-unsanitized/property
const htmlText = colorRadioTemplate(containerColor);
const parser = new DOMParser();
const parsed = parser.parseFromString(htmlText, "text/html");
const tags = parsed.getElementsByTagName("body");
for (const tag of tags) {
templateInstance.appendChild(tag);
}

colorRadioFieldset.appendChild(templateInstance);
});

Expand All @@ -1038,7 +1081,14 @@ Logic.registerPanel(P_CONTAINER_EDIT, {
const templateInstance = document.createElement("div");
templateInstance.classList.add("radio-container");
// eslint-disable-next-line no-unsanitized/property
templateInstance.innerHTML = iconRadioTemplate(containerIcon);
const htmlText= iconRadioTemplate(containerIcon);
const parser = new DOMParser();
const parsed = parser.parseFromString(htmlText, "text/html");
const tags = parsed.getElementsByTagName("body");
for (const tag of tags) {
templateInstance.appendChild(tag);
}

iconRadioFieldset.appendChild(templateInstance);
});
},
Expand Down Expand Up @@ -1154,4 +1204,4 @@ window.addEventListener("resize", function () {
root.style.setProperty("--overflow-size", difference + "px");
root.style.setProperty("--icon-fit", "12");
}
});
});