Issue 5169 - Add whitelisted tab to the new options page

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -43,18 +43,25 @@
     this.items = [];
   }
 
-  Collection.prototype._setEmpty = function(table, text)
+  Collection.prototype._setEmpty = function(table, texts)
   {
-    let placeholder = table.querySelector(".empty-placeholder");
-    if (text && !placeholder)
+    let placeholders = table.querySelectorAll(".empty-placeholder");
+
+    if (texts && placeholders.length == 0)
     {
-      placeholder = document.createElement("li");
-      placeholder.className = "empty-placeholder";
-      placeholder.textContent = getMessage(text);
-      table.appendChild(placeholder);
+      for (let i = 0; i < texts.length; i++)

[Thomas Greiner, 2017/05/09 13:42:56]

Detail: You're not using the index so I'd recommend using a for-of loop instead.

Note that we may not be able to use one for the loop below because it doesn't
iterate through an array. Older browser versions don't support that so if you
want to replace that one as well then you'd first have to check against
compatibility information.

[saroyanm, 2017/05/16 20:20:07]

Well spotted, done.
We already looping collections of nodes, so I'll use it here for consistency
also, also It's supported by Chrome 51+, while we support Chrome 55+. So in both
cases we can use it here.

+      {
+        let placeholder = document.createElement("li");
+        placeholder.className = "empty-placeholder";
+        placeholder.textContent = getMessage(texts[i]);
+        table.appendChild(placeholder);
+      }
     }
-    else if (placeholder)
-      table.removeChild(placeholder);
+    else if (placeholders.length > 0)
+    {
+      for (let i = 0; i < placeholders.length; i++)
+        table.removeChild(placeholders[i]);
+    }
   };
 
   Collection.prototype._createElementQuery = function(item)
@@ -284,17 +291,17 @@
   collections.langs = new Collection([
     {
       id: "blocking-languages-table",
-      emptyText: "options_dialog_language_added_empty"
+      emptyText: ["options_dialog_language_added_empty"]
     },
     {
       id: "blocking-languages-dialog-table",
-      emptyText: "options_dialog_language_added_empty"
+      emptyText: ["options_dialog_language_added_empty"]
     }
   ]);
   collections.allLangs = new Collection([
     {
       id: "all-lang-table",
-      emptyText: "options_dialog_language_other_empty",
+      emptyText: ["options_dialog_language_other_empty"],
       searchable: true
     }
   ]);
@@ -311,13 +318,13 @@
   collections.whitelist = new Collection([
     {
       id: "whitelisting-table",
-      emptyText: "options_whitelisted_empty"
+      emptyText: ["options_whitelist_empty_1", "options_whitelist_empty_2"]
     }
   ]);
   collections.customFilters = new Collection([
     {
       id: "custom-filters-table",
-      emptyText: "options_customFilters_empty"
+      emptyText: ["options_customFilters_empty"]
     }
   ]);
   collections.filterLists = new Collection([
@@ -767,10 +774,57 @@
     let placeholderValue = getMessage("options_dialog_language_find");
     E("find-language").setAttribute("placeholder", placeholderValue);
     E("find-language").addEventListener("keyup", onFindLanguageKeyUp, false);
-    E("whitelisting-textbox").addEventListener("keypress", (e) =>
+    let exampleValue = getMessage("options_whitelist_placeholder_example");
+    exampleValue += " www.example.com";
+    E("whitelisting-textbox").setAttribute("placeholder", exampleValue);
+    E("whitelisting-textbox").addEventListener("keyup", (e) =>

[Thomas Greiner, 2017/05/09 13:42:56]

I'm not sure it's a good idea to validate the input on each key press because
won't this cause error messages to show up as soon as you start typing? Because
the first characters won't be a valid host name yet.

[saroyanm, 2017/05/16 20:20:06]

It will, that's why I'm using keyUp instead.

[saroyanm, 2017/05/18 16:21:52]

Fine for now because we are checking for only empty state.

     {
+      let addWhitelistButton = E("whitelisting-add-button");
+      let validationElement = E("whitelisting-validation");
       if (getKey(e) == "Enter")
-        addWhitelistedDomain();
+      {
+        if (!E("whitelisting-add-button").hasAttribute("disabled"))
+          addWhitelistedDomain();
+      }
+      else
+      {
+        let validIpAddressRegex = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25"

[saroyanm, 2017/04/21 12:03:17]

Regular expression to match Hostname and IP addresses as specified:
http://stackoverflow.com/questions/106179/regular-expression-to-match-dns-hos...

[Thomas Greiner, 2017/05/09 13:42:56]

Detail: Those are constants that we can simply compile once during page load
rather than on each key press.

[Thomas Greiner, 2017/05/09 13:42:56]

I don't think we need to be that specific. Something simple like
`/^\d+\.\d+\.\d+\.\d+$/` would be sufficient to verify the syntax. Anything else
will fail during the download attempt.

Also note that this only covers IPv4 addresses, not IPv6 ones.

See https://tools.ietf.org/html/rfc3986#section-3.2.2

[saroyanm, 2017/05/16 20:20:07]

I think this is not anymore relevant, while the validation has been removed from
the specifications, same applies to validation comments below.

+          + "[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$";

[Thomas Greiner, 2017/05/09 13:42:56]

You need to escape the escape characters since you're using string literals
rather than regular expression literals.

i.e. `/a\.b/` is equivalent to `new RegExp("a\\.b")`

+        let validHostnameRegex = "^(([a-zA-Z]|[a-zA-Z][a-zA-Z0-9\-]*[a-zA-Z0-9]"

[Thomas Greiner, 2017/05/09 13:42:56]

Same here. I don't think we need to be that specific.

Also note that this only covers ASCII-based domains but not internationalized
domain names (e.g. http://яндекс.рф/).

See https://tools.ietf.org/html/rfc3490

Furthermore, we should ensure that we cover exceptions such as "localhost" and
"foo.dev:3000".

Considering the necessary effort, it might be best not to include URL validation
in the first version of the new options page.

+          +")\.)*([A-Za-z]|[A-Za-z][A-Za-z0-9\-]*[A-Za-z0-9])$";
+
+        let isDuplicate = false;
+        for (let i = 0; i < collections.whitelist.items.length; i++)
+        {
+          if (collections.whitelist.items[i].title == e.target.value)
+            isDuplicate = true;

[Thomas Greiner, 2017/05/09 13:42:56]

Detail: Why do we need this variable? We could just place the relevant code here
and return afterwards.

+        }
+
+        if (isDuplicate)
+        {
+          addWhitelistButton.setAttribute("disabled", "");

[saroyanm, 2017/04/21 12:03:17]

I think we should use checkValidity() method instead and implement all the
checks on HTML element including RegExp also implement <form> as commented in
options.html.

[Thomas Greiner, 2017/05/09 13:42:56]

Detail: Can't we just do `addWhitelistButton.disabled = true;` throughout this
function?

[saroyanm, 2017/05/16 20:20:06]

done.

+          validationElement.textContent = 
+            getMessage("options_whitelist_duplicate");
+        }
+        else if (new RegExp(validIpAddressRegex).test(e.target.value) ||
+          new RegExp(validHostnameRegex).test(e.target.value))
+        {
+          addWhitelistButton.removeAttribute("disabled");
+          validationElement.textContent = "";
+        }
+        else if (!e.target.value)
+        {
+          validationElement.textContent = "";
+          addWhitelistButton.setAttribute("disabled", "");
+        }
+        else
+        {
+          addWhitelistButton.setAttribute("disabled", "");
+          validationElement.textContent = 
+            getMessage("options_whitelist_invalid");
+
+        }
+      }
     }, false);
 
     // Advanced tab
@@ -986,8 +1040,7 @@
     }
 
     domain.value = "";
-    document.querySelector("#whitelisting .controls")
-      .classList.remove("mode-edit");
+    E("whitelisting-add-button").setAttribute("disabled", "");
   }
 
   function editCustomFilters()