Issue 3885 - Disable the checkbox to toggle the Acceptable Ads list on the options page

options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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
@@ skipping 45 matching lines @@
 
   Collection.prototype._createElementQuery = function(item)
   {
     var access = (item.url || item.text).replace(/'/g, "\\'");
     return function(container)
     {
       return container.querySelector("[data-access='" + access + "']");
     };
   };
 
-  Collection.prototype._isControlDisabled = function(item, i)
-  {
-    return item.url == acceptableAdsUrl &&
-           this.details[i].onClick == toggleDisableSubscription;
-  };
-
   Collection.prototype._getItemTitle = function(item, i)
   {
     var title = null;
     if (this.details[i].useOriginalTitle)
       title = item.originalTitle;
     if (!title)
       title = item.title || item.url || item.text;
     return title;
   };
 
   Collection.prototype.addItems = function() 
   {
     var length = Array.prototype.push.apply(this.items, arguments);
     if (length == 0)
       return;
 
     this.items.sort(function(a, b)
     {
-      var aDisabled = this._isControlDisabled(a, 0);
-      var bDisabled = this._isControlDisabled(b, 0);

[Thomas Greiner, 2016/04/07 17:21:15]

It's a nice idea to group together all disabled checkboxes but it's not ideal
that we have to hardcode the table index since this special ordering is only
useful for Acceptable Ads and only for this one table. That is especially since
we intend to get rid of disabled checkboxes later on. What about instead
checking specifically for the whitelist here to ensure that it's at the end of
the list?

e.g.
this.items.sort(function(a, b)
{
  if (a.url == acceptableAdsUrl)
    return 1;
  
  ...
});

[Sebastian Noack, 2016/04/08 14:22:17]

Yeah, hard-coding the index isn't great, but we have to do so for
_getItemTitle() anyway. And the way I did it it was more obvious that we are
grouping disabled/enabled items respectively. Now it need a comment to explain
what that is about. But fair enough. Done.

-      if (aDisabled != bDisabled)
-        return aDisabled - bDisabled;
+      // 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;
 
       var aTitle = this._getItemTitle(a, 0).toLowerCase();

[Thomas Greiner, 2016/04/07 17:21:15]

Detail: Hardcoding the table index here might cause items to be not
alphabetically ordered. However, I assume that fixing this will require major
changes in the way we keep the UI in sync with the data so feel free to ignore
it for this review.

[Sebastian Noack, 2016/04/08 14:22:17]

I know. This is kinda a hack. But at least more correct than previously.

       var bTitle = this._getItemTitle(b, 0).toLowerCase();
       return aTitle.localeCompare(bTitle);
     }.bind(this));
 
     for (var j = 0; j < this.details.length; j++) 
     {
       var table = E(this.details[j].id);
       var template = table.querySelector("template");
       for (var i = 0; i < arguments.length; i++) 
       {
@@ skipping 85 matching lines @@
         continue;
 
       var title = this._getItemTitle(item, i);
       element.querySelector(".display").textContent = title;
       if (title)
         element.setAttribute("data-search", title.toLowerCase());
       var control = element.querySelector(".control[role='checkbox']");
       if (control)
       {
         control.setAttribute("aria-checked", item.disabled == false);
-        if (this._isControlDisabled(item, i))
+        if (item.url == acceptableAdsUrl && this.details[i].onClick ==
+                                            toggleDisableSubscription)
           control.setAttribute("disabled", true);
       }
 
       var dateElement = element.querySelector(".date");
       var timeElement = element.querySelector(".time");
       if (dateElement && timeElement)
       {
         var message = element.querySelector(".message");
         if (item.isDownloading)
         {
@@ skipping 942 matching lines @@
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
         "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();