issue 3741 - Add "remove" option to list items in new options page

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -23,7 +23,7 @@
   var filtersMap = Object.create(null);
   var collections = Object.create(null);
   var acceptableAdsUrl = null;
-  var maxLabelId = 0;
+  var maxItemId = 0;
   var getMessage = ext.i18n.getMessage;
   var filterErrors =
   {
@@ -101,12 +101,14 @@
       {
         var item = arguments[i];
         var listItem = document.createElement("li");
+        var ItemId = "item-" + (++maxItemId);

[Thomas Greiner, 2016/06/16 10:42:22]

Detail: Variable names should start with a lower-case letter.

[saroyanm, 2016/06/16 18:22:59]

Done.

         listItem.appendChild(document.importNode(template.content, true));
         listItem.setAttribute("data-access", item.url || item.text);
+        listItem.setAttribute("id", ItemId);
+        listItem.setAttribute("role", "section");
 
-        var labelId = "label-" + (++maxLabelId);
         var label = listItem.querySelector(".display");
-        label.setAttribute("id", labelId);
+        label.setAttribute("for", ItemId);
         if (item.recommended && label.hasAttribute("data-tooltip"))
         {
           var tooltipId = label.getAttribute("data-tooltip");
@@ -114,20 +116,9 @@
           label.setAttribute("data-tooltip", tooltipId);
         }
 
-        var control = listItem.querySelector(".control");
-        if (control)
-        {
-          control.setAttribute("aria-labelledby", labelId);
-          control.addEventListener("click", this.details[j].onClick, false);
-
-          var role = control.getAttribute("role");
-          if (role == "checkbox" && !label.hasAttribute("data-action"))
-          {
-            var controlId = "control-" + maxLabelId;
-            control.setAttribute("id", controlId);
-            label.setAttribute("for", controlId);
-          }
-        }
+        var controls = listItem.querySelectorAll(".control");
+        for (var k = 0; k < controls.length; k++)
+          controls[k].setAttribute("aria-labelledby", ItemId);
 
         this._setEmpty(table, null);
         if (table.hasChildNodes())
@@ -194,14 +185,14 @@
 
       var title = this._getItemTitle(item, i);
       element.querySelector(".display").textContent = title;
+      element.setAttribute("label", title);

[Thomas Greiner, 2016/06/16 10:42:22]

What's the "label" attribute supposed to do?

[saroyanm, 2016/06/16 18:22:59]

I was testing with screen reader and apparently in this case it helps to provide
information about the list item, we are using labeldBy for the checkbox, but
it's still not obvious  for the people what is the item is about, so using label
we can provide information about the item to the blind people.

[Thomas Greiner, 2016/06/17 14:07:50]

Ok, in that case please use the standardized "aria-label" attribute instead (see
https://www.w3.org/TR/wai-aria/states_and_properties#aria-label) because I
couldn't find any documentation for "label".

[saroyanm, 2016/06/21 13:29:56]

I had another look and seems like removing this attribute works fine as well,
for some reason now I can hear duplication if I keep this attribute, so I think
it's better to remove it for now. So your suspicions regarding this label was
correct, sorry for that. But I was sure that this was the reason I used the
attribute :/ 

[Thomas Greiner, 2016/06/22 10:26:13]

I could imagine that the duplication might be because when you focus the
control, it reads first what section it's in and then its "aria-labelledby"
attribute which again refers to the section. In that case it should be
sufficient to remove the "aria-labelledby" attribute.

[saroyanm, 2016/06/22 13:46:16]

Genius! Done.

       if (title)
         element.setAttribute("data-search", title.toLowerCase());
       var control = element.querySelector(".control[role='checkbox']");
       if (control)
       {
         control.setAttribute("aria-checked", item.disabled == false);
-        if (item.url == acceptableAdsUrl && this.details[i].onClick ==
-                                            toggleDisableSubscription)
+        if (item.url == acceptableAdsUrl && this == collections.filterLists)
           control.setAttribute("disabled", true);
       }
 
@@ -282,63 +273,17 @@
     return true;
   }
 
-  function toggleRemoveSubscription(e)
-  {
-    e.preventDefault();
-    var subscriptionUrl = findParentData(e.target, "access", false);
-    if (e.target.getAttribute("aria-checked") == "true")
-    {
-      ext.backgroundPage.sendMessage({
-        type: "subscriptions.remove",
-        url: subscriptionUrl
-      });
-    }
-    else
-      addEnableSubscription(subscriptionUrl);
-  }
-
-  function toggleDisableSubscription(e)
-  {
-    e.preventDefault();
-    var subscriptionUrl = findParentData(e.target, "access", false);
-    ext.backgroundPage.sendMessage(
-    {
-      type: "subscriptions.toggle",
-      keepInstalled: true,
-      url: subscriptionUrl
-    });
-  }
-
-  function onAddLanguageSubscriptionClick(e)
-  {
-    e.preventDefault();
-    var url = findParentData(this, "access", false);
-    addEnableSubscription(url);
-  }
-
-  function onRemoveFilterClick()
-  {
-    var filter = findParentData(this, "access", false);
-    ext.backgroundPage.sendMessage(
-    {
-      type: "filters.remove",
-      text: filter
-    });
-  }
-
   collections.popular = new Collection(
   [
     {
-      id: "recommend-list-table",
-      onClick: toggleRemoveSubscription
+      id: "recommend-list-table"
     }
   ]);
   collections.langs = new Collection(
   [
     {
       id: "blocking-languages-table",
-      emptyText: "options_dialog_language_added_empty",
-      onClick: toggleRemoveSubscription
+      emptyText: "options_dialog_language_added_empty"
     },
     {
       id: "blocking-languages-dialog-table",
@@ -349,30 +294,26 @@
   [
     {
       id: "all-lang-table",
-      emptyText: "options_dialog_language_other_empty",
-      onClick: onAddLanguageSubscriptionClick
+      emptyText: "options_dialog_language_other_empty"
     }
   ]);
   collections.acceptableAds = new Collection(
   [
     {
-      id: "acceptableads-table",
-      onClick: toggleRemoveSubscription
+      id: "acceptableads-table"
     }
   ]);
   collections.custom = new Collection(
   [
     {
-      id: "custom-list-table",
-      onClick: toggleRemoveSubscription
+      id: "custom-list-table"
     }
   ]);
   collections.whitelist = new Collection(
   [
     {
       id: "whitelisting-table",
-      emptyText: "options_whitelisted_empty",
-      onClick: onRemoveFilterClick
+      emptyText: "options_whitelisted_empty"
     }
   ]);
   collections.customFilters = new Collection(
@@ -386,7 +327,6 @@
   [
     {
       id: "all-filter-lists-table",
-      onClick: toggleDisableSubscription,
       useOriginalTitle: true
     }
   ]);
@@ -633,6 +573,36 @@
             url: findParentData(element, "access", false)
           });
           break;
+        case "toggle-remove-subscription":
+          var subscriptionUrl = findParentData(element, "access", false);
+          if (element.getAttribute("aria-checked") == "true")
+          {
+            ext.backgroundPage.sendMessage({
+              type: "subscriptions.remove",
+              url: subscriptionUrl
+            });
+          }
+          else
+            addEnableSubscription(subscriptionUrl);
+          break;
+        case "toggle-disable-subscription":
+          ext.backgroundPage.sendMessage(
+          {
+            type: "subscriptions.toggle",
+            keepInstalled: true,
+            url: findParentData(element, "access", false)
+          });
+          break;
+        case "add-language-subscription":
+          addEnableSubscription(findParentData(element, "access", false));
+          break;
+        case "remove-filter":
+          ext.backgroundPage.sendMessage(
+          {
+            type: "filters.remove",
+            text: findParentData(element, "access", false)
+          });
+          break;
       }
     }
   }