Issue 2409 - Improved accessibility of tabs in options page

This review consists of the following changes:
- Added metadata to tabs.
- Added footer items to tab order.
- Added "open-doclink" action.
- Added centralized "keyup" handler.
- Don't use separate HTML element for icons in tab items.
- Use page hash to determine open tab.
- In `updateShareLink()`: No need to remove event listener since this was moved to centralized "click" listener.
- Instead of removing the outline when tab item is focused, use a text-shadow to subtly indicate that it is focused.

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

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:547: var dialog = findParentData(element, "dialog", false);
Are you assigning element to the variable because of the readability reasons ?
Same question goes to the variables below "doclink", "tabId".

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:656: var element = document.querySelector(":focus");
We probably should be able to use document.activeElement instead.

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:680: var tabId = findParentData(element, "tab", false);
If I don't miss something this can go outside of if/else case:
if (element.hasAttribute("aria-selected"))
{
...
}
switchTab(findParentData(element, "tab", false));

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:709: function selectTab(tabId, container, focus)
This nested function will always recreated on each hash change.

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:872: document.body.addEventListener("keyup", onKeyUp, true);
Detail: I think this listener can be defined with the other similar listeners
below, same place where "hashchange" listener defined.

https://codereview.adblockplus.org/29348642/diff/29348643/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29348642/diff/29348643/skin/new-options.cs...
skin/new-options.css:222: .tabs li[role="tab"]:focus
Side note: This feels to be distracting for eyes, but maybe it's my personal
opinion, I assume that this probably will be changed with other design changes
that Aaron planing to suggest.

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

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.html#ne...
new-options.html:62: <a id="link-version" tabindex="0">
Turns out to not be necessary since we're setting the "href" attribute through
JavaScript which makes it focusable.

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:547: var dialog = findParentData(element, "dialog", false);
On 2016/07/25 17:13:34, saroyanm wrote:
> Are you assigning element to the variable because of the readability reasons ?
> Same question goes to the variables below "doclink", "tabId".

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

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:656: var element = document.querySelector(":focus");
On 2016/07/25 17:13:34, saroyanm wrote:
> We probably should be able to use document.activeElement instead.

You're absolutely right! Done.

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:680: var tabId = findParentData(element, "tab", false);
On 2016/07/25 17:13:34, saroyanm wrote:
> If I don't miss something this can go outside of if/else case:
> if (element.hasAttribute("aria-selected"))
> {
> ...
> }
> switchTab(findParentData(element, "tab", false));

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

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:709: function selectTab(tabId, container, focus)
On 2016/07/25 17:13:34, saroyanm wrote:
> This nested function will always recreated on each hash change.

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.

https://codereview.adblockplus.org/29348642/diff/29348643/new-options.js#newc...
new-options.js:872: document.body.addEventListener("keyup", onKeyUp, true);
On 2016/07/25 17:13:34, saroyanm wrote:
> Detail: I think this listener can be defined with the other similar listeners
> below, same place where "hashchange" listener defined.

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.

https://codereview.adblockplus.org/29348642/diff/29348643/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29348642/diff/29348643/skin/new-options.cs...
skin/new-options.css:222: .tabs li[role="tab"]:focus
On 2016/07/25 17:13:34, saroyanm wrote:
> Side note: This feels to be distracting for eyes, but maybe it's my personal
> opinion, I assume that this probably will be changed with other design changes
> that Aaron planing to suggest.

I don't know. We have yet to discuss what the scope of those design changes is
going to be. Let me know if you got a different idea for highlighting the
focused tab.

[saroyanm, 2016-07-26 15:44:38]

LGTM

https://codereview.adblockplus.org/29348642/diff/29348643/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29348642/diff/29348643/skin/new-options.cs...
skin/new-options.css:222: .tabs li[role="tab"]:focus
On 2016/07/26 12:50:50, Thomas Greiner wrote:
> On 2016/07/25 17:13:34, saroyanm wrote:
> > Side note: This feels to be distracting for eyes, but maybe it's my personal
> > opinion, I assume that this probably will be changed with other design
changes
> > that Aaron planing to suggest.
> 
> I don't know. We have yet to discuss what the scope of those design changes is
> going to be. Let me know if you got a different idea for highlighting the
> focused tab.

I think it's fine for now, we can tackle that separately with designers I think
during the design improvements.