Issue 2408 - Improved accessibility of checkboxes in options page

options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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/>.
  */
 
 "use strict";
 
 (function()
 {
   var subscriptionsMap = Object.create(null);
   var recommendationsMap = Object.create(null);
   var filtersMap = Object.create(null);
   var collections = Object.create(null);
+  var maxLabelId = 0;
 
   function Collection(details)
   {
     this.details = details;
     this.items = [];
   }
 
   Collection.prototype._setEmpty = function(table, text)
   {
     var placeholder = table.querySelector(".empty-placeholder");
@@ skipping 21 matching lines @@
       return aValue.localeCompare(bValue);
     });
 
     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++) 
       {
         var item = arguments[i];
-        var text = item.title || item.url || item.text;
         var listItem = document.createElement("li");
         listItem.appendChild(document.importNode(template.content, true));
         listItem.setAttribute("data-access", item.url || item.text);
-        listItem.querySelector(".display").textContent = text;
-        if (text)
-          listItem.setAttribute("data-search", text.toLowerCase());
 
+        var labelId = "label-" + (++maxLabelId);
+        var label = listItem.querySelector(".display");

[saroyanm, 2016/01/19 11:19:50]

Nit: 
we can have two lines merged:
listItem.querySelector(".display").setAttribute("id", labelId);

[Thomas Greiner, 2016/01/19 15:15:04]

Done.

+        label.setAttribute("id", labelId);
         var control = listItem.querySelector(".control");
         if (control)
         {
+          // We use aria-labelledby to avoid triggering the control when
+          // interacting with the label
+          control.setAttribute("aria-labelledby", labelId);
           control.addEventListener("click", this.details[j].onClick, false);
-          control.checked = item.disabled == false;
         }
 
         this._setEmpty(table, null);
         if (table.hasChildNodes())
           table.insertBefore(listItem, table.childNodes[this.items.indexOf(item)]);
         else
           table.appendChild(listItem);
+        this.updateItem(item);
       }
     }
     return length;
   };
 
   Collection.prototype.removeItem = function(item)
   {
     var index = this.items.indexOf(item);
     if (index == -1)
       return;
 
     this.items.splice(index, 1);
     for (var i = 0; i < this.details.length; i++)
     {
       var table = E(this.details[i].id);
       var element = table.childNodes[index];
+
+      // Element gets removed so make sure to handle focus appropriately
+      var control = element.querySelector(".control");
+      if (control && control == document.activeElement)
+      {
+        if (!focusNextElement(element.parentElement, control))
+        {
+          // Fall back to next focusable element within same tab
+          var tab = element.parentElement;
+          while (true)

[saroyanm, 2016/01/19 11:19:49]

What about ?
while (tab.tagName == "BODY") ?

In that case you can remove "if (!tab)" part

[Thomas Greiner, 2016/01/19 15:15:04]

Done. Checking for the element type should be the last resort but I was able to
move the exit condition into the while-statement.

[saroyanm, 2016/01/25 14:45:43]

you can also check for the element itself "document.body != tab"
Anyway I see you currently went with another approach, which does make more
sense.

+          {
+            if (tab.classList.contains("tab-content"))
+              break;
+
+            tab = tab.parentElement;
+            if (!tab)
+            {
+              tab = document;
+              break;
+            }
+          }
+          focusNextElement(tab, control);
+        }
+      }
+
       element.parentElement.removeChild(element);
       if (this.items.length == 0)
         this._setEmpty(table, this.details[i].emptyText);
     }
   };
 
+  Collection.prototype.updateItem = function(item)

[saroyanm, 2016/01/19 11:19:50]

I think it make sense to have second optional argument where we can send the
listItem. 
In that case we will not need to query for the element when called from
"addItems" method, this should reduce the query of the listItem which we already
have.
ex.: updateItem(subscription, listElement)

[Thomas Greiner, 2016/01/19 15:15:04]

But an item can be associated with multiple list elements so updating just one
of them will lead to syncing issues.

+  {
+    var access = (item.url || item.text).replace(/'/g, "\\'");
+    for (var i = 0; i < this.details.length; i++)
+    {
+      var table = E(this.details[i].id);
+      var element = table.querySelector("[data-access='" + access + "']");
+      if (!element)
+        continue;
+
+      var text = item.title || item.url || item.text;
+      element.querySelector(".display").textContent = text;
+      if (text)
+        element.setAttribute("data-search", text.toLowerCase());
+      var control = element.querySelector(".control[role='checkbox']");
+      if (control)
+        control.setAttribute("aria-checked", item.disabled == false);
+    }
+  };
+
   Collection.prototype.clearAll = function()
   {
     this.items = [];
     for (var i = 0; i < this.details.length; i++)
     {
       var table = E(this.details[i].id);
       var template = table.querySelector("template");
       table.innerHTML = "";
       table.appendChild(template);
       this._setEmpty(table, this.details[i].emptyText);
     }
   };
 
+  function focusNextElement(container, currentElement)
+  {
+    var focusables = container.querySelectorAll("a, button, .control");

[saroyanm, 2016/01/19 11:19:50]

What about have a focus for input elements as well ? I can see the only place we
will need it is in "Add other website language" section, which is unlikely that
user will add all languages, so it's not really priority of having that change,
but for consistency reasons we can have it IMO.

[Thomas Greiner, 2016/01/19 15:15:04]

Done.

+    focusables = Array.prototype.slice.call(focusables);
+    var index = focusables.indexOf(currentElement);
+    if (index + 1 < focusables.length)

[saroyanm, 2016/01/19 11:19:49]

I think we can write this one line, something like:
index == focusables.length - 1 ? index-- : index++;

[Thomas Greiner, 2016/01/19 15:15:04]

Done.

+      index += 1;
+    else if (index < focusables.length)
+      index -= 1;
+
+    var nextElement = focusables[index];
+    if (!nextElement)
+      return false;
+
+    nextElement.focus();
+    return true;
+  }
+
   function onToggleSubscriptionClick(e)
   {
     e.preventDefault();
-    var subscriptionUrl = e.target.parentNode.getAttribute("data-access");
-    if (!e.target.checked)
+    var checkbox = e.target;
+    var subscriptionUrl = checkbox.parentElement.getAttribute("data-access");
+    if (checkbox.getAttribute("aria-checked") == "true")
     {
       ext.backgroundPage.sendMessage({
         type: "subscriptions.remove",
         url: subscriptionUrl
       });
     }
     else
       addEnableSubscription(subscriptionUrl);
   }
 
@@ skipping 76 matching lines @@
     var subscriptionUrl = subscription.url;
     var knownSubscription = subscriptionsMap[subscriptionUrl];
     if (knownSubscription)
       knownSubscription.disabled = subscription.disabled;
     else
     {
       getAcceptableAdsURL(function(acceptableAdsUrl)
       {
         function onObjectChanged()
         {
-          var access = (subscriptionUrl || subscription.text).replace(/'/g, "\\'");
-          var elements = document.querySelectorAll("[data-access='" + access + "']");
-          for (var i = 0; i < elements.length; i++)
+          for (var i in collections)
+            collections[i].updateItem(subscription);
+
+          var recommendation = recommendationsMap[subscriptionUrl];
+          if (recommendation && recommendation.type == "ads")
           {
-            var element = elements[i];
-            var control = element.querySelector(".control");
-            if (control.localName == "input")
-              control.checked = subscription.disabled == false;
-            if (subscriptionUrl in recommendationsMap)
+            if (subscription.disabled == false)
             {
-              var recommendation = recommendationsMap[subscriptionUrl];
-              if (recommendation.type == "ads")
-              {
-                if (subscription.disabled == false)
-                {
-                  collections.allLangs.removeItem(subscription);
-                  collections.langs.addItems(subscription);
-                }
-                else
-                {
-                  collections.allLangs.addItems(subscription);
-                  collections.langs.removeItem(subscription);
-                }
-              }
+              collections.allLangs.removeItem(subscription);
+              collections.langs.addItems(subscription);
+            }
+            else
+            {
+              collections.allLangs.addItems(subscription);
+              collections.langs.removeItem(subscription);
             }
           }
         }
 
         if (!Object.observe)
         {
           // Currently only "disabled" property of subscription used for observation
           // but with Advanced tab implementation we should also add more properties.
           ["disabled"].forEach(function(property)
           {
@@ skipping 560 matching lines @@
     filter: ["added", "loaded", "removed"]
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "removed", "title"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();