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 541 matching lines @@
     });
   }
 
   function switchTab(id)
   {
     location.hash = id;
   }
 
   function execAction(action, element)
   {
+    if (element.getAttribute("aria-disabled") == "true")

[Thomas Greiner, 2017/10/20 15:35:03]

Detail: No need to check for "true" or "false" when dealing with binary
attributes. They either exist or they don't. So we should be able to use
`Element.hasAttribute()` instead.

Same applies to "aria-checked".

[saroyanm, 2017/10/20 16:42:06]

In our code in some places we do set attribute like:
Element.setAttribute("aria-checked", value);

I think we should refactor whole codebase to use hasAttribute for binary
attributes. That will require to create setBinaryAttribute function in general.

If you agree with that I can create separate ticket, I also can fix that for
introduced attributes in current review, but in that case we will have
inconsistent codebase, if that worth it I'm ready to do that, not sure what is
better in this case.

[Thomas Greiner, 2017/10/20 17:59:57]

I'm fine with working on this in a separate ticket.

[saroyanm, 2017/10/20 19:06:46]

Ticket: https://issues.adblockplus.org/ticket/5900

+      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");
@@ skipping 74 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 isRegularAcceptableAds = value == "ads";
+
         browser.runtime.sendMessage({
-          type: value == "privacy" ? "subscriptions.add" :
+          type: isRegularAcceptableAds != isChecked ? "subscriptions.add" :
             "subscriptions.remove",
-          url: acceptableAdsPrivacyUrl
+          url: acceptableAdsUrl
         });
         browser.runtime.sendMessage({
-          type: value == "ads" ? "subscriptions.add" : "subscriptions.remove",
-          url: acceptableAdsUrl
+          type: isRegularAcceptableAds || isChecked ? "subscriptions.remove" :
+            "subscriptions.add",
+          url: acceptableAdsPrivacyUrl
         });

[Thomas Greiner, 2017/10/20 15:35:03]

This is very hard to read so for the sake of maintainability I'd strongly
recommend simplifying this a bit to avoid mistakes because it's crucial that
this work correctly.

[saroyanm, 2017/10/20 16:42:06]

I agree with you, I made it hopefully simpler now.

[Thomas Greiner, 2017/10/20 17:59:57]

It's better. As mentioned in person, I'd avoid duplicating the
`browser.runtime.sendMessage()` calls though and rename `isCheck` to something
along the lines of `shouldCheck` to clarify its purpose.

[saroyanm, 2017/10/20 19:06:46]

Done.

         break;
       case "switch-tab":
         switchTab(element.getAttribute("href").substr(1));
         break;
       case "toggle-disable-subscription":
         browser.runtime.sendMessage({
           type: "subscriptions.toggle",
           keepInstalled: true,
           url: findParentData(element, "access", false)
         });
@@ skipping 206 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) =>
+    {
+      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) =>
     {
@@ 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.setAttribute("tabindex", 0);
     if (acceptableAdsUrl in subscriptionsMap)
     {
-      option = "ads";
+      acceptableAds.setAttribute("aria-checked", true);
+      acceptableAdsPrivacy.setAttribute("aria-disabled", false);
     }
     else if (acceptableAdsPrivacyUrl in subscriptionsMap)
     {
-      option = "privacy";
+      acceptableAds.setAttribute("aria-checked", true);
+      acceptableAdsPrivacy.setAttribute("aria-checked", true);
+      acceptableAdsPrivacy.setAttribute("aria-disabled", false);
 
-      if (!navigator.doNotTrack)
-        document.forms["acceptable-ads"].classList.add("show-dnt-notification");
+      if (navigator.doNotTrack != 1)
+        acceptableAdsForm.classList.add("show-dnt-notification");
     }
-    document.forms["acceptable-ads"]["acceptable-ads"].value = option;
+    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);
@@ skipping 319 matching lines @@
   });
   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);
 }