Problem/Motivation

The responsive menu in Umami has some accessibility problems:

  1. The button only does something when JS is enabled. When JS is off, the button is there in markup, but does nothing.
  2. Menu links are always in the tabbing order, changing the wrapper's height just hides them visually. A keyboard user will have to tab through these even when the menu is closed. For screen reader users, it defeats the object of the toggle button. For sighted keyboard users, the links are invisible-but-still-operable. This is a failure of WCAG 2.0 SC 2.4.7 Focus Visible.
  3. The open closed state of the menu is not conveyed to assistive tech. Screen reader users just know there's a button, not whether the menu is open or closed.
  4. Th menu open/close is animated, but some users prefer no motion. We're forming a Drupal core policy around this, see #2928103: [policy, no patch] Use "prefers-reduced-motion" media query to disable animations.
  5. The button is outside the navigation landmark. When closed, a screen reader user might search for the navigation landmark region, but find it empty. They should find the button inside the nav landmark region.
  6. There are some unnecessary <title> and <desc> elements inside the SVG icon. The accessible name comes from button's own aria-label.

Proposed resolution

  1. DEFERRED, NEEDS FOLLOW-UP ISSUE: Remove the button from the Twig template, create it using JS instead
  2. DONE: Use display: none or visibility: hidden on the <ul> to remove the links from the tabbing order when the menu is closed.
    • If this is a problem for the animation, do it in two steps. When the menu opens, first remove the display:none, then change the height. Reverse order of these steps when closing.
  3. NEEDS WORK: As well as toggling the CSS class for styling, toggle aria-expanded=true|false. This attribute goes on the <button>.
    • The <button> also wants an aria-controls=<IDREF>, pointing to the ID of the <ul> that gets toggled.
  4. DONE, NEEDS MANUAL TESTING. Use the new prefers-reduced-motion media query to allow users to turn off animation if their platform allows it (currently just works in Safari).
  5. DEFERRED, NEEDS FOLLOW-UP ISSUE: Move the toggle button to just inside the start of <nav>. Note, in the previous github issue this was highlighted as something that could be tricky, because the CSS currently assumes the button is outside the nav.
  6. DEFERRED, NEEDS FOLLOW-UP ISSUE: Remove the <title> and <desc> elements from the SVG icon.

Remaining tasks

Update the Umami JS.

User interface changes

No visual changes. Accessibility wins:

  1. Conveys the menu open/closed state to assistive tech in a machine readable way.
  2. Fixes a failure of WCAG "Focus Visible" for sighted keyboard users
  3. Makes the menu button easier to find using landmark navigation in assistive tech.

API changes

None.

Data model changes

None.

Original report

Previously discussed in detail at issue #130 - Improve menu accessibility in the lauriii/umami Github repo.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

Issue summary: View changes
andrewmacpherson’s picture

Title: Improve accessibility of Umami's responsive main menu » Improve accessibility of Umami's responsive main menu
Issue tags: +Out of the Box Initiative
andrewmacpherson’s picture

Issue summary: View changes
andrewmacpherson’s picture

Issue summary: View changes
andrewmacpherson’s picture

ryanhayes’s picture

I'm gunna have a crack at this.

andrewmacpherson’s picture

Issue tags: +SprintWeekend2018
mgifford’s picture

Easy to test with here:
https://simplytest.me/project/drupal/8.6.x

The color contrast on "Super easy vegetarian pasta bake" with that pink background is borderline. Is it bigger than 18pt? http://leaverou.github.io/contrast-ratio/#white-on-%23e84265

The tab order doesn't in the header doesn't make a lot of sense for keyboard only issues either.

andrewmacpherson’s picture

@mgifford This issue is just about the responsive menu.

The pink button contrast is addressed already in #2938686: Pink button/link style in Umami fails WCAG 2.0 SC 1.4.3 Contrast (minimum)

The tab order is something I already have on a list, haven't got a d.o issue for it yet. We're in the midst of moving issues from the Umami github repos to d.o

Update: the old github issue is https://github.com/lauriii/umami/issues/140

mgifford’s picture

Thanks!

ryanhayes’s picture

I could't figure out how to create a new button with JS properly, so hopefully someone better at JS can pick that up. I added a simple conditional for the aria-expanded tags - feel like it could be more streamline though. There's a bug with adding visibility hidden to the menu and i couldn't find a way around it; on close, the menu would disappear before the animation finished.

ryanhayes’s picture

Status: Active » Needs review
andrewmacpherson’s picture

Issue summary: View changes

Thanks for working on this @ryanhayes. I took a quick glance at the patch, and it looks like a good start. I'll take a closer look in the next few days.

It might be worth splitting this into separate issues, if some parts are going to be tricker than others. Points 2 and 3 are the most important ones, the rest could be done in follow-ups. Accessibility often involves practical compromises, and I'm happy to go for quick-wins first.

markconroy’s picture

Hi Ryan and Andrew,

Been looking at this this morning and running into the same issue that Ryan has.

If we set the <ul> to display:none then the transition is lost as you can't have transition between display types. Next, if we set the <li> to display:none instead we have the transitions working for when the menu is expanding (if we give the <li> a display: block when then <ul> is clicked). However, when you click the menu toggle again to retract the menu, the <li>s get back their display: none so the transition doesn't work, the menu just closes with no transition.

Here's a patch that show this in action - so the links should not be available to keyboard users, but also the transition is slightly out of kilter.

I've also set the transition to none for people who choose reduced motion.

andrewmacpherson’s picture

Thanks for working on this Mark.

re: #15...
Good idea to try display:none and animation properties on different elements.

You could try using visibility:none on the list items. That also has the effect of removing them from the tabbing order, but preserves space so maybe that will make the list height animate better?

I've also set the transition to none for people who choose reduced motion.

Apparently some browsers don't work well with the CSS shorthand transition: none. Using specific properties like transition-duration: 0s would be a more robust approach. See #2940012-9: Use prefers-reduced-motion media query on batch progress bar in a related issue.

I haven't tried patch #15 yet, need to wait till I'm near an Apple device.

markconroy’s picture

Here's a patch with visibilty set to hidden and the transition set to 0s

andrewmacpherson’s picture

Review of patch #17....

Manual testing:

  1. The menu links are hidden a the wide breakpoint - BAD, unintended side effect.
  2. The menu links are not in the tabbing order when the menu is closed - GOOD, fixes problem 2 from the issue summary.
  3. aria-expanded should applied to the button when the page loads. NEEDS WORK.
    • In Firefox it doesn't appear until after the first button pressed. After that it's behaving correctly. A screen reader user won't know what state the button is in initially - they will only find out after pressing it.
    • In Chrome it isn't ever applied, even after pressing the button.
  4. aria-controls isn't being applied to the button. NEEDS WORK.

Not manually tested yet:

  • The prefers-reduced-motion part. Simplytest.me environment builds were failing, and I don't have a dev server set up on my mac. The code approach looks OK in theory. GOOD, progress towards fixing point 4 from the issue summary. Still needs manual testing though.
  • Screen readers other than ChromeVox.

=====

Code review:

  1. +++ b/core/profiles/demo_umami/themes/umami/css/components/navigation/menu-main/menu-main.css
    +.menu-main li {
    +  visibility: hidden;
    +}
    

    Only do this at the narrow breakpoint, the links are hidden in the wide breakpoint.

  2. +++ b/core/profiles/demo_umami/themes/umami/css/components/navigation/menu-main/menu-main.css
    .menu-main--active li {
      visibility: inherit;
    }
    

    Likewise I guess this wants to move into the narrow breakpoint media query?
    Why is it visibility: inherit? Is visibility: visible more reliable?

  3. +++ b/core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js

    The aria-expanded and aria-controls attributes should be initialised in JS, after finding the right elements.
    Need to add this code here.

  4. +++ b/core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js
    + var aria = false;
    1. Should be const in ES6
    2. aria is a bit vague, maybe call it expanded
  5. +++ b/core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js
    function toggleMenu()
    
    1. The logic of toggleMenu() is a bit verbose, the if/else for expanded state can be reduced
    2. possibly fragile - need to guard against the classes falling out of step with aria-expanded. Suggest we get the current value of one property from the DOM (aria-expanded, or a data attribute, or a class) and compute all classes and aria-expanded off that.
    3. no need to return false; - <button type="button> doesn't have a default action to begin with. Not sure if propagation needs to be stopped?
  6. +++ a/core/profiles/demo_umami/themes/umami/templates/components/navigation/block--umami-main-menu.html.twig
    -<div class="menu-main-togglewrap">
    -  <button type="button" name="menu_toggle" class="menu-main-toggle" data-drupal-selector="menu-main-toggle" aria-label="Toggle the menu">{% include active_theme_path() ~ '/images/svg/menu-icon.svg' %}</button>
    +<div id="menu-main-toggle" class="menu-main-togglewrap">
    +  <button type="button" name="menu_toggle" class="menu-main-toggle" data-drupal-selector="menu-main-toggle" aria-label="Toggle the menu" aria-expanded="false" aria-controls="menu-main-toggle">{% include active_theme_path() ~ '/images/svg/menu-icon.svg' %}</button>
    
    1. just say aria-label="main menu". No need to have toggle in the button name, aria-expanded will convey that.
    2. aria-expanded="false" aria-controls="menu-main-toggle". Never initialize dynamic aria properties in the HTML source, add both of these via JS.
    3. aria-controls="menu-main-toggle" - this shouldn't point at the DIV wrapping the button. It should point to the ul.menu-main, which doesn't currently have an ID attribute.
  7. +++ a/core/profiles/demo_umami/themes/umami/templates/components/navigation/menu--main.html.twig
    -      <ul{{ attributes.addClass(menu_classes).setAttribute('data-drupal-selector','menu-main') }}> {# 1. #}
    +      <ul{{ attributes.addClass(menu_classes).setAttribute('data-drupal-selector','menu-main').setAttribute('aria-expanded', 'false') }}> {# 1. #}

    This has 2 mistakes:

    1. The UL needs an ID so the button can point to it with aria-controls
    2. aria-expanded attribute should go on the button only, not the list element. JS toggleMenu() isn't touching it in any case, so just remove it here.
      BTW never initialize dynamic ARIA states in the HTML source. Always add them via JS. Doesn't matter in this case, since we need to remove the aria-expanded form the UL

Other notes:

  1. .menu-main {
      ...
      max-height: 0;
      overflow: hidden;
    }
    .menu-main--active {
      max-height: 18.75rem;
      overflow-y: auto;
    }
    

    The default state is closed, since the .menu-main--active class is not present in HTML source. This means that if JS fails to run for some reason, the main menu links will not be operable. This was discussed in the earlier github issue. The menu should be open by default with HTML + CSS, and only closed if JS runs.

  2. The SVGs have moved to separate files. Part 6 from the issue summary still stands, the useless title and desc elements should be removed. Could move that to a follow-up issue to review all SVG files though.
andrewmacpherson’s picture

Status: Needs review » Needs work
andrewmacpherson’s picture

Next steps - let's split this into must-should-could priorities.

  • Must:
    • Use visibility:hidden to remove the links form tabbing order when menu is closed (point 2 in issue summary).
      DONE, works for narrow breakpoint.
      NEEDS WORK: avoid hiding them at wide breakpoint, per #18.1
    • Convey open/closed state of menu to assistive tech using aria-expanded and aria-controls(point 3 in issue summary).
      NEEDS WORK: review in #18 identifies the mistakes.
  • Should:
    • Use prefers-reduced-motion to disable animation (point 4 in issue summary).
      DONE, still needs manual test to confirm.
    • Move the menu toggle button inside the NAV element. (point 5 in summary).
      TODO. In the earlier github issue, it was explained that this could be tricky because the CSS was built around a certain HTML pattern, so there's lots to unpick to achieve this.
  • Could:
    • Remove the button from HTML source, initialize it in the JS (point 1 in issue summary). Low priority.
    • Remove title and desc elements from the SVG icon (point 6 in issue summary). EASY, but let's review all the icons now they live in a separate folder.

Right, lets fix the must-haves here, and move the rest to follow-up issues.

andrewmacpherson’s picture

Category: Task » Bug report

Treat the must-have points from #20 as bugs.

andrewmacpherson’s picture

Issue tags: +Needs followup
andrewmacpherson’s picture

Issue summary: View changes

updating summary, to indicate which tasks need follow-ups, vs ones already in progress or done here.