Issue 2409 - Improved accessibility of tabs in options page

new-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 526 matching lines @@
         case "edit-domain-exception":
           document.querySelector("#whitelisting .controls").classList.add("mode-edit");
           E("whitelisting-textbox").focus();
           break;
         case "import-subscription":
           var url = E("blockingList-textbox").value;
           addEnableSubscription(url);
           closeDialog();
           break;
         case "open-dialog":
           var dialog = findParentData(element, "dialog", false);

[saroyanm, 2016/07/25 17:13:34]

Are you assigning element to the variable because of the readability reasons ?
Same question goes to the variables below "doclink", "tabId".

[Thomas Greiner, 2016/07/26 12:50:49]

Yes, that makes it easier to read than for instance:
openDocLink(findParentData(element, "dialog", false);

           openDialog(dialog);
           break;
         case "open-doclink":
           var doclink = findParentData(element, "doclink", false);
           openDocLink(doclink);
           break;
         case "save-custom-filters":
           sendMessageHandleErrors(
           {
             type: "filters.importRaw",
@@ skipping 88 matching lines @@
     27: "Escape",
     37: "ArrowLeft",
     38: "ArrowUp",
     39: "ArrowRight",
     40: "ArrowDown"
   };
 
   function onKeyUp(e)
   {
     var key = getKey(e);
-    var element = document.querySelector(":focus");

[saroyanm, 2016/07/25 17:13:34]

We probably should be able to use document.activeElement instead.

[Thomas Greiner, 2016/07/26 12:50:49]

You're absolutely right! Done.

+    var element = document.activeElement;
     if (!key || !element)
       return;
 
     var container = findParentData(element, "action", true);
     if (!container || !container.hasAttribute("data-keys"))
       return;
 
     var keys = container.getAttribute("data-keys").split(" ");
     if (keys.indexOf(key) < 0)
       return;
 
     switch (container.getAttribute("data-action"))
     {
       case "add-domain-exception":
         addWhitelistedDomain();
         break;
       case "open-doclink":
         var doclink = findParentData(element, "doclink", false);
         openDocLink(doclink);
         break;
       case "switch-tab":
         if (key == "Enter")
         {
           var tabId = findParentData(element, "tab", false);

[saroyanm, 2016/07/25 17:13:34]

If I don't miss something this can go outside of if/else case:
if (element.hasAttribute("aria-selected"))
{
...
}
switchTab(findParentData(element, "tab", false));

[Thomas Greiner, 2016/07/26 12:50:49]

`switchTab()` is only called if either Enter is pressed or a direction key is
pressed while a tab item is selected. In any other case nothing should happen.

           switchTab(tabId);
         }
         else if (element.hasAttribute("aria-selected"))
         {
           if (key == "ArrowLeft" || key == "ArrowUp")
           {
             element = element.previousElementSibling
                 || container.lastElementChild;
           }
           else if (key == "ArrowRight" || key == "ArrowDown")
           {
             element = element.nextElementSibling
                 || container.firstElementChild;
           }
 
           var tabId = findParentData(element, "tab", false);
           switchTab(tabId);
         }
         break;
     }
   }
 
+  function selectTabItem(tabId, container, focus)
+  {
+    // Show tab content
+    document.body.setAttribute("data-tab", tabId);
+
+    // Select tab
+    var tabList = container.querySelector("[role='tablist']");
+    if (!tabList)
+      return null;
+
+    var previousTab = tabList.querySelector("[aria-selected]");
+    previousTab.removeAttribute("aria-selected");
+    previousTab.setAttribute("tabindex", -1);
+
+    var tab = tabList.querySelector("li[data-tab='" + tabId + "']");
+    tab.setAttribute("aria-selected", true);
+    tab.setAttribute("tabindex", 0);
+
+    var tabContentId = tab.getAttribute("aria-controls");
+    var tabContent = document.getElementById(tabContentId);
+
+    // Select sub tabs
+    if (tab.hasAttribute("data-subtab"))
+      selectTabItem(tab.getAttribute("data-subtab"), tabContent, false);
+
+    if (tab && focus)
+      tab.focus();
+
+    return tabContent;
+  }
+
   function onHashChange()
   {
     var hash = location.hash.substr(1);
     if (!hash)
       return;
-
-    function selectTab(tabId, container, focus)

[saroyanm, 2016/07/25 17:13:34]

This nested function will always recreated on each hash change.

[Thomas Greiner, 2016/07/26 12:50:50]

Done.

I wanted to avoid that this function is used outside since it's not actually
switching to the tab but only selects the tab item. Therefore I renamed it to
`selectTabItem()` to make this a bit clearer so that I can move it outside.

-    {
-      // Show tab content
-      document.body.setAttribute("data-tab", tabId);
-
-      // Select tab
-      var tabList = container.querySelector("[role='tablist']");
-      if (!tabList)
-        return null;
-
-      var previousTab = tabList.querySelector("[aria-selected]");
-      previousTab.removeAttribute("aria-selected");
-      previousTab.setAttribute("tabindex", -1);
-
-      var tab = tabList.querySelector("li[data-tab='" + tabId + "']");
-      tab.setAttribute("aria-selected", true);
-      tab.setAttribute("tabindex", 0);
-
-      var tabContentId = tab.getAttribute("aria-controls");
-      var tabContent = document.getElementById(tabContentId);
-
-      // Select sub tabs
-      if (tab.hasAttribute("data-subtab"))
-        selectTab(tab.getAttribute("data-subtab"), tabContent, false);
-
-      if (tab && focus)
-        tab.focus();
-
-      return tabContent;
-    }
 
     // Select tab and parent tabs
     var tabIds = hash.split("-");
     var tabContent = document.body;
     for (var i = 0; i < tabIds.length; i++)
     {
       var tabId = tabIds.slice(0, i + 1).join("-");
-      tabContent = selectTab(tabId, tabContent, true);
+      tabContent = selectTabItem(tabId, tabContent, true);
       if (!tabContent)
         break;
     }
   }
 
   function onDOMLoaded()
   {
     populateLists();
     function onFindLanguageKeyUp()
     {
@@ skipping 17 matching lines @@
     getDocLink("releases", function(link)
     {
       E("link-version").setAttribute("href", link);
     });
 
     updateShareLink();
     updateTooltips();
 
     // Initialize interactive UI elements
     document.body.addEventListener("click", onClick, false);
+    document.body.addEventListener("keyup", onKeyUp, false);
     var placeholderValue  = getMessage("options_dialog_language_find");
     E("find-language").setAttribute("placeholder", placeholderValue);
     E("find-language").addEventListener("keyup", onFindLanguageKeyUp, false);
     E("whitelisting-textbox").addEventListener("keypress", function(e)
     {
       if (getKey(e) == "Enter")
         addWhitelistedDomain();
     }, false);
 
     // Advanced tab
@@ skipping 68 matching lines @@
           }
           else if (e.target.classList.contains("focus-last"))
           {
             e.preventDefault();
             this.querySelector(".focus-first").focus();
           }
           break;
       }
     }, false);
 
-    document.body.addEventListener("keyup", onKeyUp, true);

[saroyanm, 2016/07/25 17:13:34]

Detail: I think this listener can be defined with the other similar listeners
below, same place where "hashchange" listener defined.

[Thomas Greiner, 2016/07/26 12:50:49]

It's out of place here, I agree. I moved it to the spot where we also register
the global "click" listener and other UI listeners. Note that moving it to the
"hash" listener below means that we'd have to listen on `document` instead of
`document.body` since the DOM isn't initialized at this point yet.

     onHashChange();
   }
 
   var focusedBeforeDialog = null;
   function openDialog(name)
   {
     var dialog = E("dialog");
     dialog.setAttribute("aria-hidden", false);
     dialog.setAttribute("aria-labelledby", "dialog-title-" + name);
     document.body.setAttribute("data-dialog", name);
@@ skipping 398 matching lines @@
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
         "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 })();