Issue 5326 - General tab (HTML, strings and functionality)

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -25,6 +25,8 @@
   let filtersMap = Object.create(null);
   let collections = Object.create(null);
   let acceptableAdsUrl = null;
+  let acceptableAdsPrivacyUrl = null;
+  let subscriptionToChange = null;
   let isCustomFiltersLoaded = false;
   let {getMessage} = ext.i18n;
   let customFilters = [];
@@ -44,6 +46,8 @@
   const minuteInMs = 60000;
   const hourInMs = 3600000;
   const fullDayInMs = 86400000;
+  const privacySubscriptions = ["privacy", "social"];
+  const moreSubscriptions = ["malware", "anti-adblock"];
 
   function Collection(details)
   {
@@ -83,8 +87,10 @@
 
   Collection.prototype._getItemTitle = function(item, i)
   {
-    if (item.url == acceptableAdsUrl)
-      return getMessage("options_acceptableAds_description");
+    if (item.url === acceptableAdsUrl)

[Thomas Greiner, 2017/08/09 18:14:49]

Do we even need this special handling of Acceptable Ads subscriptions?

We introduced it in #3907 but only because it its URL showed up but that may no
longer be the case. So unless this still occurs or we need the subscription
title to be translatable, we can get rid of this check.

[saroyanm, 2017/08/14 14:00:10]

I think that we want the title to be translated, also the title is slightly
different in spec it's Allow "some"...  the word "some" is missing in the
original title.

[Thomas Greiner, 2017/08/15 17:10:29]

What's so bad about showing the original (i.e. untranslated) title that's
defined in the filter list itself? It's only going to show up in the Advanced
tab anyway AFAIK. There we also show the original titles for all the other
filter lists.

We may want to think about changing the title inside the filter list because
"Allow non-intrusive advertising" is not an actual name but a description.

[saroyanm, 2017/08/16 14:17:38]

Nevermind I totally agree, I thought that this also applies to the title in the
General tab, but now I see that it was about the Advanced tab.

+      return getMessage("options_aa_tracking_label");

[Thomas Greiner, 2017/08/09 18:14:49]

Detail: Let's be consistent with the naming scheme to clarify if things belong
together. In this case you have `acceptableAdsUrl` and `acceptableAdsPrivacyUrl`
so why not name the message IDs accordingly as "options_acceptableAds_label" and
"options_acceptableAds_privacy_label".

[saroyanm, 2017/08/14 14:00:09]

Agree, I'll change.

[saroyanm, 2017/08/16 14:17:38]

Done.

+    if (item.url === acceptableAdsPrivacyUrl)
+      return getMessage("options_aa_no_tracking_label");
     if (this.details[i].useOriginalTitle && item.originalTitle)
       return item.originalTitle;
     return item.title || item.url || item.text;
@@ -98,9 +104,9 @@
       // disabled, but only be removed. That way it's grouped together with
       // the "Own filter list" which cannot be disabled either at the bottom
       // of the filter lists in the Advanced tab.
-      if (a.url == acceptableAdsUrl)
+      if (a.url == acceptableAdsUrl || a.url == acceptableAdsPrivacyUrl)
         return 1;
-      if (b.url == acceptableAdsUrl)
+      if (b.url == acceptableAdsUrl || b.url == acceptableAdsPrivacyUrl)
         return -1;
 
       // Make sure that newly added entries always appear on top in descending
@@ -134,12 +140,19 @@
       listItem.setAttribute("data-access", item.url || item.text);
       listItem.setAttribute("role", "section");
 
-      let label = listItem.querySelector(".display");
-      if (item.recommended && label.hasAttribute("data-tooltip"))
+      let tooltip = listItem.querySelector("[data-tooltip]");
+      if (tooltip && tooltip.hasAttribute("data-tooltip"))

[Thomas Greiner, 2017/08/09 18:14:48]

Detail: The second condition is redundant because `tooltip` is an element that
matches the selector `[data-tooltip]`.

[saroyanm, 2017/08/14 14:00:10]

Well spotted.

[saroyanm, 2017/08/16 14:17:38]

Done.

       {
-        let tooltipId = label.getAttribute("data-tooltip");
-        tooltipId = tooltipId.replace("%value%", item.recommended);
-        label.setAttribute("data-tooltip", tooltipId);
+        if (item.recommended)

[Thomas Greiner, 2017/08/09 18:14:48]

According to the spec, not all subscriptions that have a tooltip are also
recommended.

Recommended:
- Subscriptions of type "ads"
- Fanboy's Annoyances
- EasyPrivacy

Tooltips:
- Adware filter
- Malware Domains
- Spam404
- Adblock Warning Removal List

So we need to either change the implementation or the spec.

[saroyanm, 2017/08/14 14:00:08]

I agree, make sense to first check if the string for tooltip exists.

[saroyanm, 2017/08/16 14:17:37]

Done.

+        {
+          let tooltipId = tooltip.getAttribute("data-tooltip");
+          tooltipId = tooltipId.replace("%value%", item.recommended);
+          tooltip.setAttribute("data-tooltip", tooltipId);
+        }
+        else
+        {
+          tooltip.parentNode.removeChild(tooltip);
+        }
       }
 
       for (let control of listItem.querySelectorAll(".control"))
@@ -222,7 +235,9 @@
       if (control)
       {
         control.setAttribute("aria-checked", item.disabled == false);
-        if (item.url == acceptableAdsUrl && this == collections.filterLists)
+        if ((item.url == acceptableAdsUrl ||
+          item.url == acceptableAdsPrivacyUrl) &&

[Thomas Greiner, 2017/08/09 18:14:50]

Detail: This is already the third time that you're doing this check so let's
create a function for that instead.

[saroyanm, 2017/08/14 14:00:10]

agree.

[saroyanm, 2017/08/16 14:17:36]

Done.

+          this == collections.filterLists)
           control.disabled = true;
       }
 
@@ -330,31 +345,26 @@
     return true;
   }
 
-  collections.popular = new Collection([
+  collections.security = new Collection([
     {
-      id: "recommend-list-table"
+      id: "recommend-security-list-table"
     }
   ]);
   collections.langs = new Collection([
     {
       id: "blocking-languages-table",
-      emptyText: ["options_dialog_language_added_empty"]
-    },
-    {
-      id: "blocking-languages-dialog-table",
-      emptyText: ["options_dialog_language_added_empty"]
+      emptyText: ["options_language_empty"],
+      switchSingleEntryControl: true

[Thomas Greiner, 2017/08/09 18:14:48]

Detail: This property appears to be unused.

[saroyanm, 2017/08/14 14:00:09]

right, will remove.

[saroyanm, 2017/08/16 14:17:38]

Done.

     }
   ]);
   collections.allLangs = new Collection([
     {
-      id: "all-lang-table",
-      emptyText: ["options_dialog_language_other_empty"],
-      searchable: true
-    }
-  ]);
-  collections.acceptableAds = new Collection([
+      id: "all-lang-table-add",
+      emptyText: ["options_dialog_language_other_empty"]
+    },
     {
-      id: "acceptableads-table"
+      id: "all-lang-table-change",
+      emptyText: ["options_dialog_language_other_empty"]
     }
   ]);
   collections.custom = new Collection([
@@ -375,20 +385,21 @@
     }
   ]);
 
-  function toggleShowLanguage(subscription)
+  function toggleShowRecommendation(subscription)
   {
     if (subscription.recommended == "ads")
     {
       if (subscription.disabled)
-      {
-        collections.allLangs.addItem(subscription);
         collections.langs.removeItem(subscription);
-      }
       else
-      {
-        collections.allLangs.removeItem(subscription);
         collections.langs.addItem(subscription);
-      }
+    }
+
+    if (moreSubscriptions.indexOf(subscription.recommended) >= 0 &&

[Thomas Greiner, 2017/08/09 18:14:50]

This is the opposite of what the spec says: "All filter lists that a user
subscribes to which are not one of Adblock Plus's recommended filter lists"

So none of the recommended filter lists should appear in the "More Filters"
section.

[saroyanm, 2017/08/14 14:00:08]

Hmmm, good point.. I'll update this.

[saroyanm, 2017/08/16 14:17:36]

Done.

+      subscription.disabled == false)
+    {
+      collections.custom.addItem(subscription);
+      updateTooltips();
     }
   }
 
@@ -397,21 +408,36 @@
     let collection;
     if (subscription.recommended)
     {
-      if (subscription.recommended != "ads")
-        collection = collections.popular;
+      if (privacySubscriptions.indexOf(subscription.recommended) >= 0)

[Thomas Greiner, 2017/08/09 18:14:49]

Detail: This naming doesn't make sense. You're putting "privacy subscriptions"
into a "security collection".

The section is called "Privacy & Security" so I don't think either is
representative of this section so it'd be great if we could come up with a more
generic and more accurate name (e.g. "protection"). Otherwise we could always
fall back to "additional", "more" or "other".

[Thomas Greiner, 2017/08/09 18:14:50]

Detail: You're only using this variable here and we don't expect to change its
value anytime soon so why not just replace it with `if (subscription.recommended
== "privacy" || subscription.recommended == "security"` to simplify the code?

[saroyanm, 2017/08/14 14:00:09]

Good point.

[saroyanm, 2017/08/16 14:17:36]

Done.

[saroyanm, 2017/08/16 14:17:36]

Not anymore

+        collection = collections.security;
+      else if (subscription.recommended == "ads")
+      {
+        if (subscription.disabled == false)
+          collection = collections.langs;
+        else
+          collection = collections.allLangs;
+      }
       else if (subscription.disabled == false)
-        collection = collections.langs;
+      {
+        collection = collections.custom;
+      }
       else
-        collection = collections.allLangs;
+      {
+        subscriptionsMap[subscription.url] = subscription;
+        return;
+      }
     }
-    else if (subscription.url == acceptableAdsUrl)
-      collection = collections.acceptableAds;
+    else if (subscription.url == acceptableAdsUrl ||
+      subscription.url == acceptableAdsPrivacyUrl)
+    {
+      return;

[Thomas Greiner, 2017/08/09 18:14:50]

Doesn't this mean that neither of the Acceptable Ads subscriptions will show up
in any of the subscription lists?

[saroyanm, 2017/08/14 14:00:09]

I think I tried to be consistent with previous implementation as we separately
were adding acceptable ads subscription into "custom" collection, but not using
current function.. I think we should be safe to move that implementation here..
I'm not sure why we were adding to custom collection separately, I'll check if
that change will have unexpected implication.

[Thomas Greiner, 2017/08/15 17:10:29]

Ok, thanks.

[saroyanm, 2017/08/16 14:17:37]

I did updated the implementation on how subscriptions are added and updated, I
guess now implementation makes more sense.

+    }
     else
       collection = collections.custom;
 
     collection.addItem(subscription);
     subscriptionsMap[subscription.url] = subscription;
-    toggleShowLanguage(subscription);
+    toggleShowRecommendation(subscription);
     updateTooltips();
   }
 
@@ -420,7 +446,7 @@
     for (let name in collections)
       collections[name].updateItem(subscription);
 
-    toggleShowLanguage(subscription);
+    toggleShowRecommendation(subscription);
   }
 
   function updateFilter(filter)
@@ -558,15 +584,57 @@
         closeDialog();
         break;
       }
+      case "block-all":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: acceptableAdsPrivacyUrl
+        });
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: acceptableAdsUrl
+        });
+        setDntNotification(false);

[Thomas Greiner, 2017/08/09 18:14:48]

Placing this here may cause inconsistent behavior because it's not guaranteed
that the extension will actually succeed in downloading, parsing and storing
both filter lists.

Therefore this needs to be placed in `onSubscriptionMessage()` which is where we
get notified that the subscription has been installed successfully.

On that note, please keep in mind that we also need to reset the value of the
radio buttons if the subscription installation fails.

[saroyanm, 2017/08/14 14:00:09]

Noted

[saroyanm, 2017/08/16 14:17:37]

Done.

+        break;
       case "cancel-custom-filters":
         setCustomFiltersView("read");
         break;
+      case "change-language-subscription":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: subscriptionToChange

[Thomas Greiner, 2017/08/09 18:14:50]

This variable seems redundant because you should be able to determine that at
runtime since it's the only language subscription that the user has installed.

[saroyanm, 2017/08/14 14:00:09]

agree.

[saroyanm, 2017/08/16 14:17:38]

Done.

+        });
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.add",
+          url: findParentData(element, "access", false)
+        });
+        break;

[Thomas Greiner, 2017/08/09 18:14:49]

What if one of those two actions fails? In theory we could end up with no filter
list being installed at all. So far we've never had two actions that were
dependent on each other so we'd have to implement some kind of transactions or
rethink our approach.

Obviously, the same problems applies to switching Acceptable Ads filter lists.

[saroyanm, 2017/08/14 14:00:10]

We still have a button for adding subscription, I think it make sense to keep 
robust error handling with notification, to the separate issue.

[Thomas Greiner, 2017/08/15 17:10:29]

That's fine with me as long as we make sure that this gets tackled so that we
not only communicate such errors to the user but also properly recover from
them.

       case "close-dialog":
         closeDialog();
         break;
       case "edit-custom-filters":
         setCustomFiltersView("write");
         break;
+      case "enable-aa":

[saroyanm, 2017/07/26 20:56:50]

Adding acceptableAds multiple times, causes the "All filters list" to add
acceptableAds multiple times. This still needs to be fixed.

[Thomas Greiner, 2017/08/09 18:14:48]

Acknowledged.

+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: acceptableAdsPrivacyUrl
+        });
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.add",
+          url: acceptableAdsUrl
+        });
+        setDntNotification(false);
+        break;
+      case "enable-privacy-aa":

[saroyanm, 2017/07/26 20:56:50]

The "Acceptable Ads notification" I think will be better to implement in
separate review, to keep current one small and I still have to figure some
implementation details.

[Thomas Greiner, 2017/08/09 18:14:49]

Acknowledged.

+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: acceptableAdsUrl
+        });
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.add",
+          url: acceptableAdsPrivacyUrl
+        });
+        break;
       case "import-subscription": {
         let url = E("blockingList-textbox").value;
         addEnableSubscription(url);
@@ -601,6 +669,9 @@
           url: findParentData(element, "access", false)
         });
         break;
+      case "save-change-subscription":
+        subscriptionToChange = findParentData(element, "access", false);
+        break;
       case "save-custom-filters":
         sendMessageHandleErrors({
           type: "filters.importRaw",
@@ -788,17 +859,6 @@
   function onDOMLoaded()
   {
     populateLists();
-    function onFindLanguageKeyUp()
-    {
-      let searchStyle = E("search-style");
-      if (!this.value)
-        searchStyle.innerHTML = "";
-      else
-      {
-        searchStyle.innerHTML = "#all-lang-table li:not([data-search*=\"" +
-          this.value.toLowerCase() + "\"]) { display: none; }";
-      }
-    }
 
     // Initialize navigation sidebar
     ext.backgroundPage.sendMessage({
@@ -820,9 +880,6 @@
     // Initialize interactive UI elements
     document.body.addEventListener("click", onClick, false);
     document.body.addEventListener("keyup", onKeyUp, false);
-    let placeholderValue = getMessage("options_dialog_language_find");
-    E("find-language").setAttribute("placeholder", placeholderValue);
-    E("find-language").addEventListener("keyup", onFindLanguageKeyUp, false);
     let exampleValue = getMessage("options_whitelist_placeholder_example",
       ["www.example.com"]);
     E("whitelisting-textbox").setAttribute("placeholder", exampleValue);
@@ -831,6 +888,11 @@
       E("whitelisting-add-button").disabled = !e.target.value;
     }, false);
 
+    getDocLink("acceptable_ads_criteria", (link) =>
+    {
+      setLinks("enable-aa-description", link);
+    });
+
     // Advanced tab
     let customize = document.querySelectorAll("#customize li[data-pref]");
     customize = Array.prototype.map.call(customize, (checkbox) =>
@@ -969,6 +1031,14 @@
     focusedBeforeDialog.focus();
   }
 
+  function setDntNotification(state)
+  {
+    if (state)
+      E("acceptable-ads").classList.add("show-dnt-notification");
+    else
+      E("acceptable-ads").classList.remove("show-dnt-notification");
+  }
+
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
@@ -1014,15 +1084,28 @@
         disabled: true
       });
 
-      // Load user subscriptions
       ext.backgroundPage.sendMessage({
-        type: "subscriptions.get",
-        downloadable: true
+        type: "prefs.get",
+        key: "subscriptions_exceptionsurl_privacy"
       },
-      (subscriptions) =>
+      (urlPrivacy) =>
       {
-        for (let subscription of subscriptions)
-          onSubscriptionMessage("added", subscription);
+        acceptableAdsPrivacyUrl = urlPrivacy;
+        addSubscription({

[Thomas Greiner, 2017/08/09 18:14:50]

This looks odd. The subscription is not installed so why should we add it to the
UI?

[saroyanm, 2017/08/14 14:00:10]

Agree it's odd :) 
Will fix.

[saroyanm, 2017/08/16 14:17:37]

Done.

+          url: acceptableAdsPrivacyUrl,
+          disabled: true
+        });
+
+        // Load user subscriptions

[Thomas Greiner, 2017/08/09 18:14:49]

This code doesn't depend on the value of `urlPrivacy` so why do we need to wait
for it before we can load user subscriptions?

[saroyanm, 2017/08/14 14:00:09]

There are checks involved dependant on acceptableAdsPrivacyUrl URL, before
adding subscription it's required in this case to know the URL for privacy
friendly Acceptable Ads list.

[Thomas Greiner, 2017/08/15 17:10:29]

Which checks in particular are you referring to?

[saroyanm, 2017/08/16 14:17:37]

To checks in "onSubscriptionMessage", we do same for the "acceptableAdsUrl" URL.

So the idea is to first get the "Acceptable Ads" and "Privacy Friendly
Acceptable Ads" URLs and after load the subscription.

[Thomas Greiner, 2017/08/16 17:57:08]

Ok, makes sense.

+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.get",
+          downloadable: true
+        },
+        (subscriptions) =>
+        {
+          for (let subscription of subscriptions)
+            onSubscriptionMessage("added", subscription);
+        });
       });
     });
   }
@@ -1130,10 +1213,24 @@
         else
           addSubscription(subscription);
 
+        if (subscription.url == acceptableAdsUrl)
+          document.querySelector(
+            "[name='acceptable-ads'][value='tracking']").checked = true;

[Thomas Greiner, 2017/08/09 18:14:49]

I noticed that you're only changing the value of `<input name="acceptable-ads">`
when a subscription gets added but since the case exists where none of them is
enabled we should also react to when a subscription gets removed.

[Thomas Greiner, 2017/08/09 18:14:49]

Coding Style: "When an if statement, an else statement or a loop spans over more
than one line always enclose it with braces."

Same applies to the similar statement below.

[saroyanm, 2017/08/14 14:00:10]

Agree.

[saroyanm, 2017/08/16 14:17:37]

Done.

+        if (subscription.url == acceptableAdsPrivacyUrl)

[Thomas Greiner, 2017/08/09 18:14:48]

Detail: This condition will never be `true` if the one above is so let's make
this an `else if` instead to clarify that relation.

[saroyanm, 2017/08/14 14:00:10]

agree.

[Thomas Greiner, 2017/08/16 17:57:08]

Maybe I'm missing something but I can't find the change.

+        {
+          document.querySelector(
+            "[name='acceptable-ads'][value='no-tracking']").checked = true;
+          if (!navigator.doNotTrack)

[saroyanm, 2017/07/26 20:56:50]

I couldn't find way to listen for "do not track" change. So if user will change
the browser setting having options page opened, it will require the user to
refresh the page to hide the message.

[Thomas Greiner, 2017/08/09 18:14:49]

Users need to move away from the options page to change the setting so it should
be sufficient to check only after they return to the page (e.g. using Page
Visibility API).

[saroyanm, 2017/08/14 14:00:10]

This is a good idea, but I think would be better to handle that separately. If
you agree I'll create a separate issue for that, not to block current review
with that implementation.

[Thomas Greiner, 2017/08/15 17:10:30]

Ok, sounds good.

+            setDntNotification(true);
+        }
+
         collections.filterLists.addItem(subscription);
         break;
       case "removed":
-        if (subscription.url == acceptableAdsUrl || subscription.recommended)
+        if (subscription.url == acceptableAdsUrl ||
+            subscription.url == acceptableAdsPrivacyUrl ||
+            subscription.recommended &&
+            moreSubscriptions.indexOf(subscription.recommended) == -1)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
@@ -1273,10 +1370,6 @@
       let tooltip = document.createElement("div");
       tooltip.setAttribute("role", "tooltip");
 
-      let flip = anchor.getAttribute("data-tooltip-flip");
-      if (flip)
-        tooltip.className = "flip-" + flip;

[Thomas Greiner, 2017/08/09 18:14:48]

How are you planning to handle tooltip orientation in the future?

[saroyanm, 2017/08/14 14:00:09]

Not yet, sure, but I think it make sense to re-implement, we don't need current
solution anymore as beforehand we had two columns layout, while here only one in
the main content.

I will need to do bit of research before implementing the tooltips.

-
       let imageSource = anchor.getAttribute("data-tooltip-image");
       if (imageSource)
       {