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 92 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 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.

+          var focusableElement = element.parentElement;
+          while (focusableElement)
           {
-            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.

+            if (focusableElement.classList.contains("tab-content")
+                || focusableElement.classList.contains("dialog-content"))
               break;
 
-            tab = tab.parentElement;
+            focusableElement = focusableElement.parentElement;
           }
-          focusNextElement(tab || document, 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)
@@ skipping 734 matching lines @@
     filter: ["added", "loaded", "removed"]
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "removed", "title"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();