From b61dc606c36aebea9855a0e9d74f5afe98c233d7 Mon Sep 17 00:00:00 2001 From: "Jeferson S. Brito" <30840709+jeferson-sb@users.noreply.github.com> Date: Mon, 9 Aug 2021 10:55:41 -0300 Subject: [PATCH] Improve accessibility of top search bar (#14263) Co-authored-by: Suzanne Aitchison Co-authored-by: Nick Taylor --- app/assets/stylesheets/components/header.scss | 4 ++ app/javascript/Search/Search.jsx | 26 ++----- app/javascript/Search/SearchForm.jsx | 69 ++++++++++++------ .../Search/__stories__/SearchForm.stories.jsx | 3 +- .../Search/__tests__/Search.test.jsx | 72 +++++++------------ .../Search/__tests__/SearchFormSync.test.jsx | 20 ++++-- app/views/layouts/_top_bar.html.erb | 11 ++- .../stories/articles_search/index.html.erb | 11 ++- .../searchFlows/searchForm.spec.js | 11 +++ 9 files changed, 126 insertions(+), 101 deletions(-) create mode 100644 cypress/integration/seededFlows/searchFlows/searchForm.spec.js diff --git a/app/assets/stylesheets/components/header.scss b/app/assets/stylesheets/components/header.scss index 2d907a140..f2037bc6a 100644 --- a/app/assets/stylesheets/components/header.scss +++ b/app/assets/stylesheets/components/header.scss @@ -44,6 +44,10 @@ margin: 0 var(--su-4); display: block; } + + .crayons-btn:focus { + outline: revert; + } } .crayons-header__link { diff --git a/app/javascript/Search/Search.jsx b/app/javascript/Search/Search.jsx index 9352aef6b..4505d74bd 100644 --- a/app/javascript/Search/Search.jsx +++ b/app/javascript/Search/Search.jsx @@ -11,7 +11,6 @@ import { SearchForm } from './SearchForm'; const GLOBAL_MINIMIZE_KEY = 'Digit0'; const GLOBAL_SEARCH_KEY = '/'; -const ENTER_KEY = 'Enter'; export class Search extends Component { constructor(props) { @@ -74,27 +73,21 @@ export class Search extends Component { }; submit = (event) => { - if (hasInstantClick) { - event.preventDefault(); - } - }; + event.preventDefault(); - search(key, searchTerm) { + const { value: searchTerm } = this.searchInputRef.current; const { searchTerm: currentSearchTerm } = this.props; + this.enableSearchPageChecker = false; - if ( - hasInstantClick() && - key === ENTER_KEY && - currentSearchTerm !== searchTerm - ) { + if (hasInstantClick() && searchTerm !== currentSearchTerm) { const { setSearchTerm } = this.props; - setSearchTerm(searchTerm); + preloadSearchResults({ searchTerm }); displaySearchResults({ searchTerm }); } - } + }; componentWillUnmount() { document.removeEventListener('keydown', this.globalKeysListener); @@ -128,13 +121,6 @@ export class Search extends Component { /> { - const { - key, - target: { value }, - } = event; - this.search(key, value); - }} onSubmitSearch={this.submit} ref={this.searchInputRef} /> diff --git a/app/javascript/Search/SearchForm.jsx b/app/javascript/Search/SearchForm.jsx index 796bf8234..ab1de7f32 100644 --- a/app/javascript/Search/SearchForm.jsx +++ b/app/javascript/Search/SearchForm.jsx @@ -1,33 +1,56 @@ import PropTypes from 'prop-types'; import { h } from 'preact'; import { forwardRef } from 'preact/compat'; +import { Button } from '@crayons'; -export const SearchForm = forwardRef( - ({ searchTerm, onSearch, onSubmitSearch }, ref) => ( -
- - -
- ), +const SearchIcon = () => ( + + + ); +export const SearchForm = forwardRef(({ searchTerm, onSubmitSearch }, ref) => ( +
+ +
+
+ +
+
+
+)); + SearchForm.propTypes = { searchTerm: PropTypes.string.isRequired, - onSearch: PropTypes.func.isRequired, onSubmitSearch: PropTypes.func.isRequired, }; diff --git a/app/javascript/Search/__stories__/SearchForm.stories.jsx b/app/javascript/Search/__stories__/SearchForm.stories.jsx index 0af65e181..0d6fcadac 100644 --- a/app/javascript/Search/__stories__/SearchForm.stories.jsx +++ b/app/javascript/Search/__stories__/SearchForm.stories.jsx @@ -4,7 +4,6 @@ import { action } from '@storybook/addon-actions'; import { SearchForm } from '..'; const commonProps = { - onSearch: action('on preloading search'), onSubmitSearch: (e) => { e.preventDefault(); action('on submit')(e); @@ -13,7 +12,7 @@ const commonProps = { class FocusedForm extends Component { componentDidMount() { - document.getElementById('nav-search').focus(); + document.querySelector('.crayons-header--search-input').focus(); } render() { diff --git a/app/javascript/Search/__tests__/Search.test.jsx b/app/javascript/Search/__tests__/Search.test.jsx index e3a3e0a0c..7ca71a23a 100644 --- a/app/javascript/Search/__tests__/Search.test.jsx +++ b/app/javascript/Search/__tests__/Search.test.jsx @@ -1,5 +1,6 @@ import { h } from 'preact'; -import { render, fireEvent } from '@testing-library/preact'; +import { render, fireEvent, waitFor } from '@testing-library/preact'; +import userEvent from '@testing-library/user-event'; import { axe } from 'jest-axe'; import { Search } from '../Search'; @@ -34,9 +35,9 @@ describe('', () => { setSearchTerm: jest.fn(), }; - const { getByLabelText } = render(); + const { getByRole } = render(); - const searchInput = getByLabelText(/search/i); + const searchInput = getByRole('textbox', { name: /search/i }); expect(searchInput.value).toEqual('fish'); expect(searchInput.getAttribute('placeholder')).toEqual('Search...'); @@ -48,23 +49,15 @@ describe('', () => { searchTerm: 'fish', setSearchTerm: jest.fn(), }; - const { getByLabelText, findByLabelText } = render(); + const { getByRole, findByRole } = render(); - let searchInput = getByLabelText(/search/i); + let searchInput = getByRole('textbox', { name: /search/i }); expect(searchInput.value).toEqual('fish'); - // user.type doesn't work in the case of - // search as the current implementation is relying on keydown - // events - fireEvent.keyDown(searchInput, { - key: 'Enter', - keyCode: 13, - which: 13, - target: { value: 'hello' }, - }); + fireEvent.change(searchInput, { target: { value: 'hello' } }); - searchInput = await findByLabelText(/search/i); + searchInput = await findByRole('textbox', { name: /search/i }); expect(searchInput.value).toEqual('hello'); }); @@ -74,53 +67,42 @@ describe('', () => { searchTerm: '', setSearchTerm: jest.fn(), }; - const { getByLabelText } = render(); + const { getByRole } = render(); - let searchInput = getByLabelText(/search/i); + const searchInput = getByRole('textbox', { name: /search/i }); expect(searchInput.value).toEqual(''); - // user.type doesn't work in the case of - // search as the current implementation is relying on keydown - // events - fireEvent.keyDown(searchInput, { - key: 'Enter', - keyCode: 13, - which: 13, - target: { value: 'hello' }, - }); + userEvent.type(searchInput, 'hello'); - expect(searchInput.value).toEqual('hello'); - expect(props.setSearchTerm).toHaveBeenCalledWith('hello'); + waitFor(() => { + expect(searchInput.value).toEqual('hello'); + expect(props.setSearchTerm).toHaveBeenCalledWith('hello'); + }); }); it('should submit the search form', async () => { - jest.spyOn(Search.prototype, 'search'); - const props = { searchTerm: '', setSearchTerm: jest.fn(), + onSubmitSearch: jest.fn(), }; - const { getByLabelText, findByLabelText } = render(); + const { getByRole, findByRole } = render(); - let searchInput = getByLabelText(/search/i); + let searchInput = getByRole('textbox', { name: /search/i }); expect(searchInput.value).toEqual(''); - // user.type doesn't work in the case of - // search as the current implementation is relying on keydown - // events - fireEvent.keyDown(searchInput, { - key: 'Enter', - keyCode: 13, - which: 13, - target: { value: 'hello' }, + userEvent.type(searchInput, 'hello'); + + fireEvent.submit(getByRole('search')); + + searchInput = await findByRole('textbox', { name: /search/i }); + + waitFor(() => { + expect(searchInput.value).toEqual('hello'); + expect(props.onSubmitSearch).toHaveBeenCalledWith('hello'); }); - - searchInput = await findByLabelText(/search/i); - - expect(searchInput.value).toEqual('hello'); - expect(Search.prototype.search).toHaveBeenCalledWith('Enter', 'hello'); }); it('should be listening for history state changes', async () => { diff --git a/app/javascript/Search/__tests__/SearchFormSync.test.jsx b/app/javascript/Search/__tests__/SearchFormSync.test.jsx index 740c4b987..428fcd27f 100644 --- a/app/javascript/Search/__tests__/SearchFormSync.test.jsx +++ b/app/javascript/Search/__tests__/SearchFormSync.test.jsx @@ -28,11 +28,11 @@ describe('', () => { }); it('should synchronize search forms', async () => { - const { findByLabelText, findAllByLabelText } = render(); + const { getByRole, getAllByRole } = render(); // Only one input is rendered at this point because the synchSearchForms custom event is what // tells us that there is a new search form to sync with the existing one. - const searchInput = await findByLabelText('search'); + const searchInput = await getByRole('textbox', { name: /search/i }); // Because window.location has no search term in it's URL expect(searchInput.value).toEqual(''); @@ -50,18 +50,20 @@ describe('', () => { }), ); - const [desktopSearch, mobileSearch] = await findAllByLabelText('search'); + const [desktopSearch, mobileSearch] = await getAllByRole('textbox', { + name: /search/i, + }); expect(desktopSearch.value).toEqual(searchTerm); expect(mobileSearch.value).toEqual(searchTerm); }); it('should synchronize search forms on a subsequent search', async () => { - const { findByLabelText, findAllByLabelText } = render(); + const { getByRole, getAllByRole } = render(); // Only one input is rendered at this point because the synchSearchForms custom event is what // tells us that there is a new search form to sync with the existing one. - const searchInput = await findByLabelText('search'); + const searchInput = await getByRole('textbox', { name: /search/i }); // Because window.location has no search term in it's URL expect(searchInput.value).toEqual(''); @@ -79,7 +81,9 @@ describe('', () => { }), ); - let [desktopSearch, mobileSearch] = await findAllByLabelText('search'); + let [desktopSearch, mobileSearch] = await getAllByRole('textbox', { + name: /search/i, + }); expect(desktopSearch.value).toEqual(searchTerm); expect(mobileSearch.value).toEqual(searchTerm); @@ -107,7 +111,9 @@ describe('', () => { }), ); - [desktopSearch, mobileSearch] = await findAllByLabelText('search'); + [desktopSearch, mobileSearch] = await getAllByRole('textbox', { + name: /search/i, + }); expect(desktopSearch.value).toEqual(searchTerm2); expect(mobileSearch.value).toEqual(searchTerm2); diff --git a/app/views/layouts/_top_bar.html.erb b/app/views/layouts/_top_bar.html.erb index 51357cb51..d38b10cfd 100644 --- a/app/views/layouts/_top_bar.html.erb +++ b/app/views/layouts/_top_bar.html.erb @@ -20,8 +20,15 @@
-
- + +
+
+ + +
+
diff --git a/app/views/stories/articles_search/index.html.erb b/app/views/stories/articles_search/index.html.erb index 4ec7a9aeb..dee26fb51 100644 --- a/app/views/stories/articles_search/index.html.erb +++ b/app/views/stories/articles_search/index.html.erb @@ -6,8 +6,15 @@
-
- + +
+
+ + +
+
diff --git a/cypress/integration/seededFlows/searchFlows/searchForm.spec.js b/cypress/integration/seededFlows/searchFlows/searchForm.spec.js new file mode 100644 index 000000000..9ad2734da --- /dev/null +++ b/cypress/integration/seededFlows/searchFlows/searchForm.spec.js @@ -0,0 +1,11 @@ +describe('Search form', () => { + it('should show results when search button is pressed', () => { + cy.visit('/'); + + cy.findByRole('textbox', { name: /search/i }).type('test'); + cy.findByRole('button', { name: /search/i }).click(); + + cy.url().should('include', '/search?q=test'); + cy.findByRole('heading', { name: 'Test article' }).should('exist'); + }); +});