[PP-1] Improve accessibility of Umami's responsive main menu

@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

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.

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.

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.

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.

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

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

Needs another re-roll, patch didn't apply to 8.7.x on my machine. Working on this now.

../patches/2940023-35.patch:76: trailing whitespace.
 
../patches/2940023-35.patch:97: trailing whitespace.
 
../patches/2940023-35.patch:107: trailing whitespace.
 
../patches/2940023-35.patch:126: trailing whitespace.
 
../patches/2940023-35.patch:134: trailing whitespace.
 
error: patch failed: core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js:7
error: core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js: patch does not apply

Patches #27 and #35 both contain the whole of patch #17 as a new file. Sounds like the re-roll approach in #35 would have missed this problem....

diff --git a/2940023-17.patch b/2940023-17.patch
new file mode 100644
index 0000000000..85ea3ef683
--- /dev/null
+++ b/2940023-17.patch
@@ -0,0 +1,148 @@
...

The patch here is another re-roll, like #35 starting from the commit before #2943107, applying #27, merging latest 8.7.x. Then I removed the stray patch #17, and made a diff.

I'll do a manual a11y review next.

Needed another re-roll since #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier. Nothing's changed here except a context line in main-menu.es6.js

The name="menu_toggle" attribute can be removed from the button element. This attribute is only relevant if the button is part of a form. It's not being used for anything - it's the only of instance of the "menu_toggle" string.

removes name attribute from the button.

This patch is just whitespace indentation fixes, effectively a re-roll, so we don't undo the coding standards clean-up from #2983355: JS codestyle: indent.

Some more clean-up in this patch:

1. toggler.classList.toggle('menu-main-toggle--active');
   Removing this, because it isn't used for anything. It's been there since Umami was committed to core, and this is the only place it's ever mentioned. Now aria-expanded is the only attribute on the button which changes.

2. (function() {
      // ...
   
      menu.classList.toggle('menu-main--collapsed');
      // ...
     
      function toggleMenu() {
   

   This is initializing the class for the first time, so I changed it to classList.add() for clarity.

3.   function toggleMenu() {
       // ...
       menu.classList.add('menu-main--transition');
   

   It took me a while to realize what this was doing. It looks like one-time set-up, but it's happening inside the click handler, instead of being initialized at the same time as the other attributes. This is by design, and it was explained back in comment #27:

   Added special class for transition .menu-main--transition to avoid transitions of collapsing on page load.

   I think this deserves a comment about why it's being done inside the click handler.

4.   function toggleMenu() {
       const expanded = toggler.getAttribute('aria-expanded');
   
       // ...
       menu.classList.toggle('menu-main--collapsed');
   
       if (expanded === 'false') {
         toggler.setAttribute('aria-expanded', 'true');
       } else {
         toggler.setAttribute('aria-expanded', 'false');
       }
   

   This is potentially fragile. When setting aria-expanded, it explicitly checks what the current value is, but then it toggles the class without being sure whether it's present currently. It's important to ensure these never fall out of sync, so I replaced the classList.toggle() with separate add() and remove() in the if/else block. Now they're both tied to the same check.

There's a visual bug: during the collapse animation, we briefly see a scrollbar in the UL list....

This affects all the desktop browsers we support: IE11, Edge, Firefox, Opera, Chrome, Safari (Windows, Linux, macOS versions). I also saw it on an Android/Chrome tablet, but not on phone-sized Android/iOS. It may not be apparent when testing with a Mac, because of the way the scrollbars get hidden. You can see it if you make the scroll bars visible at System Preferences > General > Show scroll bars - I think Apple's default setting is to make you guess which areas are scrollable.

The culprit is .menu-main--collapsed { overflow-y:auto; }, which is overriding .menu-main { overflow: hidden; }. The patch here fixes it.

Manual accessibility testing:

I tested this using a keyboard in all our supported desktop browsers, on macOS 10.13, Windows 7, Windows 10. I also tried it with a decent range of screen readers: NVDA 2018.2 (Win 7 + 10), JAWS 2018 (win 10), Narrator (Win 10), Voiceover (macOS), Voiceover + Safari (iOS latest), Android 8.0 + Talkback + Chrome/Firefox. Generally everything was as expected, but there were a few odd things:

• IE11 + JAWS (on Win 7 I think) - The links were announced when the menu was collapsed. That's unexpected, but it works in all the other combinations I tested so that's good. Meh, you can't win them all in screen reader testing... I might keep scratching this itch, but it isn't enough to block this issue.
• IE11 + JAWS (Win7 + Win 10) - The menu button's name, role, and state were announced correctly. However in the arrow-keys reading mode, the nested SVG was also announced, using the title and description elements. These are verbose, unnecessary, and irrelevant. We have a follow-up for this at #2942258: Remove unnecessary TITLE and DESC elements from Umami SVG files.
• There's an invisible tab-stop in the site-branding block - the visually-hidden site-name link. This is a failure of WCAG "focus visible", so I'll file a separate follow-up. It's not part of the responsive menu, so out of scope here.

This is great work - thanks @ryanhayes, @markconroy, @finnsky for working on this. The prefers-reduced-motion is ace, and we've seen upstream issues for Firefox to implement the media query too.

I think this is RTBC-worthy, but someone else will have to review my recent changes.

I did a quick test of moving the button inside the nav region, between the H2 and UL, by editing just the block--umami-main-menu.html.twig template. As expected, some CSS changes will be needed, so it's right to defer that to #2973258: Place the Umami menu button inside the <nav> element.

I did the SVG clean-up, removed the unhelpful title and desc.

Needed a re-roll after #2983568: Audit and improve focus styles across the Umami theme for logged out users, it appiled cleanly with git apply --3way. The previous issue changed a colour value in menu-main.css which was used for context in the patch.

@shaal - That's exactly what I changed in patch #46.

But this introduces another problem. Using overflow: hidden with max-height is a high accessibility risk, because content cannot be seen once it exceeds a certain height. Bizarrely, I don't think this is an explicit failure of WCAG! It doesn't fail WCAG "Focus Visible", because tabbing will cause the content of the overflow to move, (in Firefox and Chrome at least; I tested these). However sighted pointer users won't be able to access the links hidden inside the overflow, so that's definitely an accessibility and usability problem.

The goal, I think, is we don't want to show the scrollbar during the brief transition, if the list content height never exceeds the max-height of the list container ul. But if the content exceeds the max-heigh, it's essential to show provide access to links on the overflow, so we can't use overflow: hidden; when the menu is fully open.

I notice we apply the .menu-main--transition class once, on the first transition, and never remove it. Maybe we can use transition start/end JS events to suppress the scrollbar during the transition.

Another problem: using max-height: 18.75 rem accommodates a maximum of 5 menu links. I think evaluators playing with Drupal's menus will easily exceed this when adding some pages.

The menu can overflow to multiple rows at the wider breakpoints, and I found it could hold 7 one-word links on one row (in English). It would be a good idea to increase the max-height of the mobile menu to reduce the likelihood of menu links disappearing on the small breakpoints.

This will be a good one to work on during the Drupal Global Contribution Weekend 2019.

The work in #49 is RTBC worthy I think, but we can add tests. I had a go at writing some FunctionalJavascript tests for the keyboard behaviour, as a way to get some momentum for #3029206: [policy, no patch] Require functional test coverage for keyboard accessibility. These tests aren't finished yet, just uploading my progress.

But if we can't use nightwatch yet, why not move forward with WebDriverTestBase? This menu a11y bug is major, so I'd prefer not to postpone this for tbe nightwatch issue.

@finnsky - Can you elaborate on #60? I can move the WebDriverTestBase approach forward, but I don't understand the extra Nightwatch thing you mention. Is drupalInstallProfile a function name, or something else? I don't see it in your patch.

Meanwhile, the problem with the way max-height is used (#51-52) needs addressing.

Thanks @shaal. Review of patch #62...

1. We don't have @finnsky's first stab at a Nightwatch test from #58. I think this is worth moving forward with. In Slack, @finnsky explained that we might be able to have a temporary version of the drupalInstall.js command if #3017176: Support install profile and language code params in drupalInstall Nightwatch command isn't fixed before this one. For now, we'll be able to use the patch there, and keep working on the Nightwatch test.
2. We're also missing the whitespace cleanup from #57. No worries, we can add those back. They are in the latest codesniffer fixes patch, generated in #63.

3. ../patches/umami-a11y-main-menu-2940023-62.patch:187: trailing whitespace.
       
   ../patches/umami-a11y-main-menu-2940023-62.patch:191: trailing whitespace.
       
   ../patches/umami-a11y-main-menu-2940023-62.patch:193: trailing whitespace.
       // explicitly set the element's height to its current pixel height, so we 
   ../patches/umami-a11y-main-menu-2940023-62.patch:198: trailing whitespace.
         
   ../patches/umami-a11y-main-menu-2940023-62.patch:203: trailing whitespace.
         
   warning: squelched 1 whitespace error
   warning: 6 lines add whitespace errors.
   

   Errors seen when applying the patch. Several lines have trailing whitespace, inside the new collapsSection() function in menu-main.es6.js.

4. Comments in collapseSection() and expandSection() should use capitals at the start of sentences, full stops at the end, and be wrapped at 80 chars.

5. --- a/core/profiles/demo_umami/themes/umami/css/components/navigation/menu-main/menu-main.css
   +++ b/core/profiles/demo_umami/themes/umami/css/components/navigation/menu-main/menu-main.css
   @@ -11,20 +11,22 @@
      font-size: 1.266rem;
      font-weight: 400;
      line-height: 1.2;
   -  max-height: 0;
      overflow: hidden;
      padding: 0;
   -  transition: max-height 0.5s ease-in;
   +  transition: height 0.5s ease-in-out;
   +  height: 0;
   

   This is a regression since patch #54. If JS is disabled, or fails to run, the menu links will not be visible, so pointer users cannot navigate the site. The links are still in the tabbing order though, which means screen reader users can navigate the site, howevere it's a failure of WCAG 2.4.7 Focus Visible for sighted keyboard users. The way the patch in #54 works, the menu will be expanded if JS fails to run, so users can still navigate the site.

6. --- b/core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js
   +++ b/core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js
   @@ -13,19 +13,58 @@
    
   +  function collapseSection(element) {
   +    
   +    // temporarily disable all css transitions
   +    var elementTransition = element.style.transition;
   +    element.style.transition = '';
   +    
   +    // on the next frame (as soon as the previous style change has taken effect),
   +    // explicitly set the element's height to its current pixel height, so we 
   +    // aren't transitioning out of 'auto'
   +    requestAnimationFrame(function() {
   +      element.style.height = sectionHeight + 'px';
   +      element.style.transition = elementTransition;
   +      
   +      // on the next frame (as soon as the previous style change has taken effect),
   +      // have the element transition to height: 0
   +      requestAnimationFrame(function() {
   +        element.style.height = 0 + 'px';
   +      
   +        requestAnimationFrame(function() {
   +          // remove "height" from the element's inline styles, so it can return to its initial value
   +          element.style.height = null;
   +        });
   +      });
   +    });
   +  }
   

   The comments here are a bit hard to follow, and could be clearer. When it says "the previous style change", is that referring to the previous line which assigns element.style.transition?

7. +
   +    // when the next css transition finishes (which should be the one we just triggered)
   +    element.addEventListener('transitionend', function(e) {
   +      // remove this event listener so it only gets triggered once
   +      element.removeEventListener('transitionend', arguments.callee);
   +    });
   

   This looks a bit esoteric. We're adding an event listener which apparently does nothing, except remove itself. Can you explain why it's here? My only guess is it might be there to suppress (or ensure) something elsewhere, but I can't tell what it's for by reading this snippet.

8. +      element.removeEventListener('transitionend', arguments.callee);
   

   The use of arguments.callee is deprecated. The anonymous event listener for transitionend can be replaced with a named callback. That's good, because the name can explain the mysterious purpose in the previous point :-)

The patch still has my stub/pseudocode WebDriverTestBase approach. If we use Nightwatch, we won't need the WebDriverTestBase class.

+  'Test expand': browser => {
+    browser
+      .resizeWindow(400, 700)
+      .drupalRelativeURL('/test-page')
+      .waitForElementVisible('body', 1000)
+      .assert.elementPresent('.menu-main-toggle')
+      .assert.hidden('#menu-main')
+      .assert.attributeEquals('.menu-main-toggle', 'aria-expanded', 'false')
+      .click('.menu-main-toggle')
+      .assert.attributeEquals('.menu-main-toggle', 'aria-expanded', 'true')
+      .assert.visible('#menu-main')
+      .drupalLogAndEnd({ onlyOnError: false });
+  },

This is on the right lines, however...

• Note that it doesn't test keyboard accessibility. To do that we'd need to assert:
  • The button is in the tabbing order, not merely that it has a click handler. We could check that using jQueryUI's :tabbable selector, or similar approach. The point of asserting that it is tabbable is to guard against implementations of fake buttons like <div role="button" onclick="toggleMenu()">menu</div> (which isn't keyboard tabbable).
  • Add keypress<code> checks for space and enter keys. The point here is to guard against implementations of fake buttons like <code><div role="button" tabindex="0" onclick="toggleMenu()">menu</div> (which is tabbable, but still isn't keyboard operable without a keypress handler), or <a role="button" href="#" onclick="toggleMenu()">menu</a> (which is tabbable, and operable with enter, but things with the button role should be operable with spacebar too).

It asserts the #main-menu container is visible. But what we really want to know is that the links inside it are visible. During this issue we have encountered various problems with animating the height, and the visiblility of the overflow scrollbar, and the need to do open it in several steps. So whatever JS/CSS implementation we end up with, the desirable outcome to test is that the links themselves are visible, not their container.