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 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 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)
@@ skipping 42 matching lines @@
       if (b.url == acceptableAdsUrl)
         return -1;
 
       let aTitle = this._getItemTitle(a, 0).toLowerCase();
       let bTitle = this._getItemTitle(b, 0).toLowerCase();
       return aTitle.localeCompare(bTitle);
     });
 
     for (let j = 0; j < this.details.length; j++)
     {
-      let table = E(this.details[j].id);
+      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");
 
       let label = listItem.querySelector(".display");
       if (item.recommended && label.hasAttribute("data-tooltip"))
       {
         let tooltipId = label.getAttribute("data-tooltip");
         tooltipId = tooltipId.replace("%value%", item.recommended);
         label.setAttribute("data-tooltip", tooltipId);
       }
 
       for (let control of listItem.querySelectorAll(".control"))
       {
         if (control.hasAttribute("title"))
         {
           let titleValue = getMessage(control.getAttribute("title"));
           control.setAttribute("title", titleValue);
         }
       }
 
       this._setEmpty(table, null);
-      if (table.hasChildNodes())
-      {
-        table.insertBefore(listItem,
-            table.childNodes[this.items.indexOf(item)]);
-      }
+      if (table.children.length > 0)
+        table.insertBefore(listItem, table.children[this.items.indexOf(item)]);
       else
         table.appendChild(listItem);
+
       this.updateItem(item);
     }
     return length;
   };
 
   Collection.prototype.removeItem = function(item)
   {
     let index = this.items.indexOf(item);
     if (index == -1)
       return;
@@ skipping 47 matching lines @@
       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);
       }
 
-      let dateElement = element.querySelector(".date");
-      let timeElement = element.querySelector(".time");
-      if (dateElement && timeElement)
+      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 dateTime = i18nFormatDateTime(item.lastDownload * 1000);
-          dateElement.textContent = dateTime[0];
-          timeElement.textContent = dateTime[1];
+          let lastUpdate = item.lastDownload * 1000;
+          let lastUpdateLive = function()
+          {
+            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();
           element.classList.remove("show-message");
         }
       }
 
       let websiteElement = element.querySelector(".context-menu .website");
       if (websiteElement)
       {
         if (item.homepage)
           websiteElement.setAttribute("href", item.homepage);
         else
@@ skipping 70 matching lines @@
     {
       id: "custom-list-table"
     }
   ]);
   collections.whitelist = new Collection([
     {
       id: "whitelisting-table",
       emptyText: "options_whitelisted_empty"
     }
   ]);
-  collections.customFilters = new Collection([
-    {
-      id: "custom-filters-table",
-      emptyText: "options_customFilters_empty"
-    }
-  ]);
   collections.filterLists = new Collection([
     {
       id: "all-filter-lists-table",
       useOriginalTitle: true
     }
   ]);
 
   function toggleShowLanguage(subscription)
   {
     if (subscription.recommended == "ads")
@@ skipping 37 matching lines @@
   function updateSubscription(subscription)
   {
     for (let name in collections)
       collections[name].updateItem(subscription);
 
     toggleShowLanguage(subscription);
   }
 
   function updateFilter(filter)
   {
-    let match = filter.text.match(/^@@\|\|([^/:]+)\^\$document$/);
+    let match = filter.text.match(whitelistedDomainRegexp);
     if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
       collections.whitelist.addItem(filter);
     }
     else
-      collections.customFilters.addItem(filter);
+    {
+      customFiltersArray.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);
+    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.

+
+    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");
+  }
+
   function loadRecommendations()
   {
     fetch("subscriptions.xml")
       .then((response) =>
       {
         return response.text();
       })
       .then((text) =>
       {
         let doc = new DOMParser().parseFromString(text, "application/xml");
@@ skipping 64 matching lines @@
       else
         location.href = link;
     });
   }
 
   function switchTab(id)
   {
     location.hash = id;
   }
 
-  function execAction(action, element)
+  function execAction(action, element, event)
   {
     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");
+        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":
-        editCustomFilters();
+        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();
@@ skipping 28 matching lines @@
         });
         break;
       case "save-custom-filters":
         sendMessageHandleErrors({
           type: "filters.importRaw",
           text: E("custom-filters-raw").value,
           removeExisting: true
         },
         () =>
         {
-          E("custom-filters").classList.remove("mode-edit");
+          setCustomFiltersView("read");
         });
         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,
@@ skipping 25 matching lines @@
         break;
       case "update-subscription":
         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");
+    if (mode == "read")
+    {
+      customFiltersListElement.disabled = true;
+      if (!customFiltersListElement.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;
+  }
+
   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);
+      execAction(action, e.target, e);
     }
   }
 
   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);
+      execAction(action, element, e);
     }
   }
 
   function selectTabItem(tabId, container, focus)
   {
     // Show tab content
     document.body.setAttribute("data-tab", tabId);
 
     // Select tab
     let tabList = container.querySelector("[role='tablist']");
     if (!tabList)
       return null;
 
     let previousTab = tabList.querySelector("[aria-selected]");
     previousTab.removeAttribute("aria-selected");
     previousTab.setAttribute("tabindex", -1);
 
     let tab = tabList.querySelector("li[data-tab='" + tabId + "']");
     tab.setAttribute("aria-selected", true);
     tab.setAttribute("tabindex", 0);
 
     let tabContentId = tab.getAttribute("aria-controls");
     let tabContent = document.getElementById(tabContentId);
 
-    // Select sub tabs
-    if (tab.hasAttribute("data-subtab"))
-      selectTabItem(tab.getAttribute("data-subtab"), tabContent, false);
-
     if (tab && focus)
       tab.focus();
 
     return tabContent;
   }
 
   function onHashChange()
   {
     let hash = location.hash.substr(1);
     if (!hash)
@@ skipping 49 matching lines @@
     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();
     }, false);
 
     // Advanced tab
-    let tweaks = document.querySelectorAll("#tweaks li[data-pref]");
-    tweaks = Array.prototype.map.call(tweaks, (checkbox) =>
+    let customize = document.querySelectorAll("#customize li[data-pref]");
+    customize = Array.prototype.map.call(customize, (checkbox) =>
     {
       return checkbox.getAttribute("data-pref");
     });
-    for (let key of tweaks)
+    for (let key of customize)
     {
       getPref(key, (value) =>
       {
         onPrefMessage(key, value, true);
       });
     }
     ext.backgroundPage.sendMessage({
       type: "app.get",
       what: "features"
     },
     (features) =>
     {
       hidePref("show_devtools_panel", !features.devToolsPanel);
     });
 
-    let filterTextbox = document.querySelector("#custom-filters-add input");
-    placeholderValue = getMessage("options_customFilters_textbox_placeholder");
-    filterTextbox.setAttribute("placeholder", placeholderValue);
-    function addCustomFilters()
+    getDocLink("filterdoc", (link) =>
     {
-      let filterText = filterTextbox.value;
-      sendMessageHandleErrors({
-        type: "filters.add",
-        text: filterText
-      },
-      () =>
-      {
-        filterTextbox.value = "";
-      });
-    }
-    E("custom-filters-add").addEventListener("submit", (e) =>
+      E("link-filters").setAttribute("href", link);
+    });
+
+    getDocLink("subscriptions", (link) =>
     {
-      e.preventDefault();
-      addCustomFilters();
-    }, false);
+      setLinks("filter-lists-description", link);
+    });
+
+    E("custom-filters-raw").setAttribute("placeholder",
+      getMessage("options_customFilters_edit_placeholder", ["/ads/track/*"]));
 
     // Help tab
     getDocLink("faq", (link) =>
     {
       E("link-faq").setAttribute("href", link);
     });
     getDocLink("social_twitter", (link) =>
     {
       E("link-twitter").setAttribute("href", link);
     });
@@ skipping 112 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 25 matching lines @@
         type: "filters.add",
         text: "@@||" + domain.value.toLowerCase() + "^$document"
       });
     }
 
     domain.value = "";
     document.querySelector("#whitelisting .controls")
       .classList.remove("mode-edit");
   }
 
-  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];
     if (knownSubscription && knownSubscription.disabled == true)
       messageType = "subscriptions.toggle";
     else
       messageType = "subscriptions.add";
 
     let message = {
@@ skipping 14 matching lines @@
     {
       case "added":
         updateFilter(filter);
         updateShareLink();
         break;
       case "loaded":
         populateLists();
         break;
       case "removed":
         let knownFilter = filtersMap[filter.text];
-        collections.whitelist.removeItem(knownFilter);
-        collections.customFilters.removeItem(knownFilter);
+        if (whitelistedDomainRegexp.test(knownFilter.text))
+          collections.whitelist.removeItem(knownFilter);
+        else
+          removeCustomFilter(filter.text);
+
         delete filtersMap[filter.text];
         updateShareLink();
         break;
     }
   }
 
   function onSubscriptionMessage(action, subscription)
   {
     if (subscription.url in subscriptionsMap)
     {
@@ skipping 250 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);
 }