issue 4264 - centralize action handling

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -23,6 +23,7 @@
   var filtersMap = Object.create(null);
   var collections = Object.create(null);
   var acceptableAdsUrl = null;
+  var isCustomFiltersLoaded = false;
   var getMessage = ext.i18n.getMessage;
   var filterErrors =
   {
@@ -474,150 +475,164 @@
     location.hash = id;
   }
 
+  function execAction(action, key, element)
+  {
+    switch (action)
+    {
+      case "add-domain-exception":
+        addWhitelistedDomain();
+        break;
+      case "add-language-subscription":
+        addEnableSubscription(findParentData(element, "access", false));
+        break;
+      case "add-predefined-subscription":
+        var dialog = E("dialog-content-predefined");
+        var title = dialog.querySelector("h3").textContent;
+        var 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":
+        var url = E("blockingList-textbox").value;
+        addEnableSubscription(url);
+        closeDialog();
+        break;
+      case "open-context-menu":
+        var listItem = findParentData(element, "access", true);
+        if (listItem != context)
+          listItem.classList.add("show-context-menu");
+        break;
+      case "open-dialog":
+        var dialog = findParentData(element, "dialog", false);
+        openDialog(dialog);
+        break;
+      case "open-doclink":
+        var 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
+        },
+        function()
+        {
+          E("custom-filters").classList.remove("mode-edit");
+        });
+        break;
+      case "switch-tab":
+        if (key == "Enter")
+        {
+          var 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;
+          }
+
+          var 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":
+        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 "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)
   {
     var context = document.querySelector(".show-context-menu");
     if (context)
       context.classList.remove("show-context-menu");
 
-    var element = e.target;
-    while (true)
-    {
-      if (!element)
-        return;
+    var actions = findParentData(e.target, "action", false);
+    if (!actions)
+      return;
 
-      if (element.hasAttribute("data-action"))
-        break;
-
-      element = element.parentElement;
-    }
-
-    var element = findParentData(e.target, "action", true);
-    var actions = element.getAttribute("data-action").split(",");
+    actions = actions.split(",");
     for (var i = 0; i < actions.length; i++)
     {
-      switch (actions[i])
-      {
-        case "add-domain-exception":
-          addWhitelistedDomain();
-          break;
-        case "add-predefined-subscription":
-          var dialog = E("dialog-content-predefined");
-          var title = dialog.querySelector("h3").textContent;
-          var 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":
-          var url = E("blockingList-textbox").value;
-          addEnableSubscription(url);
-          closeDialog();
-          break;
-        case "open-dialog":
-          var dialog = findParentData(element, "dialog", false);
-          openDialog(dialog);
-          break;
-        case "open-doclink":
-          var doclink = findParentData(element, "doclink", false);
-          openDocLink(doclink);
-          break;
-        case "save-custom-filters":
-          sendMessageHandleErrors(
-          {
-            type: "filters.importRaw",
-            text: E("custom-filters-raw").value,
-            removeExisting: true
-          },
-          function()
-          {
-            E("custom-filters").classList.remove("mode-edit");
-          });
-          break;
-        case "switch-tab":
-          var 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":
-          var 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":
-          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;
-      }
+      execAction(actions[i], "Enter", e.target);
     }
   }
 
@@ -653,43 +668,18 @@
     if (keys.indexOf(key) < 0)
       return;
 
-    switch (container.getAttribute("data-action"))
+    var actions = container.getAttribute("data-action").split(",");
+    for (var i = 0; i < actions.length; i++)
     {
-      case "add-domain-exception":
-        addWhitelistedDomain();
-        break;
-      case "open-doclink":
-        var doclink = findParentData(element, "doclink", false);
-        openDocLink(doclink);
-        break;
-      case "switch-tab":
-        if (key == "Enter")
-        {
-          var 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;
-          }
-
-          var tabId = findParentData(element, "tab", false);
-          switchTab(tabId);
-        }
-        break;
+      execAction(actions[i], key, element);
     }
   }
 
   function selectTabItem(tabId, container, focus)
   {
+    if (tabId == "advanced-customFilters" && !isCustomFiltersLoaded)
+      switchTab("advanced-allFilterLists");

[Thomas Greiner, 2017/01/27 16:04:33]

This appears to be a loop that only resolves when `isCustomFiltersLoaded` is
`true`:

onHashChange -> selectTabItem -> switchTab -> (next item in event loop) ->
onHashChange -> ...

Therefore this could turn into an infinite loop in case of issues with loading
custom filters so it'd be great if we could stick to the current approach of
updating the UI by reacting to events.

But apart from that, is there a reason why this check even exists? Just because
there might not be any custom filters to look at, doesn't mean that the tab
doesn't provide any value.

[saroyanm, 2017/01/27 16:13:44]

It's not a loop, switch from one tab to another if the one not ready to be load.
It shouldn't turn into infinite loop if there is error with page.
We might reconsider the way we are showing the "Edit" tab while we currently
using the implementation in current options page, which is tricky and doesn't
include update of textarea when new filter is added. I think it's safer this way
to switch tab if the filters are not loaded and we can fix the general issue
with the texarea not being populated dynamical in separate issue.

[Thomas Greiner, 2017/01/27 16:39:27]

You're right. I must've misread that as "advanced-allFilterlists" when I wrote
the first part of the comment.
It might be safer but it's a terrible user experience because:

a) Clicking on the tab doesn't switch to it but you stay on the
"advanced-allFilterlists" tab.

b) Deep-linking to the "advanced-customFilters" tab will show the
"advanced-allFilterlists" tab. This will cause confusion used by instructions
that may refer to UI elements within that tab.

Therefore I'd suggest to stick to the implementation that's in the current
options page and not update the text area, only the list of filters.

[saroyanm, 2017/01/27 16:56:41]

I'll strongly disagree sticking to the current option page implementation while
using deeplinking to the edit mode, because if we will allow user to switch to
the edit mode while the filters are not loaded completely, click on save button
might lead to the loose of the existing custom filter lists.
So I can see two possibilities:
1) Do not allow deeplinking to the edit mode.
2) Only switch to edit mode if the custom filter lists are populated.

 

[Thomas Greiner, 2017/01/30 11:57:53]

"advanced-customFilters" is not a deep-link to the edit mode. I assume that's
where the confusion comes from. The text area should not be updated whereas the
list of custom filters should, right?

In that case maybe it'd be best to prevent the user from accessing edit mode
until custom filters have finished loading.

[saroyanm, 2017/02/01 12:28:10]

Probably, I was refering to: /new-options.html#advanced-customFilters 
Maybe I'm wrong
Does that mean that you are fine with current approach ?

[Thomas Greiner, 2017/02/02 12:24:50]

The "advanced-customFilters" can be accessed either in "view" or "edit" mode
(see `editCustomFilters()`). While you should still be able to view the custom
filters while they're loading, you shouldn't be able to edit them.

Your current approach, however, does not allow the user to even view them.

[saroyanm, 2017/02/02 12:34:01]

Clear, agree we should definately show the custom filters view tab instead of
full filterlists tab, will update with the next patch

+
     // Show tab content
     document.body.setAttribute("data-tab", tabId);
 
@@ -716,6 +706,9 @@
     if (tab && focus)
       tab.focus();
 
+    if (tabId == "advanced-customFilters")

[Thomas Greiner, 2017/01/27 16:04:33]

What about making this more generic to declare the behavior in the HTML: It's a
dependency that needs to be resolved before the tab should be shown.

For instance, I could imagine "data-dependency='edit-custom-filters'" and handle
it like this:

if (tab.hasAttribute("data-dependency"))
  execAction(tab.getAttribute("data-dependency");

[saroyanm, 2017/01/27 16:13:44]

I'll investigate that and provide in new patch, I agree that dependency should
make more sense here.

[saroyanm, 2017/02/08 12:14:00]

This is not relevant anymore while we are showing view of custom filters in new
patch and also we can use data-action="switch-tab,edit-custom-filters" currently
in HTML.

[Thomas Greiner, 2017/03/02 15:10:13]

You're right. Using "data-action" is more appropriate in this case but you
removed it in your first patch.

+      editCustomFilters();
+
     return tabContent;
   }
 
@@ -906,6 +899,8 @@
         {
           for (var i = 0; i < filters.length; i++)
             updateFilter(filters[i]);
+
+          isCustomFiltersLoaded = true;
         });
       }
     });
@@ -955,6 +950,7 @@
 
   function editCustomFilters()
   {
+    E("custom-filters").classList.add("mode-edit");
     var customFilterItems = collections.customFilters.items;
     var filterTexts = [];
     for (var i = 0; i < customFilterItems.length; i++)