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);
+        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);
         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 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 = [];
     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, 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",
         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);
 })();