Add Custom Sites to the Facebook Container #566

maxxcrawford · 2019-12-31T23:18:32Z

This PR adds buttons to the doorhanger panel for users to add/remove the active page/domain to the Facebook container.

Fixes #422
Fixes #550

PR To-do List

Update onboarding panel copy/images
Add tooltip text
Fix issue where "Add this site" shows up on default FB URLs (from FACEBOOK_DOMAINS)
Add test directions to this PR

Testing Steps

Use the following directions to verify this PR works as is.

Run the add-on. (This will open up the add-on in a temporary new browser)

$ web-ext run -s src/

Go to the following sites:

Non-Facebook tracking site:
- https://duckduckgo.com/ (There should be no Facebook trackers blocked on this site)
Facebook domain
- https://facebook.com/
Facebook tracking site
https://maxxcrawford.github.io/social-implementation-examples/facebook/login/
https://buzzfeed.com/

Once on the site, verify that trackers are detected as expected.
Click on the Facebook Container add-on icon within the browser chrome to trigger the panel:
Attempt to add that site to the Facebook Container:

Non-Facebook tracking site and Facebook tracking site should be able be to added by clicking the following button:

Once this button is clicked, the expected action is for the tab to close and reopen the same URL INSIDE the Facebook Container.

Once added, attempt to remove the newly added site from the Facebook Container:

Confirm you're in the Facebook Container
Click on the Facebook Container add-on icon within the browser chrome to trigger the panel:
- On default Facebook Container sites (like Facebook.com and Instagram.com), these sites cannot be removed.
- On non-default sites, it should close and reload the page OUTSIDE of the Facebook Container.

Screenshots

No trackers detected panel (outside of FB Container):

Trackers detected panel (outside of FB Container):

Custom site added to FB Container

On default Facebook domain:

On default Facebook domain (hover tooltip):

In-page UI Panel (Revised Copy + Image):

New Panel for Adding Custom Site:

…ncluding helper functions to determine which site user is on, and if that site is in the FBC

…cluding event listener(s) for the button

…d with

…n default FB domain URLs

maxxcrawford · 2020-01-02T22:21:07Z

src/panel.css

+  position: absolute;
+  left: 1rem;
+  bottom: 2em;


Note that this differs from the InVision mockup as the panel has overflow:hidden on it, not allowing for the tooltip to be visible outside of its' parent container.

InVision mockup:

cc/ @e-pang

…w UI for adding sites to the FB Container. This includes removing the previous strings (Panel 3)

…age from the directory.

maxxcrawford · 2020-01-02T23:03:00Z

@e-pang Note that I updated the CSS to bold the first sentence in each panel, per the InVision mockups:

ddurst · 2020-01-03T16:24:03Z

I'm struggling with the strings in this a bit, but maybe it's just me.

When we talk about "sites allowed in FBC," it sounds to me like we're putting other things in that container. Phrased like that, it sounds like we're just lumping sites together, or putting things in the same container. Maybe this is a technical detail (maybe it's not), but for the user, the potential ramifications aren't clearly communicated.

To me, "sites allowed" means that Facebook can access your data (I'm deliberately oversimplifying) -- if we're trying to make it clear that FB is contained/isolated, what "allowing" a site means to me is that the user is allowing FB to potentially access your data through/on any site you "allow."

I'd expect this to be more like "Don't block Facebook trackers on these sites" or something like that -- not to sound too opinionated, but not to softball it either.

ddurst · 2020-01-03T16:38:00Z

(I don't know that we can change this, but in the future perhaps we could/should.)

src/_locales/en/messages.json

kendallcorner

Looks pretty good

src/panel.js

kendallcorner · 2020-01-10T04:20:11Z

Also, we've already discussed a little that this has potential to conflict with MAC.

When I have a site assigned in MAC, and then try to add it to fbc, it doesn't take. It just keeps getting opened in my MAC container. I believe we can use the messaging already setup (sendJailedDomainsToMAC()) to send these new sites to MAC as well.

groovecoder

all nits except 2 of my comments.

groovecoder · 2020-01-23T16:02:40Z

src/panel.js

+  }
+
+  const addedSitesList = await browser.runtime.sendMessage("what-sites-are-added");
+  const hostname = await getActiveHostname();


nit: const activeTabHostname = await getActiveTabHostname();

groovecoder · 2020-01-23T16:03:51Z

src/panel.js

+
+  if (addedSitesList.includes(hostname)) {
+    return true;
+  } else {


nit: drop the else - return true will exit the function so the else block is not needed. I trim a lot of else blocks out of my code just for readability.

groovecoder · 2020-01-23T16:06:31Z

src/panel.js

+
+  if (panelId === "no-trackers") {
+    await addCustomSiteButton(fragment, panelId);
+  }


This is called below on line 369 whether or not panelId === "no-trackers" - so do we need this if block?

Yes – It's a quirk for how its' rendered in the panel. I tried different combinations/ordering of these.

groovecoder · 2020-01-23T16:09:17Z

src/panel.js

@@ -116,6 +149,37 @@ const addLearnHowFBCWorksButton = async (fragment) => {
  addSpan(button, "sites-added-subhead");
 };

+const addCustomSiteButton = async (fragment, panelId) => {
+  const shouldShowRemoveSiteButton = await isSiteInContainer(panelId);
+  let button;


We don't return this value from the function, so we can probably drop it?

const addCustomSiteButton = async (fragment, panelId) => { const shouldShowRemoveSiteButton = await isSiteInContainer(panelId); if (shouldShowRemoveSiteButton) { addFullWidthButton(fragment, "remove-site-from-container"); addSpan(button, "button-remove-site"); addTooltip(button, "button-remove-site-tooltip"); return; } addFullWidthButton(fragment, "add-site-to-container"); addSpan(button, "button-allow-site"); }

The button is passed as an argument in both the addSpan and addTooltip functions. I think we need to keep it.

groovecoder · 2020-01-23T16:11:05Z

src/panel.js

+
+  if (shouldShowRemoveSiteButton) {
+    const removeSiteFromContainerLink = document.querySelector(".remove-site-from-container");
+    removeSiteFromContainerLink.addEventListener("click", async () => removeSiteFromContainer());


nit: can remove the else block here with a return; early statement, like in the above if (panelId === "on-facebook") { block.

groovecoder · 2020-01-23T16:11:51Z

src/panel.js

+    if (res === 4) {
+      // This accounts for the skipped Panel #3
+      buildOnboardingPanel(2);
+    } else {


nit: remove else block with an early return;

groovecoder · 2020-01-23T16:13:03Z

src/panel.js

@@ -256,7 +325,7 @@ document.addEventListener("DOMContentLoaded", async () => {

  const onboarding = (currentPanel.includes("onboarding"));
  if (!onboarding) {
-    return buildPanel(currentPanel);
+    return await buildPanel(currentPanel);


nit: don't really need to return await

Updated variable names to provide more context Resolved "return await" issue Consolidated buildPanel function if statements

groovecoder

Hmm ... I don't think this bug is related to the PR - it happens on master too - but the badge shows a warning in duckduckgo.com now?

It looks like blockFacebookSubResources only ever sets trackersDetected: true? I.e., it doesn't "reset" trackersDetected: false? So as soon as a tab gets marked with trackersDetected: true the browserAction icon badge is stuck in that state?

The fix seems to be to add a trackersDetected: false line inside the requestDetails.type === "main_frame" block of blackFacebookSubResources ...

    if (requestDetails.type === "main_frame") {
+     tabStates[requestDetails.tabId] = { trackersDetected: false };
      return {};
    }

Can be fixed in a separate PR?

maxxcrawford added 7 commits December 31, 2019 16:53

Added button styles and icons for Add/Remove site to Container button

06dad5e

Core feature functions - add and remove custom site from container, i…

02a40e8

…ncluding helper functions to determine which site user is on, and if that site is in the FBC

Build and append the add/remove button to the FB doorhanger panel, in…

e2868d2

…cluding event listener(s) for the button

Updated buildPanel call to reflect async function type

434168a

Reload the active tab after removing the site from FBC

904c20b

Remove panel notification (purple square) once the panel is interacte…

98fb9c6

…d with

Added logic/styles to disable the "remove site from container" when o…

7bffd8f

…n default FB domain URLs

maxxcrawford commented Jan 2, 2020

View reviewed changes

maxxcrawford added 2 commits January 2, 2020 16:55

Updated onboarding panels (JS, Strings, Images and CSS) to reflect ne…

7656486

…w UI for adding sites to the FB Container. This includes removing the previous strings (Panel 3)

Updated image for revised Panel 3 (now Panel 4) and removed unused im…

f51a948

…age from the directory.

maxxcrawford marked this pull request as ready for review January 3, 2020 21:12

maxxcrawford requested a review from flodolo as a code owner January 3, 2020 21:12

maxxcrawford requested review from lesleyjanenorton and groovecoder January 3, 2020 21:12

flodolo approved these changes Jan 4, 2020

View reviewed changes

src/_locales/en/messages.json Outdated Show resolved Hide resolved

maxxcrawford requested a review from kendallcorner January 6, 2020 17:12

maxxcrawford mentioned this pull request Jan 7, 2020

Suggestion: Temporary allow manual website addition into the container #370

Open

kendallcorner suggested changes Jan 10, 2020

View reviewed changes

src/panel.js Show resolved Hide resolved

src/panel.js Show resolved Hide resolved

src/panel.js Show resolved Hide resolved

src/panel.js Show resolved Hide resolved

mrfifiak mentioned this pull request Jan 23, 2020

In-content panel enabling users to "allow FB to track me on this site" #422

Closed

groovecoder requested changes Jan 23, 2020

View reviewed changes

maxxcrawford added 2 commits February 12, 2020 16:14

Updated straight single quote to closing single quote

b79be5e

Updated if statements to not depend on the else clause

dcc846c

Updated variable names to provide more context Resolved "return await" issue Consolidated buildPanel function if statements

maxxcrawford requested review from groovecoder, kendallcorner and flodolo and removed request for lesleyjanenorton February 13, 2020 16:10

maxxcrawford requested a review from lesleyjanenorton February 13, 2020 16:10

groovecoder approved these changes Feb 13, 2020

View reviewed changes

flodolo approved these changes Feb 13, 2020

View reviewed changes

Bumped version number for new feature designation

72be4e9

kendallcorner approved these changes Feb 13, 2020

View reviewed changes

groovecoder merged commit 5742261 into master Feb 13, 2020

groovecoder deleted the 422-add-custom-sites branch February 13, 2020 21:25

maxxcrawford mentioned this pull request Feb 14, 2020

Clear browserAction between tabs #581

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Custom Sites to the Facebook Container #566

Add Custom Sites to the Facebook Container #566

maxxcrawford commented Dec 31, 2019 •

edited

Loading

maxxcrawford Jan 2, 2020

maxxcrawford commented Jan 2, 2020

ddurst commented Jan 3, 2020

ddurst commented Jan 3, 2020

kendallcorner left a comment

kendallcorner commented Jan 10, 2020

groovecoder left a comment

groovecoder Jan 23, 2020

groovecoder Jan 23, 2020

groovecoder Jan 23, 2020

maxxcrawford Feb 12, 2020

groovecoder Jan 23, 2020

maxxcrawford Feb 12, 2020

groovecoder Jan 23, 2020

groovecoder Jan 23, 2020

groovecoder Jan 23, 2020

groovecoder left a comment

Add Custom Sites to the Facebook Container #566

Add Custom Sites to the Facebook Container #566

Conversation

maxxcrawford commented Dec 31, 2019 • edited Loading

Testing Steps

Screenshots

Choose a reason for hiding this comment

maxxcrawford commented Jan 2, 2020

ddurst commented Jan 3, 2020

ddurst commented Jan 3, 2020

kendallcorner left a comment

Choose a reason for hiding this comment

kendallcorner commented Jan 10, 2020

groovecoder left a 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

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

groovecoder left a comment

Choose a reason for hiding this comment

maxxcrawford commented Dec 31, 2019 •

edited

Loading