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
@@ skipping 52 matching lines @@
       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 listItem = document.createElement("li");
         listItem.appendChild(document.importNode(template.content, true));
         listItem.setAttribute("data-access", item.url || item.text);
 
         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);
+        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);
         }
 
         this._setEmpty(table, null);
@@ skipping 18 matching lines @@
     {
       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.

+          // Fall back to next focusable element within same tab or dialog
+          var focusableElement = element.parentElement;
+          while (focusableElement)
           {
-            if (tab.classList.contains("tab-content"))
+            if (focusableElement.classList.contains("tab-content")
+                || focusableElement.classList.contains("dialog-content"))
               break;
 
-            tab = tab.parentElement;
-            if (!tab)
-            {
-              tab = document;
-              break;
-            }
+            focusableElement = focusableElement.parentElement;
           }
-          focusNextElement(tab, control);
+          focusNextElement(focusableElement || document, 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;
@@ skipping 14 matching lines @@
       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.

+    var focusables = container.querySelectorAll("a, button, input, .control");
     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;
+    index += (index == focusables.length - 1) ? -1 : 1;
 
     var nextElement = focusables[index];
     if (!nextElement)
       return false;
 
     nextElement.focus();
     return true;
   }
 
   function onToggleSubscriptionClick(e)
@@ skipping 686 matching lines @@
     filter: ["added", "loaded", "removed"]
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "removed", "title"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();