From db3f1b9cb0014b4decbd88d25d67f7b6f6d5f613 Mon Sep 17 00:00:00 2001 From: Marcos Rigoli Date: Fri, 28 Feb 2025 12:12:39 -0300 Subject: [PATCH] feat: Search a11y updated behavior. (#1614) * feat: Updated accesibility on courseware search modal * chore: Updated i18n to use useIntl() hook and dialog behavior updates * feat: Added close when clicking on the modal backdrop * chore: Swapped remove listeners with an AbortController * fix: Fixed tests * chore: Rolled back unintended husky script change --- .../courseware-search/CoursewareSearch.jsx | 102 +++++++++++------- .../CoursewareSearch.test.jsx | 40 ++++--- .../CoursewareSearchForm.jsx | 59 +++++----- .../courseware-search/courseware-search.scss | 27 ++++- src/course-tabs/CourseTabsNavigation.test.jsx | 4 +- src/setupTest.js | 12 +++ 6 files changed, 151 insertions(+), 93 deletions(-) diff --git a/src/course-home/courseware-search/CoursewareSearch.jsx b/src/course-home/courseware-search/CoursewareSearch.jsx index 4050aebf..15a227c0 100644 --- a/src/course-home/courseware-search/CoursewareSearch.jsx +++ b/src/course-home/courseware-search/CoursewareSearch.jsx @@ -1,8 +1,8 @@ -import React, { useEffect } from 'react'; +import React, { useEffect, useRef } from 'react'; import { useParams } from 'react-router'; import { useDispatch } from 'react-redux'; import { sendTrackingLogEvent } from '@edx/frontend-platform/analytics'; -import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; +import { useIntl } from '@edx/frontend-platform/i18n'; import { Alert, Button, Icon, Spinner, } from '@openedx/paragon'; @@ -18,7 +18,8 @@ import CoursewareSearchResultsFilterContainer from './CoursewareResultsFilter'; import { updateModel, useModel } from '../../generic/model-store'; import { searchCourseContent } from '../data/thunks'; -const CoursewareSearch = ({ intl, ...sectionProps }) => { +const CoursewareSearch = ({ ...sectionProps }) => { + const { formatMessage } = useIntl(); const { courseId } = useParams(); const { query: searchKeyword, setQuery, clearSearchParams } = useCoursewareSearchParams(); const dispatch = useDispatch(); @@ -29,6 +30,7 @@ const CoursewareSearch = ({ intl, ...sectionProps }) => { errors, total, } = useModel('contentSearchResults', courseId); + const dialogRef = useRef(); useLockScroll(); @@ -44,7 +46,8 @@ const CoursewareSearch = ({ intl, ...sectionProps }) => { searchKeyword: '', results: [], errors: undefined, - loading: false, + loading: + false, }, })); }; @@ -66,20 +69,46 @@ const CoursewareSearch = ({ intl, ...sectionProps }) => { setQuery(value); }; - useEffect(() => { - handleSubmit(searchKeyword); - }, []); - const handleOnChange = (value) => { if (value === searchKeyword) { return; } if (!value) { clearSearch(); } }; - const handleSearchCloseClick = () => { + const close = () => { clearSearch(); dispatch(setShowSearch(false)); }; + const handlePopState = () => close(); + + const handleBackdropClick = function (event) { + if (event.target === dialogRef.current) { + dialogRef.current.close(); + } + }; + + useEffect(() => { + // We need this to keep the dialog reference when unmounting. + const dialog = dialogRef.current; + + // Open the dialog as a modal on render to confine focus within it. + dialogRef.current.showModal(); + + if (searchKeyword) { + handleSubmit(searchKeyword); // In case it's opened with a search link, we run the search. + } + + const controller = new AbortController(); + const { signal } = controller; + + window.addEventListener('popstate', handlePopState, { signal }); + dialog.addEventListener('click', handleBackdropClick, { signal }); + + return () => controller.abort(); // Removes event listeners. + }, []); + + const handleSearchClose = () => close(); + let status = 'idle'; if (loading) { status = 'loading'; @@ -90,34 +119,37 @@ const CoursewareSearch = ({ intl, ...sectionProps }) => { } return ( -
-
- -
+
-
-

{intl.formatMessage(messages.searchModuleTitle)}

- +
+
+

{formatMessage(messages.searchModuleTitle)}

+ +
+ +
+
+ {status === 'loading' ? (
- +
) : null} {status === 'error' && ( - {intl.formatMessage(messages.searchResultsError)} + {formatMessage(messages.searchResultsError)} )} {status === 'results' ? ( @@ -129,7 +161,7 @@ const CoursewareSearch = ({ intl, ...sectionProps }) => { aria-relevant="all" aria-atomic="true" data-testid="courseware-search-summary" - >{intl.formatMessage(messages.searchResultsLabel, { total, keyword: lastSearchKeyword })} + >{formatMessage(messages.searchResultsLabel, { total, keyword: lastSearchKeyword })}
) : null} @@ -137,12 +169,8 @@ const CoursewareSearch = ({ intl, ...sectionProps }) => { ) : null}
-
+ ); }; -CoursewareSearch.propTypes = { - intl: intlShape.isRequired, -}; - -export default injectIntl(CoursewareSearch); +export default CoursewareSearch; diff --git a/src/course-home/courseware-search/CoursewareSearch.test.jsx b/src/course-home/courseware-search/CoursewareSearch.test.jsx index 38353cd5..5a0bebd4 100644 --- a/src/course-home/courseware-search/CoursewareSearch.test.jsx +++ b/src/course-home/courseware-search/CoursewareSearch.test.jsx @@ -19,6 +19,7 @@ import { updateModel, useModel } from '../../generic/model-store'; jest.mock('./hooks'); jest.mock('../../generic/model-store', () => ({ + ...jest.requireActual('../../generic/model-store'), updateModel: jest.fn(), useModel: jest.fn(), })); @@ -56,7 +57,7 @@ const defaultProps = { total: 0, }; -const coursewareSearch = { +const defaultSearchParams = { query: '', filter: '', setQuery: jest.fn(), @@ -96,14 +97,20 @@ const mockModels = ((props = defaultProps) => { }); }); -const mockSearchParams = ((props = coursewareSearch) => { +const mockSearchParams = ((params) => { + const props = { ...defaultSearchParams, ...params }; useCoursewareSearchParams.mockReturnValue(props); }); describe('CoursewareSearch', () => { - beforeAll(initializeMockApp); + beforeAll(() => initializeMockApp()); beforeEach(() => { + mockModels(); + mockSearchParams(); + }); + + afterEach(() => { jest.clearAllMocks(); }); @@ -113,27 +120,22 @@ describe('CoursewareSearch', () => { }); it('should use useElementBoundingBox() and useLockScroll() hooks', () => { - mockModels(); - mockSearchParams(); renderComponent(); - expect(useElementBoundingBox).toBeCalledTimes(1); - expect(useLockScroll).toBeCalledTimes(1); + expect(useElementBoundingBox).toHaveBeenCalledTimes(1); + expect(useLockScroll).toHaveBeenCalledTimes(1); }); it('should have a "--modal-top-position" CSS variable matching the CourseTabsNavigation top position', () => { - mockModels(); - mockSearchParams(); renderComponent(); - const section = screen.getByTestId('courseware-search-section'); + const section = screen.getByTestId('courseware-search-dialog'); expect(section.style.getPropertyValue('--modal-top-position')).toBe(`${tabsTopPosition}px`); }); }); describe('when clicking on the "Close" button', () => { - it('should dispatch setShowSearch(false)', async () => { - mockModels(); + it('should close the dialog', async () => { renderComponent(); await waitFor(() => { @@ -141,7 +143,8 @@ describe('CoursewareSearch', () => { fireEvent.click(close); }); - expect(setShowSearch).toBeCalledWith(false); + expect(HTMLDialogElement.prototype.close).toHaveBeenCalled(); + expect(setShowSearch).toHaveBeenCalledWith(false); }); }); @@ -149,29 +152,24 @@ describe('CoursewareSearch', () => { it('should use "--modal-top-position: 0" if nce element is not present', () => { useElementBoundingBox.mockImplementation(() => undefined); - mockModels(); - mockSearchParams(); renderComponent(); - const section = screen.getByTestId('courseware-search-section'); + const section = screen.getByTestId('courseware-search-dialog'); expect(section.style.getPropertyValue('--modal-top-position')).toBe('0'); }); }); describe('when passing extra props', () => { it('should pass on extra props to section element', () => { - mockModels(); - mockSearchParams(); renderComponent({ foo: 'bar' }); - const section = screen.getByTestId('courseware-search-section'); + const section = screen.getByTestId('courseware-search-dialog'); expect(section).toHaveAttribute('foo', 'bar'); }); }); describe('when submitting an empty search', () => { it('should clear the search by dispatch updateModel', async () => { - mockModels(); renderComponent(); await waitFor(() => { @@ -203,7 +201,6 @@ describe('CoursewareSearch', () => { }); it('should call searchCourseContent', async () => { - mockModels(); renderComponent(); const searchKeyword = 'course'; @@ -259,6 +256,7 @@ describe('CoursewareSearch', () => { describe('when clearing the search input', () => { it('should clear the search by dispatch updateModel', async () => { + mockSearchParams({ query: 'fubar' }); mockModels({ searchKeyword: 'fubar', total: 2, diff --git a/src/course-home/courseware-search/CoursewareSearchForm.jsx b/src/course-home/courseware-search/CoursewareSearchForm.jsx index 56d443a4..6d0856fa 100644 --- a/src/course-home/courseware-search/CoursewareSearchForm.jsx +++ b/src/course-home/courseware-search/CoursewareSearchForm.jsx @@ -1,43 +1,44 @@ -import React from 'react'; import PropTypes from 'prop-types'; import { SearchField } from '@openedx/paragon'; -import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; +import { useIntl } from '@edx/frontend-platform/i18n'; import messages from './messages'; const CoursewareSearchForm = ({ - intl, searchTerm, onSubmit, onChange, placeholder, -}) => ( - -
- - - -
- { + const { formatMessage } = useIntl(); + + return ( + - -); + className="courseware-search-form" + screenReaderText={{ + label: formatMessage(messages.searchSubmitLabel), + clearButton: formatMessage(messages.searchClearAction), + submitButton: null, // Remove the sr-only label in the button. + }} + > +
+ + + +
+ + + ); +}; CoursewareSearchForm.propTypes = { - intl: intlShape.isRequired, searchTerm: PropTypes.string, onSubmit: PropTypes.func, onChange: PropTypes.func, @@ -51,4 +52,4 @@ CoursewareSearchForm.defaultProps = { placeholder: undefined, }; -export default injectIntl(CoursewareSearchForm); +export default CoursewareSearchForm; diff --git a/src/course-home/courseware-search/courseware-search.scss b/src/course-home/courseware-search/courseware-search.scss index 3066dcfb..3e17d174 100644 --- a/src/course-home/courseware-search/courseware-search.scss +++ b/src/course-home/courseware-search/courseware-search.scss @@ -5,13 +5,25 @@ left: 0; right: 0; bottom: 0; + width: 100%; + height: 100%; + max-width: none; + margin: 0; border-top: 1px solid $light-300; z-index: $zindex-modal; // Bootstrap's z-index layer for Modals. + &__form { + position: relative; + + .h2 { + margin-right: 2.5rem; + } + } + &__close { position: absolute !important; // For some reason it gets overridden - top: 0.5rem; - right: 1rem; + top: 0; + right: 0; font-size: 1.5rem; line-height: 1; } @@ -152,9 +164,16 @@ } @media (min-width: map-get($grid-breakpoints, 'md')) { - .courseware-search__content { - padding-top: 8rem; + .courseware-search { + &__close { + right: -2.5rem; + } + + &__content { + padding-top: 8rem; + } } + } body._search-no-scroll { diff --git a/src/course-tabs/CourseTabsNavigation.test.jsx b/src/course-tabs/CourseTabsNavigation.test.jsx index a6d3db83..6f458f8f 100644 --- a/src/course-tabs/CourseTabsNavigation.test.jsx +++ b/src/course-tabs/CourseTabsNavigation.test.jsx @@ -73,13 +73,13 @@ describe('Course Tabs Navigation', () => { it('should NOT render CoursewareSearch if the flag is off', () => { renderComponent(); - expect(screen.queryByTestId('courseware-search-section')).not.toBeInTheDocument(); + expect(screen.queryByTestId('courseware-search-dialog')).not.toBeInTheDocument(); }); it('should render CoursewareSearch if the flag is on', () => { useCoursewareSearchState.mockImplementation(() => ({ show: true })); renderComponent(); - expect(screen.queryByTestId('courseware-search-section')).toBeInTheDocument(); + expect(screen.queryByTestId('courseware-search-dialog')).toBeInTheDocument(); }); }); diff --git a/src/setupTest.js b/src/setupTest.js index 02abd12c..9b2d0c5e 100755 --- a/src/setupTest.js +++ b/src/setupTest.js @@ -61,6 +61,18 @@ const supressWarningBlock = (callback) => { }; /* eslint-enable no-console */ +// Mocks for HTML Dialogs behavior. */ +// jsdom does not support HTML Dialogs yet: https://github.com/jsdom/jsdom/issues/3294 +HTMLDialogElement.prototype.show = jest.fn(); +HTMLDialogElement.prototype.showModal = jest.fn(function mock() { + const onShowModal = new CustomEvent('show_modal'); + this.dispatchEvent(onShowModal); +}); +HTMLDialogElement.prototype.close = jest.fn(function mock() { + const onClose = new CustomEvent('close'); + this.dispatchEvent(onClose); +}); + // Mock Intersection Observer which is unavailable in the context of a test. global.IntersectionObserver = jest.fn(function mockIntersectionObserver() { this.observe = jest.fn();