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
@@ -37,6 +37,7 @@
     ["synchronize_checksum_mismatch",
      "options_filterList_lastDownload_checksumMismatch"]
   ]);
+  const timestampUI = Symbol();
 
   function Collection(details)
   {
@@ -44,18 +45,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 text of texts)
+      {
+        let placeholder = document.createElement("li");
+        placeholder.className = "empty-placeholder";
+        placeholder.textContent = getMessage(text);
+        table.appendChild(placeholder);
+      }
     }
-    else if (placeholder)
-      table.removeChild(placeholder);
+    else if (placeholders.length > 0)
+    {
+      for (let placeholder of placeholders)
+        table.removeChild(placeholder);
+    }
   };
 
   Collection.prototype._createElementQuery = function(item)
@@ -76,12 +84,8 @@
     return item.title || item.url || item.text;
   };
 
-  Collection.prototype.addItem = function(item)
+  Collection.prototype._sortItems = function()
   {
-    if (this.items.indexOf(item) >= 0)
-      return;
-
-    this.items.push(item);
     this.items.sort((a, b) =>
     {
       // Make sure that Acceptable Ads is always last, since it cannot be
@@ -93,11 +97,26 @@
       if (b.url == acceptableAdsUrl)
         return -1;
 
+      // Make sure that duplicated whitelist entries are always being moved to

[Thomas Greiner, 2017/06/16 10:35:45]

Detail: This is not about handling duplicated entries. This is about making sure
that newly added entries always appear on top.

[saroyanm, 2017/06/16 11:13:54]

That's a very good question, I didn't know about that behavior.

I've created an issue for the specs to clarify this ->
https://bitbucket.org/adblockplus/spec/issues/35/how-lists-should-be-sorted

I think it's a critical issue as it blocks current review (@wspee).

[saroyanm, 2017/06/16 13:05:11]

The mentioned behavior was confirmed, so I'll update this with the next patch.

[saroyanm, 2017/06/16 16:44:07]

Done.

+      // the top of the list
+      let aTimestamp = a[timestampUI] || 0;
+      let bTimestamp = b[timestampUI] || 0;
+      if (aTimestamp || bTimestamp)
+        return bTimestamp - aTimestamp;
+
       let aTitle = this._getItemTitle(a, 0).toLowerCase();
       let bTitle = this._getItemTitle(b, 0).toLowerCase();
       return aTitle.localeCompare(bTitle);
     });
+  };
 
+  Collection.prototype.addItem = function(item)
+  {
+    if (this.items.indexOf(item) >= 0)
+      return;
+
+    this.items.push(item);
+    this._sortItems();
     for (let j = 0; j < this.details.length; j++)
     {
       let table = E(this.details[j].id);
@@ -179,6 +198,8 @@
 
   Collection.prototype.updateItem = function(item)
   {
+    var oldIndex = this.items.indexOf(item);
+    this._sortItems();
     let access = (item.url || item.text).replace(/'/g, "\\'");
     for (let i = 0; i < this.details.length; i++)
     {
@@ -197,7 +218,7 @@
       {
         control.setAttribute("aria-checked", item.disabled == false);
         if (item.url == acceptableAdsUrl && this == collections.filterLists)
-          control.setAttribute("disabled", true);
+          control.disabled = true;
       }
 
       let dateElement = element.querySelector(".date");
@@ -241,6 +262,9 @@
       let sourceElement = element.querySelector(".context-menu .source");
       if (sourceElement)
         sourceElement.setAttribute("href", item.url);
+
+      if (oldIndex != this.items.indexOf(item))

[Thomas Greiner, 2017/06/16 10:35:46]

Detail: `this.items.indexOf(item)` is duplicated so why not introduce a
`newIndex` variable to make things a bit easier to read?

[saroyanm, 2017/06/16 16:44:06]

Done.

+        table.insertBefore(element, table.childNodes[this.items.indexOf(item)]);
     }
   };
 
@@ -285,17 +309,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
     }
   ]);
@@ -312,13 +336,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([
@@ -495,22 +519,12 @@
       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": {
         let url = E("blockingList-textbox").value;
         addEnableSubscription(url);
@@ -750,10 +764,22 @@
     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 = getMessages("options_whitelist_placeholder_example", 

[saroyanm, 2017/06/14 11:51:22]

Nit: This should be getMessage(...)

[Thomas Greiner, 2017/06/16 10:35:46]

Indeed.

[saroyanm, 2017/06/16 16:43:07]

Done.

+      ["www.example.com"]);
+    E("whitelisting-textbox").setAttribute("placeholder", exampleValue);
+    E("whitelisting-textbox").addEventListener("keyup", (e) =>
     {
+      let addWhitelistButton = E("whitelisting-add-button");
+      addWhitelistButton.disabled = false;
       if (getKey(e) == "Enter")
-        addWhitelistedDomain();
+      {
+        if (!addWhitelistButton.disabled)
+          addWhitelistedDomain();

[Thomas Greiner, 2017/06/16 10:35:46]

This will clear the input field so the whitelist button should be disabled as
far as I understand.

To clarify this I'd propose to write:

let addWhitelistButton = E("whitelisting-add-button");
if (getKey(e) == "Enter")
{
  addWhitelistedDomain();
  e.target.value = "";
}
addWhitelistButton.disabled = !e.target.value;

and remove the line `domain = "";` from `addWhitelistedDomain()`.

[saroyanm, 2017/06/16 15:53:45]

We do need:
`
domain.value = "";
E("whitelisting-add-button").disabled = true;
`

Lines in `addWhitelistedDomain()`, while we need to execute that not only on
KeyUp, but also on click on the button.

I think the implementation should be changed in current way:
`
E("whitelisting-textbox").addEventListener("keyup", (e) =>
{
  addWhitelistButton.disabled = !e.target.value;
}, false);
`

And keep the addWhitelistedDomain(); as it is.. I think I just duplicated here
the logic which should only go to "addWhitelistedDomain()";

(I'll upload the fix with the new patch soon)

[saroyanm, 2017/06/16 16:43:07]

Done.

+      }
+      else
+      {
+        addWhitelistButton.disabled = !e.target.value;
+      }
     }, false);
 
     // Advanced tab
@@ -962,6 +988,16 @@
   function addWhitelistedDomain()
   {
     let domain = E("whitelisting-textbox");
+    for (let whitelistItem of collections.whitelist.items)
+    {
+      if (whitelistItem.title == domain.value)
+      {
+        whitelistItem[timestampUI] = Date.now();
+        collections.whitelist.updateItem(whitelistItem);
+        domain.value = "";
+        break;
+      }
+    }
     if (domain.value)
     {
       sendMessageHandleErrors({
@@ -971,8 +1007,7 @@
     }
 
     domain.value = "";
-    document.querySelector("#whitelisting .controls")
-      .classList.remove("mode-edit");
+    E("whitelisting-add-button").disabled = true;
   }
 
   function editCustomFilters()