Issue 5549 - Implement missing error handlings for custom filter lists

desktop-options.js

Index: desktop-options.js
===================================================================
--- a/desktop-options.js
+++ b/desktop-options.js
@@ -547,14 +547,17 @@
     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();
+      }
     });
   }
 
@@ -648,14 +651,30 @@
         });
         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":
@@ -755,6 +774,12 @@
     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)
       {
@@ -918,10 +943,10 @@
     {
       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) =>
@@ -964,6 +989,7 @@
     getDocLink("filterdoc", (link) =>
     {
       E("link-filters").setAttribute("href", link);
+      E("link-filters-on-edit-error").setAttribute("href", link);
     });
 
     getDocLink("subscriptions", (link) =>
@@ -1295,7 +1321,7 @@
         updateSubscription(subscription);
         break;
       case "added":
-        let {url, recommended} = subscription;
+        let {url} = subscription;
         // Handle custom subscription
         if (/^~user/.test(url))
         {