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

Aishat-Akinyemi · 2019-10-10T15:46:39Z

#outreachy applicant fixes #973 and #1513

I refactored badge.displayBrowserActionBadge to be used in messageHandler. However my various attempts to reuse it in pop.js resulted in error, as the 'browser Action Badge' when clicked did not display anything.

Test
I set the number of container tabs that's required to show 'achievement' to 5 (I have changed this back to hundred after testing). The tests were successful.

Problem reusing in pop.js
The picture below is how I attempted to make badge.displayBrowserActionBadge to be used in pop.js that resulted in unexpected behaviour, kindly review and advise me on what the bugs could be, thank you. I have removed this code below from the pull request

….js fixes #973

maxxcrawford

Be sure to run the linter. It will check the code and come back with errors for you to fix from the code you've added.

$ npm run lint

It's causing the build to fail.

Resolve submission Validation Warnings fixes #1513 #Outreachy2019/2020 Applicant

Aishat-Akinyemi · 2019-10-10T19:58:04Z

Okay, I will do that On Oct 10, 2019 17:31, "Maxx Crawford" <notifications@github.com> wrote: @Aishat-Akinyemi <https://github.com/Aishat-Akinyemi> Be sure to run the linter. It will check the code and come back with errors for you to fix from the code you've added. $ npm run lint It's causing the build to fail <https://travis-ci.org/mozilla/multi-account-containers/builds/596181518?utm_source=github_status&utm_medium=notification> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1523?email_source=notifications&email_token=AMUDKWEXT5M47IUHICWXYNLQN5KGVA5CNFSM4I7OY57KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA47AHA#issuecomment-540667932>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMUDKWH7YRMPV4WSYU5Z7U3QN5KGVANCNFSM4I7OY57A> .

Aishat-Akinyemi · 2019-10-10T20:01:58Z

Thanks for the feedback On Oct 10, 2019 20:58, wrote: Thanks for the feedback On Oct 10, 2019 17:31, "Maxx Crawford" <notifications@github.com> wrote: @Aishat-Akinyemi <https://github.com/Aishat-Akinyemi> Be sure to run the linter. It will check the code and come back with errors for you to fix from the code you've added. $ npm run lint It's causing the build to fail <https://travis-ci.org/mozilla/multi-account-containers/builds/596181518?utm_source=github_status&utm_medium=notification> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1523?email_source=notifications&email_token=AMUDKWEXT5M47IUHICWXYNLQN5KGVA5CNFSM4I7OY57KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA47AHA#issuecomment-540667932>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMUDKWH7YRMPV4WSYU5Z7U3QN5KGVANCNFSM4I7OY57A> .

…utreachy-fixes-#973

Aishat-Akinyemi · 2019-10-12T09:15:06Z

@maxx Crawford <mcrawford@mozilla.com> Hello, I have effected the changes yesterday and I have included commit that fixes issue #1513

…

On Thu, Oct 10, 2019 at 5:32 PM Maxx Crawford ***@***.***> wrote: ***@***.**** requested changes on this pull request. Be sure to run the linter. It will check the code and come back with errors for you to fix from the code you've added. $ npm run lint It's causing the build to fail <https://travis-ci.org/mozilla/multi-account-containers/builds/596181518?utm_source=github_status&utm_medium=notification> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1523?email_source=notifications&email_token=AMUDKWDU27XLSZAK2Q5YA73QN5KIRA5CNFSM4I7OY57KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHSLDWI#pullrequestreview-300200409>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMUDKWD2YCWCOKVCBK5BYMTQN5KIRANCNFSM4I7OY57A> .

groovecoder · 2019-10-16T15:56:37Z

Oh dang, this change has some conflicting changes now. @Aishat-Akinyemi - can you resolve them?

groovecoder · 2019-10-16T16:10:58Z

src/js/background/messageHandler.js

@@ -187,7 +181,7 @@ const messageHandler = {
    // https://bugzil.la/1314674
    // https://github.com/mozilla/testpilot-containers/issues/608
    // ... so re-call displayBrowserActionBadge on window changes
-    badge.displayBrowserActionBadge();
+    badge.displayBrowserActionBadge("remove");


This action is actually "showVersionIndicator". We (ab)use it to get around https://bugzilla.mozilla.org/show_bug.cgi?id=1314674 but it looks like that bug is fixed now? So we can just remove this call now.

@groovecoder okay, so I should only focus on refactoring badge.displayBrowserActionBadge to handle

when the user opens the 100th container and;

line 5 in badge.js

Yup, that sounds right. And line 5 in badge.js is the "showVersionIndicator" action.

thank you, I have effected the changes

groovecoder · 2019-10-16T16:11:34Z

src/js/background/badge.js

+      browser.browserAction.setBadgeBackgroundColor({color: color});
+      browser.browserAction.setBadgeText({text: text});
+    }
+    if(action==="remove") {    


Please change this action to showVersionIndicator and then update the call on line 5 to use it.

@groovecoder thanks for the feedback. Please tell me what is meant by showVersionIndicator do you mean I should change the parameter action to showVersionIndicator?

Does the showversionindicator: get the version of the browser tab (in line 5 that will be incognito and in the messageHandler.js, that will be the 100th container tab)?

Here, change it to action==="showVersionIndicator" and on line 5, change the call to pass "showVersionIndicator"

thank you, I have effected the changes

groovecoder · 2019-10-16T16:12:43Z

src/js/popup.js

@@ -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 


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

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

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.

Aishat-Akinyemi · 2019-10-17T22:46:05Z

Thanks for the guidance. I'll do that

…

On Oct 17, 2019 15:29, "luke crouch" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/js/background/badge.js <#1523 (comment)> : > 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==="remove") { Here, change it to action==="showVersionIndicator" and on line 5, change the call to pass "showVersionIndicator" — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1523?email_source=notifications&email_token=AMUDKWCLMQ4SZFOGQXL7QUDQPBZEVA5CNFSM4I7OY57KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCIKAF7Y#discussion_r336039282>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMUDKWCAYNUBF4UUNNKNGOLQPBZEVANCNFSM4I7OY57A> .

…ing badge.js

groovecoder · 2019-10-24T11:53:47Z

src/js/background/badge.js

-    const currentWindow = await browser.windows.getCurrent();
-    this.displayBrowserActionBadge(currentWindow.incognito);
+    const showVersionIndicator = await browser.windows.getCurrent();
+    this.displayBrowserActionBadge(showVersionIndicator);


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

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

jonathanKingston

Please remove the DOMParser changes as it reduces the ability to capture security issues.

kendallcorner · 2020-02-11T21:04:59Z

Looks like a new PR was submitted for this, so I will close, but let me know I am misunderstanding.

refactor badge.displayBrowserActionBadge to be used in messageHandler…

619b5f5

….js fixes #973

Aishat-Akinyemi mentioned this pull request Oct 10, 2019

Refactor badge.displayBrowserActionBadge to be used in both popup.js and messageHandler.js #973

Open

maxxcrawford requested changes Oct 10, 2019

View reviewed changes

Refactoring pop.js uses DOMParser instead of innerHTML fixes #1513

6a6e6d1

Resolve submission Validation Warnings fixes #1513 #Outreachy2019/2020 Applicant

Aishat-Akinyemi added 3 commits October 11, 2019 15:10

Merge branch '#outreachy-Aishat-Akinyemi-patch-1-fixes-#1513' into #o…

df11d27

…utreachy-fixes-#973

fixes #973

2046d41

fix travis errors

3036f99

Aishat-Akinyemi closed this Oct 11, 2019

Aishat-Akinyemi reopened this Oct 11, 2019

fix latest build errors

0ce57b0

Aishat-Akinyemi changed the title ~~refactor badge.displayBrowserActionBadge to be used in messageHandler…~~ refactor badge.displayBrowserActionBadge to be used in messageHandler fixes #973 & refactoring pop.js to fixes #1513 Oct 11, 2019

Aishat-Akinyemi requested a review from maxxcrawford October 12, 2019 09:15

groovecoder requested changes Oct 16, 2019

View reviewed changes

emoving call to displayBrowserActionBadge in messagehandler and updat…

e04a7b3

…ing badge.js

Aishat-Akinyemi mentioned this pull request Oct 24, 2019

refactor messageHandler.js to use badge.displayBrowserActionBadge #1557

Closed

groovecoder requested changes Oct 25, 2019

View reviewed changes

jonathanKingston suggested changes Jan 21, 2020

View reviewed changes

kendallcorner closed this Feb 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Aishat-Akinyemi commented Oct 10, 2019 •

edited

maxxcrawford left a comment

Aishat-Akinyemi commented Oct 10, 2019 via email

Aishat-Akinyemi commented Oct 10, 2019 via email

Aishat-Akinyemi commented Oct 12, 2019 via email

groovecoder commented Oct 16, 2019

groovecoder Oct 16, 2019

Aishat-Akinyemi Oct 17, 2019

groovecoder Oct 17, 2019

Aishat-Akinyemi Oct 18, 2019

groovecoder Oct 16, 2019

Aishat-Akinyemi Oct 17, 2019 •

edited

groovecoder Oct 17, 2019

Aishat-Akinyemi Oct 18, 2019

groovecoder Oct 16, 2019

Aishat-Akinyemi Oct 18, 2019

jonathanKingston Jan 21, 2020

Aishat-Akinyemi commented Oct 17, 2019 via email

groovecoder Oct 24, 2019

Aishat-Akinyemi Oct 26, 2019

jonathanKingston left a comment

kendallcorner commented Feb 11, 2020

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

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

Conversation

Aishat-Akinyemi commented Oct 10, 2019 • edited

#outreachy applicant fixes #973 and #1513

maxxcrawford left a comment

Choose a reason for hiding this comment

Aishat-Akinyemi commented Oct 10, 2019 via email

Aishat-Akinyemi commented Oct 10, 2019 via email

Aishat-Akinyemi commented Oct 12, 2019 via email

groovecoder commented Oct 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aishat-Akinyemi Oct 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aishat-Akinyemi commented Oct 17, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanKingston left a comment

Choose a reason for hiding this comment

kendallcorner commented Feb 11, 2020

Aishat-Akinyemi commented Oct 10, 2019 •

edited

Aishat-Akinyemi Oct 17, 2019 •

edited