From 537651db9ddd995b9fa440381a1d483bc25e95eb Mon Sep 17 00:00:00 2001
From: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Date: Tue, 23 Nov 2021 11:55:26 +0000
Subject: [PATCH] Move button inside navigation region
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Move the menu button inside the navigation region, so that it is discoverable by screen reader users even when the nav is closed:

> Place mobile open/close buttons within the <nav> element and use them to toggle state of another child wrapper of the nav. This will ensure that the navigation landmark is still discoverable by screen readers, even if it is in a closed/hidden state.

https://a11y-style-guide.com/style-guide/section-navigation.html#kssref-navigation-navigation-mobile

> Where a lot of people go wrong is by placing the button outside the region. This would mean screen reader users who move to the <nav> using a shortcut would find it to be empty, which isn’t very helpful.

https://inclusive-components.design/menus-menu-buttons/#placement

> For instance, a navigation on small screen devices may initially contain only a single toggle button. But that toggle button would reveal additional links and other navigational elements, when activated.

https://www.scottohara.me/blog/2018/03/03/landmarks.html#navigation

This means we need to make the nav region always visible, so that the menu button is always visible (on mobile) – and therefore we need to revert some of the changes from 02ce19f:

- toggling the visibility of the list rather than the nav
- moving the aria-controls assocation back to the list, by moving the `navigation` id in the HTML
---
 src/govuk/components/header/_index.scss    | 4 ++--
 src/govuk/components/header/header.js      | 4 ++--
 src/govuk/components/header/header.test.js | 6 +++---
 src/govuk/components/header/template.njk   | 7 ++++---
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/src/govuk/components/header/_index.scss b/src/govuk/components/header/_index.scss
index 7f67903dcf..684095fc69 100644
--- a/src/govuk/components/header/_index.scss
+++ b/src/govuk/components/header/_index.scss
@@ -226,14 +226,14 @@
       }
     }
 
-    .govuk-header__navigation {
+    .govuk-header__navigation-list {
       display: none;
       @include govuk-media-query ($from: desktop) {
         display: block;
       }
     }
 
-    .govuk-header__navigation--open {
+    .govuk-header__navigation-list--open {
       display: block;
     }
   }
diff --git a/src/govuk/components/header/header.js b/src/govuk/components/header/header.js
index ca2151941d..cf92b3908f 100644
--- a/src/govuk/components/header/header.js
+++ b/src/govuk/components/header/header.js
@@ -21,7 +21,7 @@ Header.prototype.init = function () {
     return
   }
 
-  this.syncState(this.$menu.classList.contains('govuk-header__navigation--open'))
+  this.syncState(this.$menu.classList.contains('govuk-header__navigation-list--open'))
   this.$menuButton.addEventListener('click', this.handleMenuButtonClick.bind(this))
 }
 
@@ -45,7 +45,7 @@ Header.prototype.syncState = function (isVisible) {
  * sync the accessibility state and menu button state
  */
 Header.prototype.handleMenuButtonClick = function () {
-  var isVisible = this.$menu.classList.toggle('govuk-header__navigation--open')
+  var isVisible = this.$menu.classList.toggle('govuk-header__navigation-list--open')
   this.syncState(isVisible)
 }
 
diff --git a/src/govuk/components/header/header.test.js b/src/govuk/components/header/header.test.js
index 267154bf00..fa952803d5 100644
--- a/src/govuk/components/header/header.test.js
+++ b/src/govuk/components/header/header.test.js
@@ -67,9 +67,9 @@ describe('Header navigation', () => {
         await page.click('.govuk-js-header-toggle')
       })
 
-      it('adds the --open modifier class to the menu, making it visible', async () => {
-        const hasOpenClass = await page.$eval('.govuk-header__navigation',
-          el => el.classList.contains('govuk-header__navigation--open')
+      it('adds the --open modifier class to the list, making it visible', async () => {
+        const hasOpenClass = await page.$eval('.govuk-header__navigation-list',
+          el => el.classList.contains('govuk-header__navigation-list--open')
         )
 
         expect(hasOpenClass).toBeTruthy()
diff --git a/src/govuk/components/header/template.njk b/src/govuk/components/header/template.njk
index 602a7bda1c..99dc06ebe8 100644
--- a/src/govuk/components/header/template.njk
+++ b/src/govuk/components/header/template.njk
@@ -54,9 +54,10 @@
     </a>
     {% endif %}
     {% if params.navigation %}
-    <button type="button" class="govuk-header__menu-button govuk-js-header-toggle" aria-controls="navigation" aria-label="{{ params.menuButtonLabel | default('Show or hide menu') }}">Menu</button>
-    <nav id="navigation" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}" aria-label="{{ params.navigationLabel | default('Menu') }}">
-      <ul class="govuk-header__navigation-list">
+    <nav class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}" aria-label="{{ params.navigationLabel | default('Menu') }}">
+      <button type="button" class="govuk-header__menu-button govuk-js-header-toggle" aria-controls="navigation" aria-label="{{ params.menuButtonLabel | default('Show or hide menu') }}">Menu</button>
+
+      <ul id="navigation" class="govuk-header__navigation-list">
         {% for item in params.navigation %}
           {% if item.text or item.html %}
             <li class="govuk-header__navigation-item{{ ' govuk-header__navigation-item--active' if item.active }}">