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
@@ skipping 635 matching lines @@
         },
         () =>
         {
           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: value == "privacy" ? "subscriptions.add" :
+          type: isAcceptableAds != isChecked ? "subscriptions.add" :
             "subscriptions.remove",
-          url: acceptableAdsPrivacyUrl
+          url: acceptableAdsUrl
         });
         chrome.runtime.sendMessage({
-          type: value == "ads" ? "subscriptions.add" : "subscriptions.remove",
-          url: acceptableAdsUrl
+          type: isAcceptableAds || isChecked ? "subscriptions.remove" :
+            "subscriptions.add",
+          url: acceptableAdsPrivacyUrl
         });
         break;
       case "switch-tab":
         switchTab(element.getAttribute("href").substr(1));
         break;
       case "toggle-disable-subscription":
         chrome.runtime.sendMessage({
           type: "subscriptions.toggle",
           keepInstalled: true,
           url: findParentData(element, "access", false)
@@ skipping 207 matching lines @@
     {
       E("whitelisting-add-button").disabled = !e.target.value;
     }, false);
 
     getDocLink("contribute", (link) =>
     {
       E("contribute").href = link;
     });
     getDocLink("acceptable_ads_criteria", (link) =>
     {
-      setLinks("enable-aa-description", 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);
     });
     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) =>
     {
@@ skipping 124 matching lines @@
   }
 
   function hideNotification()
   {
     E("notification").setAttribute("aria-hidden", true);
     E("notification-text").textContent = "";
   }
 
   function setAcceptableAds()
   {
-    let option = "none";
-    document.forms["acceptable-ads"].classList.remove("show-dnt-notification");
+    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!

     if (acceptableAdsUrl in subscriptionsMap)
     {
-      option = "ads";
+      acceptableAds.setAttribute("aria-checked", true);
+      acceptableAdsPrivacy.disabled = false;
     }
     else if (acceptableAdsPrivacyUrl in subscriptionsMap)
     {
-      option = "privacy";
+      acceptableAds.setAttribute("aria-checked", true);
+      acceptableAdsPrivacy.setAttribute("aria-checked", true);
+      acceptableAdsPrivacy.disabled = false;
 
-      if (!navigator.doNotTrack)
-        document.forms["acceptable-ads"].classList.add("show-dnt-notification");
+      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");
     }
-    document.forms["acceptable-ads"]["acceptable-ads"].value = option;
   }
 
   function isAcceptableAds(url)
   {
     return url == acceptableAdsUrl || url == acceptableAdsPrivacyUrl;
   }
 
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
@@ skipping 319 matching lines @@
   });
   chrome.runtime.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }