Issue 5255 - Advanced tab (HTML, strings and functionality)

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 customFiltersArray = [];

[Thomas Greiner, 2017/07/14 12:26:13]

Detail: I'd recommend not to include the value type in the variable name. For
instance, the variable name "customFiltersListElement" will need to be updated
if we decide not to use `<li>` elements to represent those anymore.

Something more generic like "customFilters" instead of "customFiltersArray" and
"customFiltersElement" instead of "customFiltersListElement" would already be
sufficient to avoid such inconsistencies.

[saroyanm, 2017/07/14 16:17:25]

Acknowledged.

[saroyanm, 2017/07/14 17:11:06]

Done.

+  let customFilters = [];
   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();
   const whitelistedDomainRegexp = /^@@\|\|([^/:]+)\^\$document$/;
   // Period of time in milliseconds
   const minuteInMs = 60000;
   const hourInMs = 3600000;
   const fullDayInMs = 86400000;
 
   function Collection(details)
   {
     this.details = details;
     this.items = [];
   }
 
-  Collection.prototype._setEmpty = function(table, text)
-  {
-    let placeholder = table.querySelector(".empty-placeholder");
-    if (text && !placeholder)
-    {
-      placeholder = document.createElement("li");
-      placeholder.className = "empty-placeholder";
-      placeholder.textContent = getMessage(text);
-      table.appendChild(placeholder);
-    }
-    else if (placeholder)
-      table.removeChild(placeholder);
+  Collection.prototype._setEmpty = function(table, texts)
+  {
+    let placeholders = table.querySelectorAll(".empty-placeholder");
+
+    if (texts && placeholders.length == 0)
+    {
+      for (let text of texts)
+      {
+        let placeholder = document.createElement("li");
+        placeholder.className = "empty-placeholder";
+        placeholder.textContent = getMessage(text);
+        table.appendChild(placeholder);
+      }
+    }
+    else if (placeholders.length > 0)
+    {
+      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 detail = this.details[j];
       let table = E(detail.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 60 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 lastUpdateElement = element.querySelector(".last-update");
       if (lastUpdateElement)
       {
         let message = element.querySelector(".message");
         if (item.isDownloading)
         {
           let text = getMessage("options_filterList_lastDownload_inProgress");
           message.textContent = text;
           element.classList.add("show-message");
         }
         else if (item.downloadStatus != "synchronize_ok")
         {
           let error = filterErrors.get(item.downloadStatus);
           if (error)
             message.textContent = getMessage(error);
           else
             message.textContent = item.downloadStatus;
           element.classList.add("show-message");
         }
         else if (item.lastDownload > 0)
         {
           let lastUpdate = item.lastDownload * 1000;
-          let lastUpdateLive = function()
+          let sinceUpdate = Date.now() - lastUpdate;
+          if (sinceUpdate > fullDayInMs)
           {
-            let sinceUpdate = Date.now() - lastUpdate;
-            if (sinceUpdate > fullDayInMs)
-            {
-              let lastUpdateDate = new Date(item.lastDownload * 1000);
-              let monthName = lastUpdateDate.toLocaleString(undefined,
-                {month: "short"});
-              let day = lastUpdateDate.getDate();
-              day = day < 10 ? "0" + day : day;
-              lastUpdateElement.textContent = day + " " + monthName + " " +
-                lastUpdateDate.getFullYear();
-            }
-            else if (sinceUpdate > hourInMs)
-            {
-              let placeholder = [Math.round(sinceUpdate / hourInMs)];
-              lastUpdateElement.textContent =
-                getMessage("options_filterList_hours", placeholder);

[Thomas Greiner, 2017/07/14 12:26:12]

This placeholder isn't defined in new-options.json and in the last meeting we
agreed on leaving out the numbers as a compromise to not having to implement the
live updating of the dates.

Therefore there'd be no need for this placeholder.

[saroyanm, 2017/07/14 16:17:24]

Well spotted, will remove it.

[saroyanm, 2017/07/14 17:11:06]

Done.

-            }
-            else if (sinceUpdate > minuteInMs)
-            {
-              let placeholder = [Math.round(sinceUpdate / minuteInMs)];
-              lastUpdateElement.textContent =
-                getMessage("options_filterList_minutes", placeholder);
-            }
-            else
-            {
-              lastUpdateElement.textContent =
-                getMessage("options_filterList_now");
-            }
-          };
-          lastUpdateLive();
+            let lastUpdateDate = new Date(item.lastDownload * 1000);
+            let monthName = lastUpdateDate.toLocaleString(undefined,
+              {month: "short"});
+            let day = lastUpdateDate.getDate();
+            day = day < 10 ? "0" + day : day;
+            lastUpdateElement.textContent = day + " " + monthName + " " +
+              lastUpdateDate.getFullYear();
+          }
+          else if (sinceUpdate > hourInMs)
+          {
+            lastUpdateElement.textContent =
+              getMessage("options_filterList_hours");
+          }
+          else if (sinceUpdate > minuteInMs)
+          {
+            lastUpdateElement.textContent =
+              getMessage("options_filterList_minutes");
+          }
+          else
+          {
+            lastUpdateElement.textContent =
+              getMessage("options_filterList_now");
+          }
           element.classList.remove("show-message");
         }
       }
 
       let websiteElement = element.querySelector(".context-menu .website");
       if (websiteElement)
       {
         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 24 matching lines @@
   }
 
   collections.popular = new Collection([
     {
       id: "recommend-list-table"
     }
   ]);
   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
     }
   ]);
   collections.acceptableAds = new Collection([
     {
       id: "acceptableads-table"
     }
   ]);
   collections.custom = new Collection([
     {
       id: "custom-list-table"
     }
   ]);
   collections.whitelist = new Collection([
     {
       id: "whitelisting-table",
-      emptyText: "options_whitelisted_empty"
+      emptyText: ["options_whitelist_empty_1", "options_whitelist_empty_2"]
     }
   ]);
   collections.filterLists = new Collection([
     {
       id: "all-filter-lists-table",
       useOriginalTitle: true
     }
   ]);
 
   function toggleShowLanguage(subscription)
@@ skipping 47 matching lines @@
   function updateFilter(filter)
   {
     let match = filter.text.match(whitelistedDomainRegexp);
     if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
       collections.whitelist.addItem(filter);
     }
     else
     {
-      customFiltersArray.push(filter.text);
+      customFilters.push(filter.text);
       if (isCustomFiltersLoaded)

[Thomas Greiner, 2017/07/14 12:26:12]

Detail: What is this check for? It seems that `updateCustomFiltersUi()` isn't
doing anything that requires custom filters to be loaded.

[saroyanm, 2017/07/14 16:17:25]

Shouldn't we wait only for the time when all filters are loaded to update the
view ? Otherwise the view will be updated for each filter, on start. ex.: When
user have a lot of filters, this might have performance impact.

[Thomas Greiner, 2017/07/14 16:37:42]

Right, I remember and I see that this has actually already been there before.

Presumably, the most appropriate way would be to handle this within the
collection but let's leave it like this for now since it works.

[saroyanm, 2017/07/14 16:41:34]

I agree, I'll create a separate issue for that, when this review is resolved.

         updateCustomFiltersUi();
     }
 
     filtersMap[filter.text] = filter;
   }
 
   function removeCustomFilter(text)
   {
-    let index = customFiltersArray.indexOf(text);
+    let index = customFilters.indexOf(text);
     if (index >= 0)
-      customFiltersArray.splice(index, 1);

[Thomas Greiner, 2017/07/14 12:26:12]

What if there are multiple instances of the same filter?

[saroyanm, 2017/07/14 16:17:25]

There shouldn't be multiple instances of same filter AFAIK. Extension should not
allow having duplicated instances ?

[Thomas Greiner, 2017/07/14 16:37:42]

My thinking was that a pre-existing filter could already be duplicated even
before our UI has had the chance to check for that. But seeing that the current
options page also checks for duplicates and also doesn't consider this
circumstance, we should be safe if we simply ignore this.

[saroyanm, 2017/07/14 16:41:34]

Acknowledged.

+      customFilters.splice(index, 1);
 
     updateCustomFiltersUi();
   }
 
   function updateCustomFiltersUi()
   {
     let customFiltersListElement = E("custom-filters-raw");
-    customFiltersListElement.value = "";

[Thomas Greiner, 2017/07/14 12:26:12]

Detail: This is redundant because the line below will have the same effect if
`customFiltersArray` is empty.

[saroyanm, 2017/07/14 16:17:24]

Thanks for noticing. Will update.

[saroyanm, 2017/07/14 17:11:06]

Done.

-    customFiltersListElement.value = customFiltersArray.join("\n");
+    customFiltersListElement.value = customFilters.join("\n");
   }
 
   function loadRecommendations()
   {
     fetch("subscriptions.xml")
       .then((response) =>
       {
         return response.text();
       })
       .then((text) =>
@@ skipping 66 matching lines @@
       else
         location.href = link;
     });
   }
 
   function switchTab(id)
   {
     location.hash = id;
   }
 
-  function execAction(action, element, event)
+  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":
-        updateCustomFiltersUi();
         setCustomFiltersView("read");
-        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":
         setCustomFiltersView("write");
-        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-context-menu": {
         let listItem = findParentData(element, "access", true);
         if (listItem && !listItem.classList.contains("show-context-menu"))
@@ skipping 71 matching lines @@
         ext.backgroundPage.sendMessage({
           type: "subscriptions.update",
           url: findParentData(element, "access", false)
         });
         break;
     }
   }
 
   function setCustomFiltersView(mode)
   {
-    let customFilters = E("custom-filters");
-    let customFiltersListElement = E("custom-filters-raw");
+    let customFiltersElement = E("custom-filters-raw");
+    updateCustomFiltersUi();
     if (mode == "read")
     {
-      customFiltersListElement.disabled = true;
-      if (!customFiltersListElement.value)
+      customFiltersElement.disabled = true;
+      if (!customFiltersElement.value)
       {
         setCustomFiltersView("empty");
         return;
       }
     }
     else if (mode == "write")
     {
-      updateCustomFiltersUi();

[Thomas Greiner, 2017/07/14 12:26:13]

Interesting that you're calling `updateCustomFiltersUi()` only for the mode
"write". Is there a reason why you're not calling it unconditionally to ensure
that the list will always be up-to-date whenever switching its mode?

[saroyanm, 2017/07/14 16:17:25]

You are right, I'm not sure why I did this.. I'll fix this

[saroyanm, 2017/07/14 17:11:06]

Done.

-      customFiltersListElement.disabled = false;
-    }
-
-    customFilters.dataset.mode = mode;
+      customFiltersElement.disabled = false;
+    }
+
+    E("custom-filters").dataset.mode = mode;
   }
 
   function onClick(e)
   {
     let context = document.querySelector(".show-context-menu");
     if (context)
       context.classList.remove("show-context-menu");
 
     let actions = findParentData(e.target, "action", false);
     if (!actions)
       return;
 
     actions = actions.split(",");
     for (let action of actions)
     {
-      execAction(action, e.target, e);
+      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 26 matching lines @@
     {
       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, e);
+      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 66 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);
-    E("whitelisting-textbox").addEventListener("keypress", (e) =>
-    {
-      if (getKey(e) == "Enter")
-        addWhitelistedDomain();
+    let exampleValue = getMessage("options_whitelist_placeholder_example",
+      ["www.example.com"]);
+    E("whitelisting-textbox").setAttribute("placeholder", exampleValue);
+    E("whitelisting-textbox").addEventListener("keyup", (e) =>
+    {
+      E("whitelisting-add-button").disabled = !e.target.value;
     }, false);
 
     // Advanced tab
     let customize = document.querySelectorAll("#customize li[data-pref]");
     customize = Array.prototype.map.call(customize, (checkbox) =>
     {
       return checkbox.getAttribute("data-pref");
     });
     for (let key of customize)
     {
@@ skipping 148 matching lines @@
         ext.backgroundPage.sendMessage({
           type: "filters.get",
           subscriptionUrl: subscription.url
         },
         (filters) =>
         {
           for (let filter of filters)
             updateFilter(filter);
 
           isCustomFiltersLoaded = true;
-          updateCustomFiltersUi();
           setCustomFiltersView("read");
         });
       }
     });
     loadRecommendations();
     ext.backgroundPage.sendMessage({
       type: "prefs.get",
       key: "subscriptions_exceptionsurl"
     },
     (url) =>
@@ skipping 13 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 = "";
-    document.querySelector("#whitelisting .controls")
-      .classList.remove("mode-edit");
+    E("whitelisting-add-button").disabled = true;
   }
 
   function addEnableSubscription(url, title, homepage)
   {
     let messageType = null;
     let knownSubscription = subscriptionsMap[url];
     if (knownSubscription && knownSubscription.disabled == true)
       messageType = "subscriptions.toggle";
     else
       messageType = "subscriptions.add";
 
     let message = {
       type: messageType,
       url
     };
     if (title)
       message.title = title;
     if (homepage)
       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];
         if (whitelistedDomainRegexp.test(knownFilter.text))
           collections.whitelist.removeItem(knownFilter);
@@ skipping 263 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);
 }