From 0f812d8dd926744f0e379df9791c31a606c5fd96 Mon Sep 17 00:00:00 2001
From: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Date: Tue, 9 Nov 2021 16:34:50 +0000
Subject: [PATCH 1/5] Move label in header to from list to nav
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Move the 'Navigation menu' label from the `<ul>` to the parent `<nav>` element.

This has previously been flagged as a 'possible misuse of `aria-label`.'

According to WAI ARIA Practises 1.2 [2], it's recommended to label navigation regions, especially as 'if a page includes more than one navigation landmark, each should have a unique label' [3].

Lists can optionally be labelled, but this is only '_potentially_ beneficial for users of screen readers that support both list names and navigation among lists on a page'.

Applying the label to both the list and the parent navigation could be noisy – "Potentially a source of distracting or undesirable screen reader verbosity, especially if nested within a named container, such as a navigation region."

The provided examples [4] also apply the label consistently to the navigation region (albeit using `aria-labelledby`)

[1]: https://github.com/alphagov/govuk-frontend/issues/1340#issue-443492815
[2]: https://www.w3.org/TR/wai-aria-practices-1.1/#naming_role_guidance
[3]: https://www.w3.org/TR/wai-aria-practices-1.1/#aria_lh_navigation
[4]: https://www.w3.org/TR/wai-aria-practices-1.1/examples/landmarks/navigation.html
---
 src/govuk/components/header/template.njk     | 4 ++--
 src/govuk/components/header/template.test.js | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/govuk/components/header/template.njk b/src/govuk/components/header/template.njk
index b54d9cc423..c0f9b0ab55 100644
--- a/src/govuk/components/header/template.njk
+++ b/src/govuk/components/header/template.njk
@@ -55,8 +55,8 @@
     {% 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 navigation menu') }}">Menu</button>
-    <nav>
-      <ul id="navigation" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}" aria-label="{{ params.navigationLabel | default('Navigation menu') }}">
+    <nav aria-label="{{ params.navigationLabel | default('Navigation menu') }}">
+      <ul id="navigation" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}">
         {% 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 }}">
diff --git a/src/govuk/components/header/template.test.js b/src/govuk/components/header/template.test.js
index 3446d6f972..da864590c5 100644
--- a/src/govuk/components/header/template.test.js
+++ b/src/govuk/components/header/template.test.js
@@ -121,18 +121,18 @@ describe('header', () => {
       const $ = render('header', examples['with navigation'])
 
       const $component = $('.govuk-header')
-      const $list = $component.find('ul.govuk-header__navigation')
+      const $nav = $component.find('nav')
 
-      expect($list.attr('aria-label')).toEqual('Navigation menu')
+      expect($nav.attr('aria-label')).toEqual('Navigation menu')
     })
 
     it('allows navigation label to be customised', () => {
       const $ = render('header', examples['with custom navigation label'])
 
       const $component = $('.govuk-header')
-      const $list = $component.find('ul.govuk-header__navigation')
+      const $nav = $component.find('nav')
 
-      expect($list.attr('aria-label')).toEqual('Custom navigation label')
+      expect($nav.attr('aria-label')).toEqual('Custom navigation label')
     })
 
     it('renders navigation with active item', () => {

From aed4b3e60f991d47595f4af7a7d03c8ff7f22455 Mon Sep 17 00:00:00 2001
From: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Date: Tue, 9 Nov 2021 16:39:14 +0000
Subject: [PATCH 2/5] Remove redundant 'navigation' from landmark name
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In line with the section 'Composing Effective and User-friendly Accessible Names' in WAI-ARIA Authoring Practices 1.2 [1], remove the word navigation from the aria-label on the navigation.

'Do NOT include a WAI-ARIA role name in the accessible name. For example, do not include [..] the word "navigation" in the name of a navigation region. Doing so would create duplicate screen reader output since screen readers convey the role of an element in addition to its name.'

Without this change, assistive technologies report this as 'Navigation menu navigation'.

Also remove the word 'navigation' from the button that shows or hides the menu, as it also seems redundant – the word 'menu' should be descriptive enough for users to understand what it represents, especially when encountered inside a header landmark.

[1]: https://www.w3.org/TR/wai-aria-practices-1.2/#naming_effectively
---
 src/govuk/components/header/template.njk     | 4 ++--
 src/govuk/components/header/template.test.js | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/govuk/components/header/template.njk b/src/govuk/components/header/template.njk
index c0f9b0ab55..18995cd05e 100644
--- a/src/govuk/components/header/template.njk
+++ b/src/govuk/components/header/template.njk
@@ -54,8 +54,8 @@
     </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 navigation menu') }}">Menu</button>
-    <nav aria-label="{{ params.navigationLabel | default('Navigation 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>
+    <nav aria-label="{{ params.navigationLabel | default('Menu') }}">
       <ul id="navigation" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}">
         {% for item in params.navigation %}
           {% if item.text or item.html %}
diff --git a/src/govuk/components/header/template.test.js b/src/govuk/components/header/template.test.js
index da864590c5..4a5b185339 100644
--- a/src/govuk/components/header/template.test.js
+++ b/src/govuk/components/header/template.test.js
@@ -123,7 +123,7 @@ describe('header', () => {
       const $component = $('.govuk-header')
       const $nav = $component.find('nav')
 
-      expect($nav.attr('aria-label')).toEqual('Navigation menu')
+      expect($nav.attr('aria-label')).toEqual('Menu')
     })
 
     it('allows navigation label to be customised', () => {
@@ -191,7 +191,7 @@ describe('header', () => {
 
         const $button = $('.govuk-header__menu-button')
 
-        expect($button.attr('aria-label')).toEqual('Show or hide navigation menu')
+        expect($button.attr('aria-label')).toEqual('Show or hide menu')
       })
       it('allows label to be customised', () => {
         const $ = render('header', examples['with custom menu button label'])

From 53a6c00f44a9ca6fed4bb77888a0ce4c902eb312 Mon Sep 17 00:00:00 2001
From: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Date: Wed, 1 Dec 2021 11:50:16 +0000
Subject: [PATCH 3/5] 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
---
 src/govuk/components/header/template.njk | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/govuk/components/header/template.njk b/src/govuk/components/header/template.njk
index 18995cd05e..5dcbc8a6d6 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 aria-label="{{ params.navigationLabel | default('Menu') }}">
-      <ul id="navigation" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}">
+      <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 {{ params.navigationClasses if params.navigationClasses }}" >
         {% 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 }}">

From e0e4731b6bdd9b1f72eca507d85c4ded2e93ba4a Mon Sep 17 00:00:00 2001
From: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Date: Thu, 2 Dec 2021 09:54:14 +0000
Subject: [PATCH 4/5] Document in CHANGELOG

Co-authored-by: EoinShaughnessy <72507742+EoinShaughnessy@users.noreply.github.com>
---
 CHANGELOG.md | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9ef9af36b3..7ebeef00e6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -63,6 +63,19 @@ Do not include an empty `<span class="govuk-summary-list__actions"></span>` with
 
 This change was introduced in [pull request #2323: Avoid invalid nesting of `<span>` within a `<dd>` in summary list](https://github.com/alphagov/govuk-frontend/pull/2323).
 
+#### Update the HTML for navigation in the header
+
+We've updated the HTML for the header. This update only affects you if your header includes navigation.
+
+If you're not using Nunjucks macros, then you should move:
+
+- the `<button>` inside the `<nav>`, immediately before the `<ul>`
+- the `aria-label` from the `<ul>` to the `<nav>`
+
+Check your changes against the [header example in the Design System](https://design-system.service.gov.uk/components/header/#header-with-service-name-and-navigation).
+
+This change was introduced in [pull request #2427: Improve the screen reader experience for the header menu](https://github.com/alphagov/govuk-frontend/pull/2427).
+
 #### Make sure components that conditionally reveal other questions still work as expected
 
 On radios and checkboxes, the JavaScript now looks within the whole page for conditionally revealed content. Before, it only looked within the same set of radios or checkboxes.

From d085e96d72fe9264c50f52d8b9415087acb31e1f Mon Sep 17 00:00:00 2001
From: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Date: Fri, 3 Dec 2021 14:53:08 +0000
Subject: [PATCH 5/5] Move navigation classes to the `<nav>`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Move the `govuk-header__navigation` class and any additional classes provided by the `navigationClasses` param to the `<nav>` element rather than the list.

Create a new `govuk-header__navigation-list` for the list, and move the CSS properties that reset the user-agent default list styles and the properties that show and hide the list to this new class.

The `--open` modifier class is now applied to the list (the nav itself now contains the menu button is always visible) so rename it so that it's clear it's applied to the list rather than the nav.

We're making this change because it's more 'correct' and less 'surprising' – users would logically expect the `navigation` BEM element and the `navigationClasses` to be applied to the nav element itself.

There may be a need to allow users to pass classes to apply to the list – but we can do that if and when it comes up. In the meantime, users will still be able to style the list by passing a class `navigationClasses` and targeting the `ul` as a descendant.
---
 CHANGELOG.md                                 |  2 ++
 src/govuk/components/header/_index.scss      | 16 +++++++++-------
 src/govuk/components/header/header.js        |  4 ++--
 src/govuk/components/header/header.test.js   |  8 ++++----
 src/govuk/components/header/template.njk     |  4 ++--
 src/govuk/components/header/template.test.js |  2 +-
 6 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7ebeef00e6..4191ddba4c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -67,6 +67,8 @@ This change was introduced in [pull request #2323: Avoid invalid nesting of `<sp
 
 We've updated the HTML for the header. This update only affects you if your header includes navigation.
 
+Any additional classes passed using the `navigationClasses` Nunjucks option are now applied to the `<nav>` rather than the `<ul>`. Check that the additional classes are still doing what you expect.
+
 If you're not using Nunjucks macros, then you should move:
 
 - the `<button>` inside the `<nav>`, immediately before the `<ul>`
diff --git a/src/govuk/components/header/_index.scss b/src/govuk/components/header/_index.scss
index 67ac75469b..9f0c241747 100644
--- a/src/govuk/components/header/_index.scss
+++ b/src/govuk/components/header/_index.scss
@@ -206,16 +206,18 @@
   }
 
   .govuk-header__navigation {
-    display: block;
-    margin: 0;
-    padding: 0;
-    list-style: none;
-
     @include govuk-media-query ($from: desktop) {
       margin-bottom: govuk-spacing(2);
     }
   }
 
+  .govuk-header__navigation-list {
+    // Reset user-agent default list styles
+    margin: 0;
+    padding: 0;
+    list-style: none;
+  }
+
   .js-enabled {
     .govuk-header__menu-button {
       display: block;
@@ -224,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..6a546a205f 100644
--- a/src/govuk/components/header/header.test.js
+++ b/src/govuk/components/header/header.test.js
@@ -68,8 +68,8 @@ describe('Header navigation', () => {
       })
 
       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')
+        const hasOpenClass = await page.$eval('.govuk-header__navigation-list',
+          el => el.classList.contains('govuk-header__navigation-list--open')
         )
 
         expect(hasOpenClass).toBeTruthy()
@@ -102,8 +102,8 @@ describe('Header navigation', () => {
       })
 
       it('removes the --open modifier class from the menu, hiding it', async () => {
-        const hasOpenClass = await page.$eval('.govuk-header__navigation',
-          el => el.classList.contains('govuk-header__navigation--open')
+        const hasOpenClass = await page.$eval('.govuk-header__navigation-list',
+          el => el.classList.contains('govuk-header__navigation-list--open')
         )
 
         expect(hasOpenClass).toBeFalsy()
diff --git a/src/govuk/components/header/template.njk b/src/govuk/components/header/template.njk
index 5dcbc8a6d6..96dbed671a 100644
--- a/src/govuk/components/header/template.njk
+++ b/src/govuk/components/header/template.njk
@@ -54,10 +54,10 @@
     </a>
     {% endif %}
     {% if params.navigation %}
-    <nav aria-label="{{ params.navigationLabel | default('Menu') }}">
+    <nav aria-label="{{ params.navigationLabel | default('Menu') }}" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}">
       <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 {{ params.navigationClasses if params.navigationClasses }}" >
+      <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 }}">
diff --git a/src/govuk/components/header/template.test.js b/src/govuk/components/header/template.test.js
index 4a5b185339..c0b884140f 100644
--- a/src/govuk/components/header/template.test.js
+++ b/src/govuk/components/header/template.test.js
@@ -109,7 +109,7 @@ describe('header', () => {
       const $ = render('header', examples['with navigation'])
 
       const $component = $('.govuk-header')
-      const $list = $component.find('ul.govuk-header__navigation')
+      const $list = $component.find('ul.govuk-header__navigation-list')
       const $items = $list.find('li.govuk-header__navigation-item')
       const $firstItem = $items.find('a.govuk-header__link:first-child')
       expect($items.length).toEqual(4)