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,19 @@
       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);
+        listItem.querySelector(".display").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 +85,7 @@
           table.insertBefore(listItem, table.childNodes[this.items.indexOf(item)]);
         else
           table.appendChild(listItem);
+        this.updateItem(item);
       }
     }
     return length;
@@ -100,12 +102,53 @@
     {
       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 or dialog
+          var tab = element.parentElement;

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

Detail: This is not only tab already, so maybe make sense to rename it to
something more generic. ex.: focusContainer 

[Thomas Greiner, 2016/01/25 18:04:21]

Done.

+          while (tab)

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

What if the dialog where the collection is implemented doesn't contain any other
focusable elements, should the close button be selected ? 

[Thomas Greiner, 2016/01/25 18:04:21]

The main challenge with focus management is that you can make it as extensive as
you want but the more you do that the more complicated it gets. Therefore I
decided to go with the most minimal implementation that's necessary for our UI.

At the moment there's always a focusable element in the dialog (see
"default-focus" class name) which is why this problem only exists in theory.

[saroyanm, 2016/01/26 18:40:12]

Fare enough.

+          {
+            if (tab.classList.contains("tab-content")
+                || tab.classList.contains("dialog-content"))

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

I think we need to make it obvious whether it's something related to
removeItem() or separate function, because currently feel like we try to fix
what focusNextElement do not capable, so I'll suggest to implement whole
functionality here without separate function, or make the existing function more
complex not to list all the focusablecontainers that are ancestors of current
element on current line.
Here is how I imagine the function behavior:
* receive element
* find ancestor that need to be so called "focusContainer"
* if "focusContainer" doesn't contain focusable children go up recursively until
reached the top.

We can also introduce focusableContainer class or data attribute to make this
manual trigger more manageable.

[Thomas Greiner, 2016/01/25 18:04:21]

We could put this into a separate function that's called by MutationObserver but
I wonder whether it's worth setting up an observer for this single case that's
only triggered in this exact spot.
Unfortunately, it's not as easy as recursing up the DOM to the top element
because usually, it makes more sense that the focus shifts to a sibling element
rather than the parent. That's what I meant with my previous comment when I said
that it can get complicated real fast.

+              break;
+
+            tab = tab.parentElement;
+          }
+          focusNextElement(tab || document, control);
+        }
+      }
+
       element.parentElement.removeChild(element);
       if (this.items.length == 0)
         this._setEmpty(table, this.details[i].emptyText);
     }
   };
 
+  Collection.prototype.updateItem = function(item)
+  {
+    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 +162,27 @@
     }
   };
 
+  function focusNextElement(container, currentElement)
+  {
+    var focusables = container.querySelectorAll("a, button, input, .control");
+    focusables = Array.prototype.slice.call(focusables);
+    var index = focusables.indexOf(currentElement);
+    index += (index == focusables.length - 1) ? -1 : 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 +279,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);
             }
           }
         }