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, 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" ?

+  function sendMessageHandleErrors(message, callback)
   {
     browser.runtime.sendMessage(message, (errors) =>
     {
-      if (onMessage)
+      if (callback)
       {
         if (errors.length > 0)
-          onMessage(errors);
+          callback(errors);
         else
-          onMessage();
+          callback();
       }
     });
   }
 
   function switchTab(id)
   {
     location.hash = id;
   }
 
   function execAction(action, element)
@@ skipping 74 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");
+        const filters = E("custom-filters-raw").value;
         sendMessageHandleErrors({
           type: "filters.importRaw",
-          text: filters.value,
+          text: filters,
           removeExisting: true
         },
         (errors) =>
         {
           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.

+            E("custom-filters").classList.add("warning");
+            const customFiltersError = clearAndGetCustomFiltersError();
+
+            // The current error does not contain info about the line
+            // that generated such error.
+            // Whenever the error object will pass the bad filter
+            // within its properties, this split should be removed.
+            const lines = filters.split("\n");
+            const messages = errors.map(error => lines[error.lineno - 1]);
+            for (const message of messages)
+            {
+              const li = document.createElement("li");
+              customFiltersError.appendChild(li).textContent = message;
+            }
             if (errors.length > 5)
-              editFilters.classList.add("many");
+              customFiltersError.classList.add("many");
           }
           else
           {
             setCustomFiltersView("read");
           }
         });
         break;
       case "show-more-filters-section":
         E("more-filters").setAttribute("aria-hidden", false);
         break;
@@ skipping 78 matching lines @@
           closeDialog();
         }
         else
         {
           form.querySelector(":invalid").focus();
         }
         break;
     }
   }
 
+  function clearAndGetCustomFiltersError()
+  {
+    const customFiltersError = E("custom-filters-error");
+    customFiltersError.textContent = "";
+    customFiltersError.classList.remove("many");
+    return customFiltersError;
+  }
+
   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");
+      E("custom-filters").classList.remove("warning");
+      clearAndGetCustomFiltersError();
       customFiltersElement.disabled = true;
       if (!customFiltersElement.value)
       {
         setCustomFiltersView("empty");
         return;
       }
     }
     else if (mode == "write")
     {
       customFiltersElement.disabled = false;
@@ skipping 188 matching lines @@
       type: "app.get",
       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);
+      E("link-filters-1").setAttribute("href", link);
+      E("link-filters-2").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 174 matching lines @@
       special: true
     },
     (subscriptions) =>
     {
       // Load filters
       for (let subscription of subscriptions)
       {
         browser.runtime.sendMessage({
           type: "filters.get",
           subscriptionUrl: subscription.url
-        },
-        (filters) =>
-        {
-          loadCustomFilters(filters);
-        });
+        }, loadCustomFilters);
       }
     });
     loadRecommendations();
     browser.runtime.sendMessage({
       type: "prefs.get",
       key: "subscriptions_exceptionsurl"
     },
     (url) =>
     {
       acceptableAdsUrl = url;
@@ skipping 293 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);
 }