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-23086 A11y when org search is not shown, it should be fully hidden for all users #1531

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

Javi-er
Copy link
Contributor

@Javi-er Javi-er commented Oct 14, 2021

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

Description

Hides the search form so it's not focusable by the keyboard instead of using just opacity.

Related Issue / Ticket

Steps to Test

  1. Visit any organization and place the focus on the organization navigation.
  2. Press the "tab" key until focusing on the search, the next tab should go to the content and not dissapear.

Screenshots

Use something like licecap to capture gifs to demonstrate behaviors.

Additional Notes:

Anything else to add?

Impacted Areas in Application

@todo

Today I learned...

@clairesunstudio
Copy link
Contributor

clairesunstudio commented Nov 1, 2021

So, I fixed the keyboard focus trapping issue:

  • Main nav only:
    org-nav-fix
  • Sub nav is open:
    org-nav-subnav-fix

The sub nav part was more involved:

  1. There is a bug that prevents me from interacting with the subnav with keyboard, if you use keyboard (instead of use mouse to click ) to open the sub nav the sub nav content is not displaying properly in terms of its positioning and it doesn't toggle. In order to go around this to work on the focus trapping, I temporarily commented out a section of code which is breaking the subnav on mobile - I think it's for the desktop interaction. Can you look into the issue and fix it?
    See my screen capture (Uncomment the code block to replicate this issue):
    org-nav-bug
  • I had to fix a semantic issue for link list which was using heading as interactive element
    I added a version for mobile to include a button in the heading in the template and adjusted the SCSS and JS accordingly. Please make sure nothing needs to be done in openmass.
    Update: created a openmass PR for the changes needed --> https://github.com/massgov/openmass/pull/1120/files

}
// Capture click, spacebar and enter keys.
$mobileToggle.keypress(function() {
if (event.which == 13 || event.which == 32) mobileToggleClick();
Copy link
Contributor

Choose a reason for hiding this comment

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

.which is deprecated and don't support IE11, please use KeyboardEvent.key and KeyboardEvent.code for new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I just updated this.

}

// Capture click, spacebar and enter keys.
$mobileToggle.keypress(function(e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$mobileToggle.keypress(function(e) {
$mobileToggle.keypress(function(event) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it with keyboardEvent

$button.on('focus', function () {
$thisMenu.find("a[href]").attr("tabindex", -1);
$otherMenus.find("a[href]").attr("tabindex", -1);
// $button.on('focus', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the commented code I mentioned above


$('.item-open').removeClass('item-open');
// TO DO: Close mobile menu container.
Copy link
Contributor

@ygannett ygannett Nov 24, 2021

Choose a reason for hiding this comment

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

Note for TO DO:
This is tricky one. It's meant to close the mobile menu container, which is different from one above. The one above at L. 182 closes an open submenu. Based on the way the mobile nav works, this is triggered when submenus are closed, which means one of the navigation buttons/links or the search is supposed to have focus. If you just check if the mobile container is open for the trigger, the condition also applies to L. 182.

@ygannett
Copy link
Contributor

@Javi-er I merged my changes into your branch.
Here are the changes from my branch:

Template + CSS

  • Remove role=”menu” from li
  • Change the org nav menu button from div to button (10)
    • Add aria-expanded
    • Add aria-haspopup
    • Add role=”menu”
    • Remove tabindex
  • Change visibility of the sub menu container from visually-hidden to visibility: hidden; (10)
  • Change the org search component toggle from div to button (17)
    • Add aria-label
  • Add the org search component toggle button’s aria-describbedby content container, div#org-search-note (14)
  • Replace css pseudo content with span for x icon (18)
    • Related CSS change

JS

  • Change the event handler to open/close the org sub menu from hover to click (ENTER/SPACE) (13)
  • Set up state change for aria-expanded, aria-hidden and css for visibility for the sub menu for desktop (10)
  • Set up state change for aria-expanded for mobile toggle (10)
  • Set up state change for aria-expanded and aria-label for search toggle (14)
  • Set up the close state condition for the nav menu (10, 13)
  • Set up to open one submenu at once (= no multiple submenus open) (10)
  • Consolidate the event listener for the menu button for mobile and desktop
  • Set up to close submenu and set focus on menu button with ESC on desktop (5)
  • Set up to close submenu when tab is hit on the last focusable element in the submenu
  • Set up to close submenu when shift + tab are hit on menu button while its submenu is open to prevent covering the main page content when the submenu is not in use
  • Set up focus switch between original and clone menu button on mobile.
  • Fix animation of flying out submenu on mobile.

Here is the to-do list. At the point my changes were merged, I did some quick testing with keyboard and screen readers. Items listed at the fix are not working as expected. Not sure if they are syntax or jQuery issues or something else.

TO DOs

  • Set up focus trapping for mobile version top menu level (currently, once focus reaches the last focusable element in the menu, next tabbing moves to the main page while the navigation container is still open.
  • Set up to close the mobile navigation with ESC. (The subnav already closes with ESC.)
  • Fix:
    • Unable to access to second and later top level nav items in mobile with NVDA
    • Unable to access to subnav items in mobile in MS Edge, FF with NVDA
    • Unable to open the org nav container in mobile with ENTER/SPACE in Chrome, Edge and FF with JAWS and NVDA
    • Unable to access submenu items in desktop in Chrome, Edge with JAWS though submenu is displayed
    • Unable to open the mobile menu with ENTER and SPACE in Windows
    • Nothing happens as ENTER is hit
    • Hitting SPACE opens the menu, then close like a blink, it doesn’t stay open
    • After using ESC, focus gets lost with JAWS, NVDA
    • Opening a submenu with ctr + option + space (Voiceover key command equivalent to “click” or hitting ENTER) opens the browser menu in Firefox with Voiceover
    Screen Shot 2021-11-24 at 9 32 40 AM

Screen Shot 2021-11-24 at 9 31 36 AM

  • Unable to open submenu in desktop in Safari with Voiceover

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.

3 participants