issue 4264 - centralize action handling

new-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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 456 matching lines @@
       else
         location.href = link;
     });
   }
 
   function switchTab(id)
   {
     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.

+  function execAction(action, element)
   {
     switch (action)
     {
       case "add-domain-exception":
         addWhitelistedDomain();
         break;
       case "add-language-subscription":
         addEnableSubscription(findParentData(element, "access", false));
         break;
       case "add-predefined-subscription": {
@@ skipping 62 matching lines @@
           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);
-        }
+        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":
@@ skipping 16 matching lines @@
     if (context)
       context.classList.remove("show-context-menu");
 
     let actions = findParentData(e.target, "action", false);
     if (!actions)
       return;
 
     actions = actions.split(",");
     for (let action of actions)
     {
-      execAction(action, "Enter", e.target);
+      execAction(action, e.target);
     }
   }
 
   function getKey(e)
   {
     // e.keyCode has been deprecated so we attempt to use e.key
     if ("key" in e)
       return e.key;
     return getKey.keys[e.keyCode];
   }
@@ skipping 15 matching lines @@
       return;
 
     let container = findParentData(element, "action", true);
     if (!container || !container.hasAttribute("data-keys"))
       return;
 
     let keys = container.getAttribute("data-keys").split(" ");
     if (keys.indexOf(key) < 0)
       return;
 
+    if (element.getAttribute("role") == "tab")
+    {
+      if (key == "ArrowLeft" || key == "ArrowUp")
+        element = element.previousElementSibling || container.lastElementChild;
+      else if (key == "ArrowRight" || key == "ArrowDown")
+        element = element.nextElementSibling || container.firstElementChild;
+    }
+
     let actions = container.getAttribute("data-action").split(",");
     for (let action of actions)
     {
-      execAction(action, key, element);
+      execAction(action, element);
     }
   }
 
   function selectTabItem(tabId, container, focus)
   {
     // Show tab content
     document.body.setAttribute("data-tab", tabId);
 
     // Select tab
     let tabList = container.querySelector("[role='tablist']");
@@ skipping 616 matching lines @@
   });
   ext.backgroundPage.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }