Issue 2408 - Improved accessibility of checkboxes in options page

options.js

Index: options.js
===================================================================
--- a/options.js
+++ b/options.js
@@ -23,6 +23,7 @@
   var recommendationsMap = Object.create(null);
   var filtersMap = Object.create(null);
   var collections = Object.create(null);
+  var maxLabelId = 0;
 
   function Collection(details)
   {
@@ -64,19 +65,20 @@
       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);
@@ -84,6 +86,7 @@
           table.insertBefore(listItem, table.childNodes[this.items.indexOf(item)]);
         else
           table.appendChild(listItem);
+        this.updateItem(item);
       }
     }
     return length;
@@ -100,12 +103,57 @@
     {
       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 = [];
@@ -119,11 +167,30 @@
     }
   };
 
+  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",
@@ -220,30 +287,21 @@
       {
         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);
             }
           }
         }