Issue 5169 - Add whitelisted tab to the new options page

new-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* globals checkShareResource, getDocLink, i18nFormatDateTime, openSharePopup,
            E */
 
 "use strict";
 
 {
   let subscriptionsMap = Object.create(null);
   let filtersMap = Object.create(null);
   let collections = Object.create(null);
   let acceptableAdsUrl = null;
+  let isCustomFiltersLoaded = false;
   let {getMessage} = ext.i18n;
   let filterErrors = new Map([
     ["synchronize_invalid_url",
      "options_filterList_lastDownload_invalidURL"],
     ["synchronize_connection_error",
      "options_filterList_lastDownload_connectionError"],
     ["synchronize_invalid_data",
      "options_filterList_lastDownload_invalidData"],
     ["synchronize_checksum_mismatch",
      "options_filterList_lastDownload_checksumMismatch"]
   ]);
+  const timestampUI = Symbol();
 
   function Collection(details)
   {
     this.details = details;
     this.items = [];
   }
 
   Collection.prototype._setEmpty = function(table, texts)
   {
     let placeholders = table.querySelectorAll(".empty-placeholder");
 
     if (texts && placeholders.length == 0)
     {
-      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.

+      for (let text of texts)
       {
         let placeholder = document.createElement("li");
         placeholder.className = "empty-placeholder";
-        placeholder.textContent = getMessage(texts[i]);
+        placeholder.textContent = getMessage(text);
         table.appendChild(placeholder);
       }
     }
     else if (placeholders.length > 0)
     {
-      for (let i = 0; i < placeholders.length; i++)
-        table.removeChild(placeholders[i]);
+      for (let placeholder of placeholders)
+        table.removeChild(placeholder);
     }
   };
 
   Collection.prototype._createElementQuery = function(item)
   {
     let access = (item.url || item.text).replace(/'/g, "\\'");
     return function(container)
     {
       return container.querySelector("[data-access='" + access + "']");
     };
   };
 
   Collection.prototype._getItemTitle = function(item, i)
   {
     if (item.url == acceptableAdsUrl)
       return getMessage("options_acceptableAds_description");
     if (this.details[i].useOriginalTitle && item.originalTitle)
       return item.originalTitle;
     return item.title || item.url || item.text;
   };
 
-  Collection.prototype.addItem = function(item)
-  {
-    if (this.items.indexOf(item) >= 0)
-      return;
-
-    this.items.push(item);
+  Collection.prototype._sortItems = function()
+  {
     this.items.sort((a, b) =>
     {
       // Make sure that Acceptable Ads is always last, since it cannot be
       // disabled, but only be removed. That way it's grouped together with
       // the "Own filter list" which cannot be disabled either at the bottom
       // of the filter lists in the Advanced tab.
       if (a.url == acceptableAdsUrl)
         return 1;
       if (b.url == acceptableAdsUrl)
         return -1;
 
+      // Make sure that newly added entries always appear on top in descending
+      // chronological order
+      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);
       let template = table.querySelector("template");
       let listItem = document.createElement("li");
       listItem.appendChild(document.importNode(template.content, true));
       listItem.setAttribute("aria-label", this._getItemTitle(item, j));
       listItem.setAttribute("data-access", item.url || item.text);
       listItem.setAttribute("role", "section");
 
@@ skipping 61 matching lines @@
       }
 
       element.parentElement.removeChild(element);
       if (this.items.length == 0)
         this._setEmpty(table, detail.emptyText);
     }
   };
 
   Collection.prototype.updateItem = function(item)
   {
+    let oldIndex = this.items.indexOf(item);
+    this._sortItems();
     let access = (item.url || item.text).replace(/'/g, "\\'");
     for (let i = 0; i < this.details.length; i++)
     {
       let table = E(this.details[i].id);
       let element = table.querySelector("[data-access='" + access + "']");
       if (!element)
         continue;
 
       let title = this._getItemTitle(item, i);
       element.querySelector(".display").textContent = title;
       element.setAttribute("aria-label", title);
       if (this.details[i].searchable)
         element.setAttribute("data-search", title.toLowerCase());
       let control = element.querySelector(".control[role='checkbox']");
       if (control)
       {
         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");
       let timeElement = element.querySelector(".time");
       if (dateElement && timeElement)
       {
         let message = element.querySelector(".message");
         if (item.isDownloading)
         {
           let text = getMessage("options_filterList_lastDownload_inProgress");
@@ skipping 23 matching lines @@
       {
         if (item.homepage)
           websiteElement.setAttribute("href", item.homepage);
         else
           websiteElement.setAttribute("aria-hidden", true);
       }
 
       let sourceElement = element.querySelector(".context-menu .source");
       if (sourceElement)
         sourceElement.setAttribute("href", item.url);
+
+      let newIndex = this.items.indexOf(item);
+      if (oldIndex != newIndex)
+        table.insertBefore(element, table.childNodes[newIndex]);
     }
   };
 
   Collection.prototype.clearAll = function()
   {
     this.items = [];
     for (let detail of this.details)
     {
       let table = E(detail.id);
       let element = table.firstChild;
@@ skipping 213 matching lines @@
       else
         location.href = link;
     });
   }
 
   function switchTab(id)
   {
     location.hash = id;
   }
 
+  function execAction(action, element)
+  {
+    switch (action)
+    {
+      case "add-domain-exception":
+        addWhitelistedDomain();
+        break;
+      case "add-language-subscription":
+        addEnableSubscription(findParentData(element, "access", false));
+        break;
+      case "add-predefined-subscription": {
+        let dialog = E("dialog-content-predefined");
+        let title = dialog.querySelector("h3").textContent;
+        let url = dialog.querySelector(".url").textContent;
+        addEnableSubscription(url, title);
+        closeDialog();
+        break;
+      }
+      case "cancel-custom-filters":
+        E("custom-filters").classList.remove("mode-edit");
+        break;
+      case "close-dialog":
+        closeDialog();
+        break;
+      case "edit-custom-filters":
+        editCustomFilters();
+        break;
+      case "import-subscription": {
+        let url = E("blockingList-textbox").value;
+        addEnableSubscription(url);
+        closeDialog();
+        break;
+      }
+      case "open-context-menu": {
+        let listItem = findParentData(element, "access", true);
+        if (listItem && !listItem.classList.contains("show-context-menu"))
+          listItem.classList.add("show-context-menu");
+        break;
+      }
+      case "open-dialog": {
+        let dialog = findParentData(element, "dialog", false);
+        openDialog(dialog);
+        break;
+      }
+      case "open-doclink": {
+        let doclink = findParentData(element, "doclink", false);
+        openDocLink(doclink);
+        break;
+      }
+      case "remove-filter":
+        ext.backgroundPage.sendMessage({
+          type: "filters.remove",
+          text: findParentData(element, "access", false)
+        });
+        break;
+      case "remove-subscription":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: findParentData(element, "access", false)
+        });
+        break;
+      case "save-custom-filters":
+        sendMessageHandleErrors({
+          type: "filters.importRaw",
+          text: E("custom-filters-raw").value,
+          removeExisting: true
+        },
+        () =>
+        {
+          E("custom-filters").classList.remove("mode-edit");
+        });
+        break;
+      case "switch-tab":
+        let tabId = findParentData(element, "tab", false);
+        switchTab(tabId);
+        break;
+      case "toggle-disable-subscription":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.toggle",
+          keepInstalled: true,
+          url: findParentData(element, "access", false)
+        });
+        break;
+      case "toggle-pref":
+        ext.backgroundPage.sendMessage({
+          type: "prefs.toggle",
+          key: findParentData(element, "pref", false)
+        });
+        break;
+      case "toggle-remove-subscription":
+        let subscriptionUrl = findParentData(element, "access", false);
+        if (element.getAttribute("aria-checked") == "true")
+        {
+          ext.backgroundPage.sendMessage({
+            type: "subscriptions.remove",
+            url: subscriptionUrl
+          });
+        }
+        else
+          addEnableSubscription(subscriptionUrl);
+        break;
+      case "update-all-subscriptions":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.update"
+        });
+        break;
+      case "update-subscription":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.update",
+          url: findParentData(element, "access", false)
+        });
+        break;
+    }
+  }
+
   function onClick(e)
   {
     let context = document.querySelector(".show-context-menu");
     if (context)
       context.classList.remove("show-context-menu");
 
-    let element = findParentData(e.target, "action", true);
-    if (!element)
+    let actions = findParentData(e.target, "action", false);
+    if (!actions)
       return;
 
-    let actions = element.getAttribute("data-action").split(",");
+    actions = actions.split(",");
     for (let action of actions)
     {
-      switch (action)
-      {
-        case "add-domain-exception":
-          addWhitelistedDomain();
-          break;
-        case "add-predefined-subscription": {
-          let dialog = E("dialog-content-predefined");
-          let title = dialog.querySelector("h3").textContent;
-          let url = dialog.querySelector(".url").textContent;
-          addEnableSubscription(url, title);
-          closeDialog();
-          break;
-        }
-        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":
-          E("custom-filters").classList.add("mode-edit");
-          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);
-          closeDialog();
-          break;
-        }
-        case "open-dialog": {
-          let dialog = findParentData(element, "dialog", false);
-          openDialog(dialog);
-          break;
-        }
-        case "open-doclink": {
-          let doclink = findParentData(element, "doclink", false);
-          openDocLink(doclink);
-          break;
-        }
-        case "save-custom-filters":
-          sendMessageHandleErrors({
-            type: "filters.importRaw",
-            text: E("custom-filters-raw").value,
-            removeExisting: true
-          },
-          () =>
-          {
-            E("custom-filters").classList.remove("mode-edit");
-          });
-          break;
-        case "switch-tab": {
-          let tabId = findParentData(e.target, "tab", false);
-          switchTab(tabId);
-          break;
-        }
-        case "toggle-pref":
-          ext.backgroundPage.sendMessage({
-            type: "prefs.toggle",
-            key: findParentData(element, "pref", false)
-          });
-          break;
-        case "update-all-subscriptions":
-          ext.backgroundPage.sendMessage({
-            type: "subscriptions.update"
-          });
-          break;
-        case "open-context-menu": {
-          let listItem = findParentData(element, "access", true);
-          if (listItem != context)
-            listItem.classList.add("show-context-menu");
-          break;
-        }
-        case "update-subscription":
-          ext.backgroundPage.sendMessage({
-            type: "subscriptions.update",
-            url: findParentData(element, "access", false)
-          });
-          break;
-        case "remove-subscription":
-          ext.backgroundPage.sendMessage({
-            type: "subscriptions.remove",
-            url: findParentData(element, "access", false)
-          });
-          break;
-        case "toggle-remove-subscription": {
-          let subscriptionUrl = findParentData(element, "access", false);
-          if (element.getAttribute("aria-checked") == "true")
-          {
-            ext.backgroundPage.sendMessage({
-              type: "subscriptions.remove",
-              url: subscriptionUrl
-            });
-          }
-          else
-            addEnableSubscription(subscriptionUrl);
-          break;
-        }
-        case "toggle-disable-subscription":
-          ext.backgroundPage.sendMessage({
-            type: "subscriptions.toggle",
-            keepInstalled: true,
-            url: findParentData(element, "access", false)
-          });
-          break;
-        case "add-language-subscription":
-          addEnableSubscription(findParentData(element, "access", false));
-          break;
-        case "remove-filter":
-          ext.backgroundPage.sendMessage({
-            type: "filters.remove",
-            text: findParentData(element, "access", false)
-          });
-          break;
-      }
+      execAction(action, e.target);
     }
   }
 
   function getKey(e)
   {
     // e.keyCode has been deprecated so we attempt to use e.key
     if ("key" in e)
       return e.key;
     return getKey.keys[e.keyCode];
   }
@@ skipping 15 matching lines @@
       return;
 
     let container = findParentData(element, "action", true);
     if (!container || !container.hasAttribute("data-keys"))
       return;
 
     let keys = container.getAttribute("data-keys").split(" ");
     if (keys.indexOf(key) < 0)
       return;
 
-    switch (container.getAttribute("data-action"))
-    {
-      case "add-domain-exception":
-        addWhitelistedDomain();
-        break;
-      case "open-doclink":
-        let doclink = findParentData(element, "doclink", false);
-        openDocLink(doclink);
-        break;
-      case "switch-tab":
-        if (key == "Enter")
-        {
-          let tabId = findParentData(element, "tab", false);
-          switchTab(tabId);
-        }
-        else if (element.hasAttribute("aria-selected"))
-        {
-          if (key == "ArrowLeft" || key == "ArrowUp")
-          {
-            element = element.previousElementSibling ||
-                container.lastElementChild;
-          }
-          else if (key == "ArrowRight" || key == "ArrowDown")
-          {
-            element = element.nextElementSibling ||
-                container.firstElementChild;
-          }
-
-          let tabId = findParentData(element, "tab", false);
-          switchTab(tabId);
-        }
-        break;
+    if (element.getAttribute("role") == "tab")
+    {
+      if (key == "ArrowLeft" || key == "ArrowUp")
+        element = element.previousElementSibling || container.lastElementChild;
+      else if (key == "ArrowRight" || key == "ArrowDown")
+        element = element.nextElementSibling || container.firstElementChild;
+    }
+
+    let actions = container.getAttribute("data-action").split(",");
+    for (let action of actions)
+    {
+      execAction(action, element);
     }
   }
 
   function selectTabItem(tabId, container, focus)
   {
     // Show tab content
     document.body.setAttribute("data-tab", tabId);
 
     // Select tab
     let tabList = container.querySelector("[role='tablist']");
@@ skipping 70 matching lines @@
 
     updateShareLink();
     updateTooltips();
 
     // Initialize interactive UI elements
     document.body.addEventListener("click", onClick, false);
     document.body.addEventListener("keyup", onKeyUp, false);
     let placeholderValue = getMessage("options_dialog_language_find");
     E("find-language").setAttribute("placeholder", placeholderValue);
     E("find-language").addEventListener("keyup", onFindLanguageKeyUp, false);
-    let exampleValue = getMessage("options_whitelist_placeholder_example");
-    exampleValue += " www.example.com";
+    let exampleValue = getMessage("options_whitelist_placeholder_example", 
+      ["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")
-      {
-        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");
-
-        }
-      }
+      E("whitelisting-add-button").disabled = !e.target.value;
     }, false);
 
     // Advanced tab
     let tweaks = document.querySelectorAll("#tweaks li[data-pref]");
     tweaks = Array.prototype.map.call(tweaks, (checkbox) =>
     {
       return checkbox.getAttribute("data-pref");
     });
     for (let key of tweaks)
     {
@@ skipping 154 matching lines @@
       for (let subscription of subscriptions)
       {
         ext.backgroundPage.sendMessage({
           type: "filters.get",
           subscriptionUrl: subscription.url
         },
         (filters) =>
         {
           for (let filter of filters)
             updateFilter(filter);
+
+          isCustomFiltersLoaded = true;
         });
       }
     });
     loadRecommendations();
     ext.backgroundPage.sendMessage({
       type: "prefs.get",
       key: "subscriptions_exceptionsurl"
     },
     (url) =>
     {
@@ skipping 12 matching lines @@
       {
         for (let subscription of subscriptions)
           onSubscriptionMessage("added", subscription);
       });
     });
   }
 
   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({
         type: "filters.add",
         text: "@@||" + domain.value.toLowerCase() + "^$document"
       });
     }
 
     domain.value = "";
-    E("whitelisting-add-button").setAttribute("disabled", "");
+    E("whitelisting-add-button").disabled = true;
   }
 
   function editCustomFilters()
   {
+    if (!isCustomFiltersLoaded)
+    {
+      console.error("Custom filters are not loaded");
+      return;
+    }
+
+    E("custom-filters").classList.add("mode-edit");
     let filterTexts = [];
     for (let customFilterItem of collections.customFilters.items)
       filterTexts.push(customFilterItem.text);
     E("custom-filters-raw").value = filterTexts.join("\n");
   }
 
   function addEnableSubscription(url, title, homepage)
   {
     let messageType = null;
     let knownSubscription = subscriptionsMap[url];
@@ skipping 12 matching lines @@
       message.homepage = homepage;
 
     ext.backgroundPage.sendMessage(message);
   }
 
   function onFilterMessage(action, filter)
   {
     switch (action)
     {
       case "added":
+        filter[timestampUI] = Date.now();
         updateFilter(filter);
         updateShareLink();
         break;
       case "loaded":
         populateLists();
         break;
       case "removed":
         let knownFilter = filtersMap[filter.text];
         collections.whitelist.removeItem(knownFilter);
         collections.customFilters.removeItem(knownFilter);
@@ skipping 260 matching lines @@
   });
   ext.backgroundPage.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }