issue 4264 - centralize action handling

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -25,6 +25,7 @@
   let filtersMap = Object.create(null);
   let collections = Object.create(null);
   let acceptableAdsUrl = null;
+  let isCustomFiltersLoaded = false;
   let {getMessage} = ext.i18n;
   let filterErrors = new Map([
     ["synchronize_invalid_url",
@@ -473,143 +474,164 @@
     location.hash = id;
   }
 
+  function execAction(action, key, element)

[Thomas Greiner, 2017/05/05 14:33:05]

`key` is only used for keyboard-initiated actions so let's keep them in
`onKeyUp()`.

Also note that it's only used for "switch-tabs" and there `key` is merely
determining the value of `element` so moving it out of here shouldn't be an
issue.

[saroyanm, 2017/05/16 10:36:42]

Done.

+  {
+    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":
+        E("custom-filters").classList.remove("mode-edit");
+        break;
+      case "cancel-domain-exception":
+        E("whitelisting-textbox").value = "";
+        document.querySelector("#whitelisting .controls").classList
+          .remove("mode-edit");
+        break;
+      case "close-dialog":
+        closeDialog();
+        break;
+      case "edit-custom-filters":
+        editCustomFilters();
+        break;
+      case "edit-domain-exception":
+        document.querySelector("#whitelisting .controls").classList
+          .add("mode-edit");
+        E("whitelisting-textbox").focus();
+        break;
+      case "import-subscription": {
+        let url = E("blockingList-textbox").value;
+        addEnableSubscription(url);
+        closeDialog();
+        break;
+      }
+      case "open-context-menu": {
+        let listItem = findParentData(element, "access", true);
+        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 "open-doclink": {
+        let doclink = findParentData(element, "doclink", false);
+        openDocLink(doclink);
+        break;
+      }
+      case "remove-filter":
+        ext.backgroundPage.sendMessage({
+          type: "filters.remove",
+          text: findParentData(element, "access", false)
+        });
+        break;
+      case "remove-subscription":
+        ext.backgroundPage.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
+        },
+        () =>
+        {
+          E("custom-filters").classList.remove("mode-edit");
+        });
+        break;
+      case "switch-tab":
+        if (key == "Enter")
+        {
+          let tabId = findParentData(element, "tab", false);
+          switchTab(tabId);
+        }
+        else if (element.hasAttribute("aria-selected"))
+        {
+          let container = findParentData(element, "action", true);
+          if (key == "ArrowLeft" || key == "ArrowUp")
+          {
+            element = element.previousElementSibling ||
+                container.lastElementChild;
+          }
+          else if (key == "ArrowRight" || key == "ArrowDown")
+          {
+            element = element.nextElementSibling ||
+                container.firstElementChild;
+          }
+          let tabId = findParentData(element, "tab", false);
+          switchTab(tabId);
+        }
+        break;
+      case "toggle-disable-subscription":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.toggle",
+          keepInstalled: true,
+          url: findParentData(element, "access", false)
+        });
+        break;
+      case "toggle-pref":
+        ext.backgroundPage.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")

[Thomas Greiner, 2017/05/05 14:33:05]

Did you verify that this still works? The reason why I'm asking is thta this is
the only place where we're referring to `element` directly rather than just for
using it for `findParentData()`. Because before `element` had the value
`findParentData(e.target, "action", true)` whereas now it has the value
`e.target` so it might be a different element than before.

[saroyanm, 2017/05/16 10:36:42]

It does work, until we introduce child element to the "Checkbox button".

+        {
+          ext.backgroundPage.sendMessage({
+            type: "subscriptions.remove",
+            url: subscriptionUrl
+          });
+        }
+        else
+          addEnableSubscription(subscriptionUrl);
+        break;
+      case "update-all-subscriptions":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.update"
+        });
+        break;
+      case "update-subscription":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.update",
+          url: findParentData(element, "access", false)
+        });
+        break;
+    }
+  }
+
   function onClick(e)
   {
     let context = document.querySelector(".show-context-menu");
     if (context)
       context.classList.remove("show-context-menu");
 
-    let element = findParentData(e.target, "action", true);
-    if (!element)
+    let actions = findParentData(e.target, "action", false);
+    if (!actions)
       return;
 
-    let actions = element.getAttribute("data-action").split(",");
+    actions = actions.split(",");
     for (let action of actions)
     {
-      switch (action)
-      {
-        case "add-domain-exception":
-          addWhitelistedDomain();
-          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":
-          E("custom-filters").classList.remove("mode-edit");
-          break;
-        case "cancel-domain-exception":
-          E("whitelisting-textbox").value = "";
-          document.querySelector("#whitelisting .controls").classList
-            .remove("mode-edit");
-          break;
-        case "close-dialog":
-          closeDialog();
-          break;
-        case "edit-custom-filters":
-          E("custom-filters").classList.add("mode-edit");
-          editCustomFilters();
-          break;
-        case "edit-domain-exception":
-          document.querySelector("#whitelisting .controls").classList
-            .add("mode-edit");
-          E("whitelisting-textbox").focus();
-          break;
-        case "import-subscription": {
-          let url = E("blockingList-textbox").value;
-          addEnableSubscription(url);
-          closeDialog();
-          break;
-        }
-        case "open-dialog": {
-          let dialog = findParentData(element, "dialog", false);
-          openDialog(dialog);
-          break;
-        }
-        case "open-doclink": {
-          let doclink = findParentData(element, "doclink", false);
-          openDocLink(doclink);
-          break;
-        }
-        case "save-custom-filters":
-          sendMessageHandleErrors({
-            type: "filters.importRaw",
-            text: E("custom-filters-raw").value,
-            removeExisting: true
-          },
-          () =>
-          {
-            E("custom-filters").classList.remove("mode-edit");
-          });
-          break;
-        case "switch-tab": {
-          let tabId = findParentData(e.target, "tab", false);
-          switchTab(tabId);
-          break;
-        }
-        case "toggle-pref":
-          ext.backgroundPage.sendMessage({
-            type: "prefs.toggle",
-            key: findParentData(element, "pref", false)
-          });
-          break;
-        case "update-all-subscriptions":
-          ext.backgroundPage.sendMessage({
-            type: "subscriptions.update"
-          });
-          break;
-        case "open-context-menu": {
-          let listItem = findParentData(element, "access", true);
-          if (listItem != context)
-            listItem.classList.add("show-context-menu");
-          break;
-        }
-        case "update-subscription":
-          ext.backgroundPage.sendMessage({
-            type: "subscriptions.update",
-            url: findParentData(element, "access", false)
-          });
-          break;
-        case "remove-subscription":
-          ext.backgroundPage.sendMessage({
-            type: "subscriptions.remove",
-            url: findParentData(element, "access", false)
-          });
-          break;
-        case "toggle-remove-subscription": {
-          let 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;
-      }
+      execAction(action, "Enter", e.target);
     }
   }
 
@@ -645,38 +667,10 @@
     if (keys.indexOf(key) < 0)
       return;
 
-    switch (container.getAttribute("data-action"))
+    let actions = container.getAttribute("data-action").split(",");
+    for (let action of actions)
     {
-      case "add-domain-exception":
-        addWhitelistedDomain();
-        break;
-      case "open-doclink":
-        let doclink = findParentData(element, "doclink", false);
-        openDocLink(doclink);
-        break;
-      case "switch-tab":
-        if (key == "Enter")
-        {
-          let tabId = findParentData(element, "tab", false);
-          switchTab(tabId);
-        }
-        else if (element.hasAttribute("aria-selected"))
-        {
-          if (key == "ArrowLeft" || key == "ArrowUp")
-          {
-            element = element.previousElementSibling ||
-                container.lastElementChild;
-          }
-          else if (key == "ArrowRight" || key == "ArrowDown")
-          {
-            element = element.nextElementSibling ||
-                container.firstElementChild;
-          }
-
-          let tabId = findParentData(element, "tab", false);
-          switchTab(tabId);
-        }
-        break;
+      execAction(action, key, element);
     }
   }
 
@@ -945,6 +939,8 @@
         {
           for (let filter of filters)
             updateFilter(filter);
+
+          isCustomFiltersLoaded = true;
         });
       }
     });
@@ -992,6 +988,13 @@
 
   function editCustomFilters()
   {
+    if (!isCustomFiltersLoaded)
+    {
+      console.error("Custom filters are not loaded");
+      return;
+    }
+
+    E("custom-filters").classList.add("mode-edit");
     let filterTexts = [];
     for (let customFilterItem of collections.customFilters.items)
       filterTexts.push(customFilterItem.text);