Issue 5632 - Use checkboxes for toggling acceptable ads

desktop-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* globals checkShareResource, getDocLink, i18nFormatDateTime, openSharePopup,
            setLinks, E */
 
 "use strict";
 
 {
   let subscriptionsMap = Object.create(null);
   let filtersMap = Object.create(null);
   let collections = Object.create(null);
   let acceptableAdsUrl = null;
   let acceptableAdsPrivacyUrl = null;
   let isCustomFiltersLoaded = false;
-  let {getMessage} = chrome.i18n;
+  let {getMessage} = browser.i18n;
   let customFilters = [];
   let filterErrors = new Map([
     ["synchronize_invalid_url",
      "options_filterList_lastDownload_invalidURL"],
     ["synchronize_connection_error",
      "options_filterList_lastDownload_connectionError"],
     ["synchronize_invalid_data",
      "options_filterList_lastDownload_invalidData"],
     ["synchronize_checksum_mismatch",
      "options_filterList_lastDownload_checksumMismatch"]
@@ skipping 495 matching lines @@
         return element.getAttribute("data-" + dataName);
       }
 
       element = element.parentElement;
     }
     return null;
   }
 
   function sendMessageHandleErrors(message, onSuccess)
   {
-    chrome.runtime.sendMessage(message, (errors) =>
+    browser.runtime.sendMessage(message, (errors) =>
     {
       if (errors.length > 0)
         alert(errors.join("\n"));
       else if (onSuccess)
         onSuccess();
     });
   }
 
   function switchTab(id)
   {
     location.hash = id;
   }
 
   function execAction(action, element)
   {
+    if (element.getAttribute("aria-disabled") == "true")
+      return;
+
     switch (action)
     {
       case "add-domain-exception":
         addWhitelistedDomain();
         break;
       case "add-language-subscription":
         addEnableSubscription(findParentData(element, "access", false));
         break;
       case "add-predefined-subscription": {
         let dialog = E("dialog-content-predefined");
         let title = dialog.querySelector("h3").textContent;
         let url = dialog.querySelector(".url").textContent;
         addEnableSubscription(url, title);
         closeDialog();
         break;
       }
       case "cancel-custom-filters":
         setCustomFiltersView("read");
         break;
       case "change-language-subscription":
         for (let key in subscriptionsMap)
         {
           let subscription = subscriptionsMap[key];
           let subscriptionType = subscription.recommended;
           if (subscriptionType == "ads" && subscription.disabled == false)
           {
-            chrome.runtime.sendMessage({
+            browser.runtime.sendMessage({
               type: "subscriptions.remove",
               url: subscription.url
             });
-            chrome.runtime.sendMessage({
+            browser.runtime.sendMessage({
               type: "subscriptions.add",
               url: findParentData(element, "access", false)
             });
             break;
           }
         }
         break;
       case "close-dialog":
         closeDialog();
         break;
@@ skipping 17 matching lines @@
         if (listItem && !listItem.classList.contains("show-context-menu"))
           listItem.classList.add("show-context-menu");
         break;
       }
       case "open-dialog": {
         let dialog = findParentData(element, "dialog", false);
         openDialog(dialog);
         break;
       }
       case "remove-filter":
-        chrome.runtime.sendMessage({
+        browser.runtime.sendMessage({
           type: "filters.remove",
           text: findParentData(element, "access", false)
         });
         break;
       case "remove-subscription":
-        chrome.runtime.sendMessage({
+        browser.runtime.sendMessage({
           type: "subscriptions.remove",
           url: findParentData(element, "access", false)
         });
         break;
       case "save-custom-filters":
         sendMessageHandleErrors({
           type: "filters.importRaw",
           text: E("custom-filters-raw").value,
           removeExisting: true
         },
         () =>
         {
           setCustomFiltersView("read");
         });
         break;
       case "show-more-filters-section":
         E("more-filters").setAttribute("aria-hidden", false);
         break;
       case "switch-acceptable-ads":
-        let isChecked = element.getAttribute("aria-checked") == "true";
         let value = element.value || element.dataset.value;
-        let isAcceptableAds = value == "ads";

[ire, 2017/10/17 08:16:34]

I'm getting the eslint error: isAcceptableAds is already declared in the upper
scope (no-shadow)

I think this is to do with the isAcceptableAds function that's already defined. 

Since you're checking if it's the #acceptable-ads-allow element, perhaps you can
name this something like `isAcceptableAdsAllow`

[saroyanm, 2017/10/17 19:23:07]

Thanks, I isolated scope with the braces. Let me know if you think renaming
still make sense.

[ire, 2017/10/18 17:48:45]

I'm still getting the same error, I don't think the braces made a difference

[saroyanm, 2017/10/19 20:41:55]

Right, my bad, I thought it was a variable in the same scope as switch, I've
missed that.
I used "isRegularAcceptableAds", if you have a strong preference regarding the
name I still can change it.
I'd like to avoid using "allow" not to sound ambiguous in this context.

But I also not really sure about this name, I would like to differentiate here
between Acceptable Ads and privacy-friendly Acceptable Ads.

[ire, 2017/10/20 08:14:41]

I don't have a strong preference about the name. I can't think of a better name
than what you have here already, so I think it's good enough.

-
-        chrome.runtime.sendMessage({
-          type: isAcceptableAds != isChecked ? "subscriptions.add" :
+        // User check the checkbox
+        let shouldCheck = element.getAttribute("aria-checked") != "true";
+        let installAcceptableAds = false;
+        let installAcceptableAdsPrivacy = false;
+        // Acceptable Ads checkbox clicked
+        if (value == "ads")
+        {
+          installAcceptableAds = shouldCheck;
+        }
+        // Privacy Friendly Acceptable Ads checkbox clicked
+        else
+        {
+          installAcceptableAdsPrivacy = shouldCheck;
+          installAcceptableAds = !shouldCheck;
+        }
+
+        browser.runtime.sendMessage({
+          type: installAcceptableAds ? "subscriptions.add" :
             "subscriptions.remove",
           url: acceptableAdsUrl
         });
-        chrome.runtime.sendMessage({
-          type: isAcceptableAds || isChecked ? "subscriptions.remove" :
-            "subscriptions.add",
+        browser.runtime.sendMessage({
+          type: installAcceptableAdsPrivacy ? "subscriptions.add" :
+            "subscriptions.remove",
           url: acceptableAdsPrivacyUrl
         });
         break;
       case "switch-tab":
         switchTab(element.getAttribute("href").substr(1));
         break;
       case "toggle-disable-subscription":
-        chrome.runtime.sendMessage({
+        browser.runtime.sendMessage({
           type: "subscriptions.toggle",
           keepInstalled: true,
           url: findParentData(element, "access", false)
         });
         break;
       case "toggle-pref":
-        chrome.runtime.sendMessage({
+        browser.runtime.sendMessage({
           type: "prefs.toggle",
           key: findParentData(element, "pref", false)
         });
         break;
       case "toggle-remove-subscription":
         let subscriptionUrl = findParentData(element, "access", false);
         if (element.getAttribute("aria-checked") == "true")
         {
-          chrome.runtime.sendMessage({
+          browser.runtime.sendMessage({
             type: "subscriptions.remove",
             url: subscriptionUrl
           });
         }
         else
           addEnableSubscription(subscriptionUrl);
         break;
       case "update-all-subscriptions":
-        chrome.runtime.sendMessage({
+        browser.runtime.sendMessage({
           type: "subscriptions.update"
         });
         break;
       case "update-subscription":
-        chrome.runtime.sendMessage({
+        browser.runtime.sendMessage({
           type: "subscriptions.update",
           url: findParentData(element, "access", false)
         });
         break;
       case "validate-import-subscription":
         let form = findParentData(element, "validation", true);
         if (!form)
           return;
 
         if (form.checkValidity())
@@ skipping 141 matching lines @@
       if (!tabContent)
         break;
     }
   }
 
   function onDOMLoaded()
   {
     populateLists();
 
     // Initialize navigation sidebar
-    chrome.runtime.sendMessage({
+    browser.runtime.sendMessage({
       type: "app.get",
       what: "addonVersion"
     },
     (addonVersion) =>
     {
       E("abp-version").textContent = getMessage("options_dialog_about_version",
         [addonVersion]);
     });
 
     updateTooltips();
@@ skipping 10 matching lines @@
     }, false);
 
     getDocLink("contribute", (link) =>
     {
       E("contribute").href = link;
     });
     getDocLink("acceptable_ads_criteria", (link) =>
     {
       setLinks("enable-acceptable-ads-description", link);
     });
     getDocLink("privacy_friendly_ads", (link) =>

[saroyanm, 2017/10/16 17:45:51]

We still missing this redirection link, I'll reach Winsley regarding this and
will be sure not to push this before we will have one.

[ire, 2017/10/17 08:16:34]

Acknowledged.

     {
-      setLinks("enable-acceptable-ads-privacy-description", link);
+      E("enable-acceptable-ads-privacy-description").href = link;
     });
     getDocLink("adblock_plus_{browser}_dnt", url =>
     {
       setLinks("dnt", url);
     });
 
     // Advanced tab
     let customize = document.querySelectorAll("#customize li[data-pref]");
     customize = Array.prototype.map.call(customize, (checkbox) =>
     {
       return checkbox.getAttribute("data-pref");
     });
     for (let key of customize)
     {
       getPref(key, (value) =>
       {
         onPrefMessage(key, value, true);
       });
     }
-    chrome.runtime.sendMessage({
+    browser.runtime.sendMessage({
       type: "app.get",
       what: "features"
     },
     (features) =>
     {
       hidePref("show_devtools_panel", !features.devToolsPanel);
     });
 
     getDocLink("filterdoc", (link) =>
     {
@@ skipping 110 matching lines @@
   }
 
   function setAcceptableAds()
   {
     let acceptableAdsForm = E("acceptable-ads");
     let acceptableAds = E("acceptable-ads-allow");
     let acceptableAdsPrivacy = E("acceptable-ads-privacy-allow");
     acceptableAdsForm.classList.remove("show-dnt-notification");
     acceptableAds.setAttribute("aria-checked", false);
     acceptableAdsPrivacy.setAttribute("aria-checked", false);
-    acceptableAdsPrivacy.disabled = true;

[ire, 2017/10/17 08:16:34]

When you toggle the acceptableAdsPrivacy checkbox on and off via keyboard, focus
is moved away from it. I narrowed it down to this line. I guess because it's
being set as disabled temporarily, focus is removed for it.

[saroyanm, 2017/10/17 19:23:07]

yes this is trickier than it appears. I'll try to address this in a separate
patch.

I moved this to the "else" case, but this is not a solution, the problem here is
that we are disabling the toggle when both Acceptable Ads and Acceptable Ads
that Privacy Friendly list are not installed, but while the addition and removal
is async and we can't be completely sure what was the user intention when one or
another list is removed or added it makes this bit tricky when browsers remove
focus from focused elements.
So the scenario is that when we are clicking on the toggle we do remove one list
and add the another one and if one is being removed before the other one is
added we have the toggle set to disabled.

[saroyanm, 2017/10/17 19:54:29]

Done.

[ire, 2017/10/18 17:48:45]

Ah I see, very tricky!

+    acceptableAdsPrivacy.setAttribute("tabindex", 0);
     if (acceptableAdsUrl in subscriptionsMap)
     {
       acceptableAds.setAttribute("aria-checked", true);
-      acceptableAdsPrivacy.disabled = false;
+      acceptableAdsPrivacy.setAttribute("aria-disabled", false);
     }
     else if (acceptableAdsPrivacyUrl in subscriptionsMap)
     {
       acceptableAds.setAttribute("aria-checked", true);
       acceptableAdsPrivacy.setAttribute("aria-checked", true);
-      acceptableAdsPrivacy.disabled = false;
+      acceptableAdsPrivacy.setAttribute("aria-disabled", false);
 
       if (navigator.doNotTrack != 1)

[saroyanm, 2017/10/16 17:45:52]

See -> https://issues.adblockplus.org/ticket/5866 

I think we can handle this here.

[ire, 2017/10/17 08:16:34]

Seen. I agree with you.

[ire, 2017/10/17 08:16:34]

I don't know if support for Edge/Safari is relevant here, but they use
`window.doNotTrack` instead (see
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack#Browser...)

[saroyanm, 2017/10/17 19:23:07]

We don't consider Safari, but it make sense for the Edge, we have a lot of to
adapt, we will cover that here -> https://issues.adblockplus.org/ticket/5813

[ire, 2017/10/18 17:48:45]

Acknowledged.

         acceptableAdsForm.classList.add("show-dnt-notification");
+    }
+    else
+    {
+      // Using aria-disabled in order to keep the focus
+      acceptableAdsPrivacy.setAttribute("aria-disabled", true);
+      acceptableAdsPrivacy.setAttribute("tabindex", -1);
     }
   }
 
   function isAcceptableAds(url)
   {
     return url == acceptableAdsUrl || url == acceptableAdsPrivacyUrl;
   }
 
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
     filtersMap = Object.create(null);
 
     // Empty collections and lists
     for (let property in collections)
       collections[property].clearAll();
 
     setCustomFiltersView("empty");
-    chrome.runtime.sendMessage({
+    browser.runtime.sendMessage({
       type: "subscriptions.get",
       special: true
     },
     (subscriptions) =>
     {
       // Load filters
       for (let subscription of subscriptions)
       {
-        chrome.runtime.sendMessage({
+        browser.runtime.sendMessage({
           type: "filters.get",
           subscriptionUrl: subscription.url
         },
         (filters) =>
         {
           loadCustomFilters(filters);
         });
       }
     });
     loadRecommendations();
-    chrome.runtime.sendMessage({
+    browser.runtime.sendMessage({
       type: "prefs.get",
       key: "subscriptions_exceptionsurl"
     },
     (url) =>
     {
       acceptableAdsUrl = url;
 
-      chrome.runtime.sendMessage({
+      browser.runtime.sendMessage({
         type: "prefs.get",
         key: "subscriptions_exceptionsurl_privacy"
       },
       (urlPrivacy) =>
       {
         acceptableAdsPrivacyUrl = urlPrivacy;
 
         // Load user subscriptions
-        chrome.runtime.sendMessage({
+        browser.runtime.sendMessage({
           type: "subscriptions.get",
           downloadable: true
         },
         (subscriptions) =>
         {
           for (let subscription of subscriptions)
             onSubscriptionMessage("added", subscription);
 
           setAcceptableAds();
         });
@@ skipping 37 matching lines @@
 
     let message = {
       type: messageType,
       url
     };
     if (title)
       message.title = title;
     if (homepage)
       message.homepage = homepage;
 
-    chrome.runtime.sendMessage(message);
+    browser.runtime.sendMessage(message);
   }
 
   function onFilterMessage(action, filter)
   {
     switch (action)
     {
       case "added":
         filter[timestampUI] = Date.now();
         updateFilter(filter);
         break;
@@ skipping 90 matching lines @@
   {
     let checkPref = getPref.checks[key] || getPref.checkNone;
     checkPref((isActive) =>
     {
       if (!isActive)
       {
         hidePref(key, !isActive);
         return;
       }
 
-      chrome.runtime.sendMessage({
+      browser.runtime.sendMessage({
         type: "prefs.get",
         key
       }, callback);
     });
   }
 
   getPref.checkNone = function(callback)
   {
     callback(true);
   };
@@ skipping 73 matching lines @@
         break;
       case "prefs.respond":
         onPrefMessage(message.action, message.args[0], false);
         break;
       case "subscriptions.respond":
         onSubscriptionMessage(message.action, message.args[0]);
         break;
     }
   });
 
-  chrome.runtime.sendMessage({
+  browser.runtime.sendMessage({
     type: "app.listen",
     filter: ["addSubscription", "focusSection"]
   });
-  chrome.runtime.sendMessage({
+  browser.runtime.sendMessage({
     type: "filters.listen",
     filter: ["added", "loaded", "removed"]
   });
-  chrome.runtime.sendMessage({
+  browser.runtime.sendMessage({
     type: "prefs.listen",
     filter: ["notifications_ignoredcategories", "notifications_showui",
              "show_devtools_panel", "shouldShowBlockElementMenu"]
   });
-  chrome.runtime.sendMessage({
+  browser.runtime.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }