Issue 2409 - Improved accessibility of tabs in options page

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -470,6 +470,22 @@
     });
   }
 
+  function openDocLink(id)
+  {
+    getDocLink(id, function(link)
+    {
+      if (id == "share-general")
+        openSharePopup(link);
+      else
+        location.href = link;
+    });
+  }
+
+  function switchTab(id)
+  {
+    location.hash = id;
+  }
+
   function onClick(e)
   {
     var context = document.querySelector(".show-context-menu");
@@ -488,6 +504,7 @@
       element = element.parentElement;
     }
 
+    var element = findParentData(e.target, "action", true);
     var actions = element.getAttribute("data-action").split(",");
     for (var i = 0; i < actions.length; i++)
     {
@@ -527,7 +544,12 @@
           closeDialog();
           break;
         case "open-dialog":
-          openDialog(element.getAttribute("data-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(
@@ -542,8 +564,8 @@
           });
           break;
         case "switch-tab":
-          document.body.setAttribute("data-tab",
-            element.getAttribute("data-tab"));
+          var tabId = findParentData(e.target, "tab", false);
+          switchTab(tabId);
           break;
         case "toggle-pref":
           ext.backgroundPage.sendMessage(
@@ -611,6 +633,122 @@
     }
   }
 
+  function getKey(e)
+  {
+    // e.keyCode has been deprecated so we attempt to use e.key
+    if ("key" in e)
+      return e.key;
+    return getKey.keys[e.keyCode];
+  }
+  getKey.keys = {
+    9: "Tab",
+    13: "Enter",
+    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.

+    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 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);
+      if (!tabContent)
+        break;
+    }
+  }
+
   function onDOMLoaded()
   {
     populateLists();
@@ -623,19 +761,6 @@
         searchStyle.innerHTML = "#all-lang-table li:not([data-search*=\"" + this.value.toLowerCase() + "\"]) { display: none; }";
     }
 
-    function getKey(e)
-    {
-      // e.keyCode has been deprecated so we attempt to use e.key
-      if ("key" in e)
-        return e.key;
-      return getKey.keys[e.keyCode];
-    }
-    getKey.keys = {
-      9: "Tab",
-      13: "Enter",
-      27: "Escape"
-    };
-
     // Initialize navigation sidebar
     ext.backgroundPage.sendMessage(
     {
@@ -651,11 +776,6 @@
       E("link-version").setAttribute("href", link);
     });
 
-    getDocLink("contribute", function(link)
-    {
-      document.querySelector("#tab-contribute a").setAttribute("href", link);
-    });
-
     updateShareLink();
     updateTooltips();
 
@@ -748,6 +868,9 @@
           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;
@@ -1005,13 +1128,6 @@
       checkbox.setAttribute("aria-checked", value);
   }
 
-  function onShareLinkClick(e)
-  {
-    e.preventDefault();
-
-    getDocLink("share-general", openSharePopup);
-  }
-
   function updateShareLink()
   {
     var shareResources = [
@@ -1028,14 +1144,7 @@
       if (!--checksRemaining)
       {
         // Hide the share tab if a script on the share page would be blocked
-        var tab = E("tab-share");
-        if (isAnyBlocked)
-        {
-          tab.hidden = true;
-          tab.removeEventListener("click", onShareLinkClick, false);
-        }
-        else
-          tab.addEventListener("click", onShareLinkClick, false);
+        E("tab-share").hidden = isAnyBlocked;
       }
     }
 
@@ -1177,4 +1286,5 @@
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
+  window.addEventListener("hashchange", onHashChange, false);
 })();