Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DP-6198: moving work from broken branch to new #488

Merged
merged 19 commits into from
Apr 16, 2019

Conversation

clair0917
Copy link
Contributor

Any PRs being created needs a changelog.txt file before being merged into dev. See: Change Log Instructions

Description

Updates tabbing order and sets keyboard open/close for utility menu

Related Issue / Ticket

https://jira.mass.gov/browse/DP-6198

Steps to Test

From any page with a header, tab around in the navigation.

  1. The utility nav options are now reachable by tab, opening and closing as expected
  2. Tab through the main nav items, they tabbing now catches all items
  3. From a menu item, click enter, spacebar, or an up or down arrow to see the submenu open
  4. Navigate through the submenu using tabs or arrows. Enter selects the link

Screenshots

menu

Copy link
Contributor

@ygannett ygannett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see below comments for the markup changes.

Here is the spec of the context for the button/link labels to add some clarification. The issue is described in my comment in the JIRA.

main-nav-unit

1. Top level menu

  • Add visually hidden text of "Show/Hide the sub topics of" before the current label text.
  • "Show" and "Hide" reflect the sub menu's status. It matches aria-expanded's status.

So, with Living, the button would be announced as "Show/Hide the sub topics of Living".

2. The last link of the sub menu

  • Add visually hidden text of "See all topics under ".

With above sample, the link is announced as "See all topics under Living".

Please let me know for any questions.

@@ -19,7 +19,7 @@
</div>
{% endif %}
</div>
<nav class="ma__header__nav" aria-labelledby="main_navigation" id="main-navigation">
<nav class="ma__header__nav" aria-label="main_navigation" id="main-navigation" role="navigation">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aria-labelledby creates association with other HTML element using the element's ID. So, in the original markup, <nav class="ma__header__nav" aria-labelledby="main_navigation" id="main-navigation"> has aria-labelledby="main_navigation". The following h2 is <h2 id="main_navigation" class="visually-hidden">Main Navigation</h2>. Find its ID value, main_navigation matches the value of aria-labelledby in nav. They tells screen readers the h2 is a heading/label for the <nav>. So, this was a correct markup.

aria-label, on the other hand, is stand-alone. Its value is a label for the element. There is no association with other html elements. Screen readers read out the value as the element's label.

Now, nav has aria-label instead of aria-labelledby, 2 things need be done.

  1. remove the underscore in the aria-label value, otherwise screen readers announce the value as "main underscore navigation".
  2. remove <h2 id="main_navigation" class="visually-hidden">Main Navigation</h2> since it's replaced with the aria-label. If you leave it, screen readers says: "main underscore navigation. main navigation"

Or, change the aria-label back to aria-labelledby. They both do the same thing, so it doesn't matter either method.

@@ -1,7 +1,7 @@
<div class="ma__utility-nav js-util-nav">
<ul class="ma__utility-nav__items">
{% if googleLanguages %}
<li class="ma__utility-nav__item">
<li class="ma__utility-nav__item" tabindex="0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a little bit deceptive. It doesn't show the link added by Google Translate's JS in the template, but you can see it in the DOM:

<ul class="ma__utility-nav__items">
  <li class="ma__utility-nav__item">
    <div class="ma__utility-nav__translate">
          ...
              <a aria-haspopup="true" class="goog-te-menu-value" href="javascript:void(0)"><span>Select Language</span><img src="https://www.google.com/images/cleardot.gif" alt="" width="1" height="1"><span style="border-left: 1px solid rgb(187, 187, 187);">​</span><img src="https://www.google.com/images/cleardot.gif" alt="" width="1" height="1"><span aria-hidden="true" style="color: rgb(118, 118, 118);">▼</span></a>
            ...
    </div>
  </li>

Tab key navigates you between clickable elements such as links and buttons. Those elements have tabindex="0" as default even it's not shown in the markup. The link is already set up to receive focus. So, its parent container <li> doesn't need tabindex="0". If you tab through Mass.gov, you see "Select Language" gets focus though it's a little hard to see the focus outline.

If there is some reason to add it, please let me know.

@@ -14,6 +14,7 @@
<li class="ma__utility-nav__item js-util-nav-toggle">
<a class="ma__utility-nav__link"
href="#"
tabindex="0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like mentioned above, <a> has its default setting with tabindex="0" even though it doesn't show the attribute expressively. So, this can be removed.

@@ -1,11 +1,11 @@
<div class="ma__main-nav">
<ul class="ma__main-nav__items js-main-nav">
<ul class="ma__main-nav__items js-main-nav" role="menubar" aria-hidden="false">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, aria-hidden's default value to html elements you see in a browser window without the attribute is "false."
So, you don't need to add it at all, which makes your life a little easier... less typing 😋

{% for nav in mainNav %}
{% set buttonId = 'button' ~ loop.index %}
{% set menuId = 'menu' ~ loop.index %}
<li class="ma__main-nav__item {{ nav.active ? 'is-active' : '' }} {{ nav.subNav ? 'has-subnav js-main-nav-toggle' : 'js-main-nav-top-link' }}">
<li role="menuitem" class="ma__main-nav__item {{ nav.active ? 'is-active' : '' }} {{ nav.subNav ? 'has-subnav js-main-nav-toggle' : 'js-main-nav-top-link' }}" {% if nav.subNav %}aria-haspopup="true" {% endif %}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. role="menuitem" is supposed to be on elements, which actually have menu labels. Move
    role="menuitem" to button.ma__main-nav__top-link. And, add role="presentation" to this element, li.ma__main-nav__item since this is just a container to represent the structure of the navigation.

  2. aria-haspopup is supposed to placed on a popup trigger element, which is button.ma__main-nav__top-link in this case. You fine the button.ma__main-nav__top-link has aria-haspopup below.
    {% if nav.subNav %}aria-haspopup="true" {% endif %} can be removed from li.ma__main-nav__item .

{% if nav.subNav %}
<button id="{{ buttonId }}" class="ma__main-nav__top-link" aria-haspopup="true" aria-expanded="false" tabindex="{{ loop.first? 0 : -1 }}">{{ nav.text }}</button>
<button id="{{ buttonId }}" class="ma__main-nav__top-link" aria-haspopup="true" aria-expanded="false" tabindex="0">{{ nav.text }}</button>
<div class="ma__main-nav__subitems js-main-nav-content is-closed">
<ul id="{{ menuId }}" role="menu" aria-labelledby="{{ buttonId }}" class="ma__main-nav__container">
<li role="menuitem" class="ma__main-nav__subitem"><a href="{{ nav.href }}" class="ma__main-nav__link" tabindex="-1">{{ nav.text }}</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change as the top level.

  1. Change the role, "menuitem" on li.ma__main-nav__subitems to "presentation".
  2. Add role="menuitem" to a.ma__main-nav__link.

tabindex="-1" on li.ma__main-nav__subitem can be removed when div.ma__main-nav__subitems has display: none; with its status class is-closed.

{% if nav.subNav %}
<button id="{{ buttonId }}" class="ma__main-nav__top-link" aria-haspopup="true" aria-expanded="false" tabindex="{{ loop.first? 0 : -1 }}">{{ nav.text }}</button>
<button id="{{ buttonId }}" class="ma__main-nav__top-link" aria-haspopup="true" aria-expanded="false" tabindex="0">{{ nav.text }}</button>
<div class="ma__main-nav__subitems js-main-nav-content is-closed">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

div.ma__main-nav__subitems should be set display: none; when the sub menu is NOT displayed (with its status class is-closed) to prevent any of its descendant elements accidentally get focus.
It shouldn't be just pushed away from the visible area of the browser window since this is still focusable.

<div class="ma__main-nav__subitems js-main-nav-content is-closed">
<ul id="{{ menuId }}" role="menu" aria-labelledby="{{ buttonId }}" class="ma__main-nav__container">
<li role="menuitem" class="ma__main-nav__subitem"><a href="{{ nav.href }}" class="ma__main-nav__link" tabindex="-1">{{ nav.text }}</a></li>
<li role="presentation" class="ma__main-nav__subitem"><a href="{{ nav.href }}" class="ma__main-nav__link">{{ nav.text }}</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry I made a mistake. During testing with screen readers, I realized <li>'s role is better with menuitem. Please change back to menuitem from presentation.

{% for subNav in nav.subNav %}
<li role="menuitem" class="ma__main-nav__subitem"><a href="{{ subNav.href }}" class="ma__main-nav__link" tabindex="-1">{{ subNav.text }}</a></li>
<li class="ma__main-nav__subitem"><a role="menuitem" href="{{ subNav.href }}" class="ma__main-nav__link">{{ subNav.text }}</a></li>
Copy link
Contributor

@ygannett ygannett Apr 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, this is my mistake. <li>s should have role="menuitem", not <a>s.
With <a role="menuitem">, screen readers don't announce it as a clickable element in Chrome. In FF, JAWS doesn't recognized it as a part of a group menu since <a role="menuitem"> isn't a direct descendant of <ul role="menu">.
Please move role="menuitem" from <a> to its parent <li>.
The same change applies to LL. 15-16 as well.

<li role="menuitem" class="ma__main-nav__subitem">
<a href="{{ nav.href }}" class="ma__main-nav__link" tabindex="-1">
<li class="ma__main-nav__subitem">
<a role="menuitem" href="{{ nav.href }}" class="ma__main-nav__link">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the role="menuitem" from <a> to <li> to maintain the link role on <a>.

@ygannett ygannett merged commit c926858 into develop Apr 16, 2019
@avrilpearl avrilpearl deleted the patternlab/DP-6198--main-menu-keyboard-navigation-V2 branch June 27, 2019 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants