Header: Fix issues with search toggle in header

1. Was using title rather than aria-label for the description 2. Was missing a type attribute on the button element 3. Was missing aria-controls to associate the button with the search 4. Both the toggle and the search form were using icon fonts which causes additional text to be read out when interacting with either element. Switch to use SVG icons
citizensadvice · Jun 13, 2022 · c0cd9a7 · c0cd9a7
1 parent edb4d58
commit c0cd9a7
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 62 deletions.
diff --git a/engine/app/components/citizens_advice_components/header.html.haml b/engine/app/components/citizens_advice_components/header.html.haml
@@ -9,12 +9,15 @@
       .cads-grid-col-md-2.cads-header__logo-row
         = logo
         - if search_form.present?
-          = tag.button(class: "cads-header__search-reveal js-cads-search-reveal cads-icon_search",
-            title: t(".open_search"),
-            "aria-expanded": "false",
+          = tag.button(class: "cads-header__search-reveal js-cads-search-reveal",
+            type: "button",
+            "aria-label": t(".open_search"),
+            "aria-controls": "header-search-form",
             "data-testid": "expand-button",
             "data-descriptive-label-show": t(".open_search"),
-            "data-descriptive-label-hide": t(".close_search"))
+            "data-descriptive-label-hide": t(".close_search")) do
+            = render CitizensAdviceComponents::Icons::Search.new
+            = render CitizensAdviceComponents::Icons::Close.new
 
       .cads-grid-col-md-10.cads-header__search-row
         %ul.cads-header__links.js-cads-copy-into-nav
@@ -25,7 +28,7 @@
             %li.cads-header__links-item= account_link
 
         - if search_form.present?
-          .cads-header__search-form
+          .cads-header__search-form#header-search-form
             = form_tag(search_form.search_action_url,
               method: :get,
               role: "search",
@@ -38,4 +41,5 @@
                 title: t("citizens_advice_components.search.submit_title"),
                 "data-testid": "search-button",
                 class: "cads-search__button") do
+                = render CitizensAdviceComponents::Icons::Search.new
                 %span.cads-search__button-label= t("citizens_advice_components.search.submit_label")
diff --git a/engine/spec/components/citizens_advice_components/header_spec.rb b/engine/spec/components/citizens_advice_components/header_spec.rb
@@ -156,7 +156,7 @@
     end
 
     it "renders search toggle" do
-      expect(component.at("button[title='Open search']")).to be_present
+      expect(component.at("button[aria-label='Open search']")).to be_present
     end
 
     context "when welsh language" do
@@ -167,7 +167,7 @@
       end
 
       it "has translated search label" do
-        expect(component.at("button[title='Ymchwiliad agored']")).to be_present
+        expect(component.at("button[aria-label='Ymchwiliad agored']")).to be_present
       end
     end
   end

diff --git a/scss/6-components/_header.scss b/scss/6-components/_header.scss
@@ -2,7 +2,7 @@
 // Header
 // ============================================================================
 
-/** @define header */
+/** @define header; weak */
 .cads-header {
   background-color: $cads-header__background-colour;
   padding: $cads-spacing-4 0;
@@ -41,14 +41,21 @@
   &__search-reveal {
     @include cads-button-defaults();
 
-    display: none;
+    display: none; // Hide by default
+    width: $cads-interactive-target-size;
+    height: $cads-interactive-target-size;
     margin: 0;
-    padding: 11px 12px 11px 12px;
+    padding: 0;
     color: $cads-language__primary-button-colour;
     background: transparent;
     border: solid $cads-border-width-medium
       $cads-language__primary-button-colour;
 
+    .cads-icon {
+      display: inline-block;
+      vertical-align: middle;
+    }
+
     &:hover,
     &:active {
       color: $cads-language__primary-button-hover-colour;
@@ -114,7 +121,18 @@
 
     .cads-header__search-reveal {
       display: inline-block;
-      width: $cads-interactive-target-size;
+
+      .cads-icon {
+        height: 18px;
+      }
+
+      .cads-icon--search {
+        display: inline-block;
+      }
+
+      .cads-icon--close {
+        display: none;
+      }
     }
 
     .cads-header__links {
@@ -147,6 +165,16 @@
   .cads-header__search-row {
     display: block;
   }
+
+  .cads-header__search-reveal {
+    .cads-icon--search {
+      display: none;
+    }
+
+    .cads-icon--close {
+      display: inline-block;
+    }
+  }
 }
 
 /* stylelint-disable no-descending-specificity */

diff --git a/scss/6-components/_search.scss b/scss/6-components/_search.scss
@@ -2,7 +2,7 @@
 // Search
 // ============================================================================
 
-/** @define search */
+/** @define search; weak */
 .cads-search {
   position: relative;
   white-space: nowrap;
@@ -53,22 +53,17 @@
 }
 
 ///
-// Modifier:
+// Modifier: Icon only
 ///
 .cads-search--icon-only {
-  @include cads-media-breakpoint-only(sm) {
-    .cads-search__button {
-      &::before {
-        display: inline;
-        content: '\004b';
-        font-family: 'cads';
-        font-weight: normal;
-      }
+  .cads-icon--search {
+    display: none;
+  }
 
-      &::after {
-        display: none;
-        content: '';
-      }
+  @include cads-media-breakpoint-only(sm) {
+    .cads-icon--search {
+      display: inline-block;
+      height: 20px;
     }
 
     .cads-search__button-label {

diff --git a/src/ts/header/header.test.ts b/src/ts/header/header.test.ts
@@ -20,12 +20,12 @@ const minimalHeaderHtml = `<header class='cads-header' data-testid="header">
       <div class='cads-grid-col-md-5 cads-header__logo-row'>
         <a class='cads-logo' href='root_path'
           title='Citizens Advice homepage'></a>
-        <button aria-expanded="false" 
-          class="cads-header__search-reveal js-cads-search-reveal cads-icon_search" 
-          data-descriptive-label-hide="Close search" 
-          data-descriptive-label-show="Open search" 
-          data-testid="expand-button" 
-          title="Open search"></button>
+        <button type="button"
+          class="cads-header__search-reveal js-cads-search-reveal cads-icon_search"
+          data-descriptive-label-hide="Close search"
+          data-descriptive-label-show="Open search"
+          data-testid="expand-button"
+          aria-label="Open search"></button>
       </div>
       <div class='cads-grid-col-md-7 cads-header__search-row'>
         ${searchFormHtml}
@@ -39,18 +39,17 @@ test('allow toggling search', () => {
   initHeader();
 
   const headerEl = screen.getByTestId('header');
-  const controlButtonEl = screen.getByTitle('Open search');
+  const controlButtonEl = screen.getByTestId('expand-button');
 
   expect(headerEl).not.toHaveClass('cads-header--show-search');
+  expect(controlButtonEl).toHaveAttribute('aria-label', 'Open search');
   expect(controlButtonEl).toHaveAttribute('aria-expanded', 'false');
-  expect(controlButtonEl).toHaveClass('cads-icon_search');
 
   controlButtonEl.click();
 
   expect(headerEl).toHaveClass('cads-header--show-search');
-  expect(controlButtonEl.title).toBe('Close search');
+  expect(controlButtonEl).toHaveAttribute('aria-label', 'Close search');
   expect(controlButtonEl).toHaveAttribute('aria-expanded', 'true');
-  expect(controlButtonEl).toHaveClass('cads-icon_close');
 });
 
 test('only initialises when header is present', () => {

diff --git a/src/ts/header/header.ts b/src/ts/header/header.ts
@@ -1,38 +1,44 @@
 const initHeader = (): void => {
-  const CLASS_NAMES = {
-    showSearch: 'cads-header--show-search',
-    iconSearch: 'cads-icon_search',
-    iconClose: 'cads-icon_close',
-  };
+  const SELECTOR = '.js-cads-search-reveal';
+  const SHOW_SEARCH_CLASS_NAME = 'cads-header--show-search';
 
   const header = document.querySelector('.cads-header');
 
   if (header) {
-    const controlButton = header.querySelector(
-      '.js-cads-search-reveal'
-    ) as HTMLButtonElement;
+    const controlButton = header.querySelector(SELECTOR) as HTMLButtonElement;
+
+    const setClosed = () => {
+      header.classList.remove(SHOW_SEARCH_CLASS_NAME);
+      controlButton.setAttribute('aria-expanded', 'false');
+
+      const showLabel = controlButton.getAttribute(
+        'data-descriptive-label-show'
+      );
+      if (showLabel) {
+        controlButton.setAttribute('aria-label', showLabel);
+      }
+    };
+
+    const setOpen = () => {
+      header.classList.add(SHOW_SEARCH_CLASS_NAME);
+      controlButton.setAttribute('aria-expanded', 'true');
+
+      const hideLabel = controlButton.getAttribute(
+        'data-descriptive-label-hide'
+      );
+      if (hideLabel) {
+        controlButton.setAttribute('aria-label', hideLabel);
+      }
+    };
+
+    // Set initial control state on init
+    setClosed();
 
     controlButton.addEventListener('click', () => {
-      if (header.classList.contains(CLASS_NAMES.showSearch)) {
-        header.classList.remove(CLASS_NAMES.showSearch);
-        controlButton.classList.remove(CLASS_NAMES.iconClose);
-        controlButton.classList.add(CLASS_NAMES.iconSearch);
-        controlButton.setAttribute('aria-expanded', 'false');
-
-        const title = controlButton.getAttribute('data-descriptive-label-show');
-        if (title) {
-          controlButton.title = title;
-        }
+      if (header.classList.contains(SHOW_SEARCH_CLASS_NAME)) {
+        setClosed();
       } else {
-        header.classList.add(CLASS_NAMES.showSearch);
-        controlButton.classList.remove(CLASS_NAMES.iconSearch);
-        controlButton.classList.add(CLASS_NAMES.iconClose);
-        controlButton.setAttribute('aria-expanded', 'true');
-
-        const title = controlButton.getAttribute('data-descriptive-label-hide');
-        if (title) {
-          controlButton.title = title;
-        }
+        setOpen();
       }
     });
   }