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 452 matching lines @@
   {
     ext.backgroundPage.sendMessage(message, function(errors)
     {
       if (errors.length > 0)
         alert(errors.join("\n"));
       else if (onSuccess)
         onSuccess();
     });
   }
 
+  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");
     if (context)
       context.classList.remove("show-context-menu");
 
     var element = e.target;
     while (true)
     {
       if (!element)
         return;
 
       if (element.hasAttribute("data-action"))
         break;
 
       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++)
     {
       switch (actions[i])
       {
         case "add-domain-exception":
           addWhitelistedDomain();
           break;
         case "add-predefined-subscription":
           var dialog = E("dialog-content-predefined");
@@ skipping 19 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":
-          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(
           {
             type: "filters.importRaw",
             text: E("custom-filters-raw").value,
             removeExisting: true
           },
           function()
           {
             E("custom-filters").classList.remove("mode-edit");
           });
           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(
           {
             type: "prefs.toggle",
             key: findParentData(element, "pref", false)
           });
           break;
         case "update-all-subscriptions":
           ext.backgroundPage.sendMessage(
@@ skipping 47 matching lines @@
           ext.backgroundPage.sendMessage(
           {
             type: "filters.remove",
             text: findParentData(element, "access", false)
           });
           break;
       }
     }
   }
 
+  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();
     function onFindLanguageKeyUp()
     {
       var searchStyle = E("search-style");
       if (!this.value)
         searchStyle.innerHTML = "";
       else
         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(
     {
       type: "app.get",
       what: "addonVersion"
     },
     function(addonVersion)
     {
       E("abp-version").textContent = addonVersion;
     });
     getDocLink("releases", function(link)
     {
       E("link-version").setAttribute("href", link);
     });
 
-    getDocLink("contribute", function(link)
-    {
-      document.querySelector("#tab-contribute a").setAttribute("href", link);
-    });
-
     updateShareLink();
     updateTooltips();
 
     // Initialize interactive UI elements
     document.body.addEventListener("click", onClick, 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)
     {
@@ skipping 72 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 237 matching lines @@
       case "safari_contentblocker":
         E("restart-safari").setAttribute("aria-hidden", value || initial);
         break;
     }
 
     var checkbox = document.querySelector("[data-pref='" + key + "'] button[role='checkbox']");
     if (checkbox)
       checkbox.setAttribute("aria-checked", value);
   }
 
-  function onShareLinkClick(e)
-  {
-    e.preventDefault();
-
-    getDocLink("share-general", openSharePopup);
-  }
-
   function updateShareLink()
   {
     var shareResources = [
       "https://facebook.com/plugins/like.php?",
       "https://platform.twitter.com/widgets/",
       "https://apis.google.com/se/0/_/+1/fastbutton?"
     ];
     var isAnyBlocked = false;
     var checksRemaining = shareResources.length;
 
     function onResult(isBlocked)
     {
       isAnyBlocked |= isBlocked;
       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;
       }
     }
 
     for (var i = 0; i < shareResources.length; i++)
       checkShareResource(shareResources[i], onResult);
   }
 
   function getMessages(id)
   {
     var messages = [];
@@ skipping 121 matching lines @@
         "shouldShowBlockElementMenu"]
   });
   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);
 })();