Issue 5549 - Implement missing error handlings for custom filter lists

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 529 matching lines @@
         if (returnElement)
           return element;
         return element.getAttribute("data-" + dataName);
       }
 
       element = element.parentElement;
     }
     return null;
   }
 
-  function sendMessageHandleErrors(message, onSuccess)
+  function sendMessageHandleErrors(message, onMessage)

[saroyanm, 2018/01/23 15:26:06]

Suggestion/detail: seems like onMessage is used for the callback, what about
renaming it so "callback" instead of "onMessage" ?

   {
     browser.runtime.sendMessage(message, (errors) =>
     {
-      if (errors.length > 0)
-        alert(errors.join("\n"));
-      else if (onSuccess)
-        onSuccess();
+      if (onMessage)
+      {
+        if (errors.length > 0)
+          onMessage(errors);
+        else
+          onMessage();
+      }
     });
   }
 
   function switchTab(id)
   {
     location.hash = id;
   }
 
   function execAction(action, element)
   {
@@ skipping 73 matching lines @@
           text: findParentData(element, "access", false)
         });
         break;
       case "remove-subscription":
         browser.runtime.sendMessage({
           type: "subscriptions.remove",
           url: findParentData(element, "access", false)
         });
         break;
       case "save-custom-filters":
+        const filters = E("custom-filters-raw");
         sendMessageHandleErrors({
           type: "filters.importRaw",
-          text: E("custom-filters-raw").value,
+          text: filters.value,
           removeExisting: true
         },
-        () =>
+        (errors) =>
         {
-          setCustomFiltersView("read");
+          if (errors)
+          {
+            filters.classList.add("warning");
+            E("custom-filters-edit-error").classList.add("warning");

[saroyanm, 2018/01/23 15:26:06]

Suggestion: I think we can use "custom-filters" in order to specify the
"warning" message, rather than on each element separately.

+            E("link-filters-on-edit-error").classList.add("visible");

[saroyanm, 2018/01/23 15:26:06]

Note: If we agree on making this message persistent, we probably won't need
this.

+            const lines = filters.value.split(/\n+/);
+            const info = errors.map(error => lines[error.lineno - 1]);

[saroyanm, 2018/01/23 15:26:05]

Note: As soon we introduce a new functionality which is not yet testable in the
test environment we also update background.js.
background.js is only for test, it imitates production background page/script,
it doesn't have to match 100%, but at least return similar error object, so we
can easily test locally.

[saroyanm, 2018/01/23 15:26:06]

I think would be great if we could return the filter within the error object in
the messageResponder.js(filters.importRaw), so we will be sure that the "lines"
are the one we have initially passed.

[a.giammarchi, 2018/01/23 15:43:18]

yup, that's the "there's surely still stuff to do here" part of the ticket ;-)

[a.giammarchi, 2018/01/23 15:59:23]

about this ... the error object doesn't offer much. When it validates, it strips
out empty lines. We can either wait for another ticket to have the raw
input/processed data or I can just ignore with that split multiple new lines and
be done with it. I chose the fast way but I also would prefer the former one.

[saroyanm, 2018/01/24 09:05:49]

I think I couldn't well describe what I meant:
sendMessageHandleErrors method is called async and content of
E("custom-filters-raw") can be changed before callback is fired so
"lines[error.lineno - 1]" can have a wrong result.
I think we should process the E("custom-filters-raw").value in the message
responder rather than in the Async callback.

I did mean to propose updating "filterValidation", because it might take some
time as it's on the platform side, but not UI.

+            const editFilters = E("custom-filters-edit-filters");
+            editFilters.textContent = info.join("\n");

[saroyanm, 2018/01/23 15:26:05]

Adding all the elements as a textContent to div makes it less accessible. I'd
suggest to use a textarea or list with overflow, that should also help us not to
specify "many" class.

[a.giammarchi, 2018/01/23 15:43:18]

I either follow UI or I don't ... this comment is misleading to me. Have you
checked the image?

The many class is also accordingly to the UI. There is a border when there are
many items, your suggestion won't make that happen, unless I misunderstand.

[saroyanm, 2018/01/23 17:37:18]

I missed that, I didn't review the CSS, my fault.
But I think we still can change the element to make it more accessible.
I don't have good solution either for specifying the border depending on height
or child count.

+            if (errors.length > 5)
+              editFilters.classList.add("many");
+          }
+          else
+          {
+            setCustomFiltersView("read");
+          }
         });
         break;
       case "show-more-filters-section":
         E("more-filters").setAttribute("aria-hidden", false);
         break;
       case "switch-acceptable-ads":
         let value = element.value || element.dataset.value;
         // User check the checkbox
         let shouldCheck = element.getAttribute("aria-checked") != "true";
         let installAcceptableAds = false;
@@ skipping 79 matching lines @@
         break;
     }
   }
 
   function setCustomFiltersView(mode)
   {
     let customFiltersElement = E("custom-filters-raw");
     updateCustomFiltersUi();
     if (mode == "read")
     {
+      E("custom-filters-raw").classList.remove("warning");

[saroyanm, 2018/01/23 15:26:06]

Note: As mentioned above we don't need to remove the warning twice if we use
parent element.

+      E("custom-filters-edit-error").classList.remove("warning");
+      E("link-filters-on-edit-error").classList.remove("visible");
+      const editFilters = E("custom-filters-edit-filters");
+      editFilters.textContent = "";
+      editFilters.classList.remove("many");
       customFiltersElement.disabled = true;
       if (!customFiltersElement.value)
       {
         setCustomFiltersView("empty");
         return;
       }
     }
     else if (mode == "write")
     {
       customFiltersElement.disabled = false;
@@ skipping 143 matching lines @@
 
     // General tab
     getDocLink("contribute", (link) =>
     {
       E("contribute").href = link;
     });
     getDocLink("acceptable_ads_criteria", (link) =>
     {
       setLinks("enable-acceptable-ads-description", link);
     });
-    setElementText(E("tracking-warning-1"), "options_tracking_warning_1", 
+    setElementText(E("tracking-warning-1"), "options_tracking_warning_1",
       [getMessage("common_feature_privacy_title"),
-      getMessage("options_acceptableAds_ads_label")]);
-    setElementText(E("tracking-warning-3"), "options_tracking_warning_3", 
+       getMessage("options_acceptableAds_ads_label")]);
+    setElementText(E("tracking-warning-3"), "options_tracking_warning_3",
       [getMessage("options_acceptableAds_privacy_label")]);
 
     getDocLink("privacy_friendly_ads", (link) =>
     {
       E("enable-acceptable-ads-privacy-description").href = link;
     });
     getDocLink("adblock_plus_{browser}_dnt", url =>
     {
       setLinks("dnt", url);
     });
@@ skipping 22 matching lines @@
       what: "features"
     },
     (features) =>
     {
       hidePref("show_devtools_panel", !features.devToolsPanel);
     });
 
     getDocLink("filterdoc", (link) =>
     {
       E("link-filters").setAttribute("href", link);
+      E("link-filters-on-edit-error").setAttribute("href", link);
     });
 
     getDocLink("subscriptions", (link) =>
     {
       E("filter-lists-learn-more").setAttribute("href", link);
     });
 
     E("custom-filters-raw").setAttribute("placeholder",
       getMessage("options_customFilters_edit_placeholder", ["/ads/track/*"]));
 
@@ skipping 311 matching lines @@
         setPrivacyConflict();
         break;
       case "downloading":
       case "downloadStatus":
       case "homepage":
       case "lastDownload":
       case "title":
         updateSubscription(subscription);
         break;
       case "added":
-        let {url, recommended} = subscription;
+        let {url} = subscription;
         // Handle custom subscription
         if (/^~user/.test(url))
         {
           loadCustomFilters(subscription.filters);
           return;
         }
         else if (url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
@@ skipping 160 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);
 }