From 5ee61904d5a99b58e4d749d7612bdb88d519f98b Mon Sep 17 00:00:00 2001 From: Mohamed Akram Date: Mon, 14 Aug 2023 21:18:55 +0400 Subject: [PATCH] fix: make nav buttons use links for accessibility (#1137) --- src/courseware/CoursewareContainer.jsx | 13 +---- src/courseware/CoursewareContainer.test.jsx | 6 +-- src/courseware/course/Course.test.jsx | 4 +- src/courseware/course/sequence/Sequence.jsx | 10 +--- .../course/sequence/Sequence.test.jsx | 35 ++++++++----- .../SequenceNavigation.jsx | 51 ++++++++++++++----- .../SequenceNavigation.test.jsx | 35 +++++++------ .../SequenceNavigationDropdown.test.jsx | 4 +- .../SequenceNavigationTabs.test.jsx | 6 +-- .../sequence-navigation/UnitButton.jsx | 7 ++- .../sequence-navigation/UnitButton.test.jsx | 6 +-- .../sequence-navigation/UnitNavigation.jsx | 41 +++++++++------ .../UnitNavigation.test.jsx | 23 ++++----- .../sequence/sequence-navigation/hooks.js | 42 +++++++++++++-- src/product-tours/ProductTours.test.jsx | 2 +- 15 files changed, 172 insertions(+), 113 deletions(-) diff --git a/src/courseware/CoursewareContainer.jsx b/src/courseware/CoursewareContainer.jsx index 34fcc092..0bc792b5 100644 --- a/src/courseware/CoursewareContainer.jsx +++ b/src/courseware/CoursewareContainer.jsx @@ -243,7 +243,7 @@ class CoursewareContainer extends Component { checkSequenceUnitMarkerToSequenceUnitRedirect(courseId, sequenceStatus, sequence, routeUnitId); } - handleUnitNavigationClick = (nextUnitId) => { + handleUnitNavigationClick = () => { const { courseId, sequenceId, match: { @@ -254,21 +254,17 @@ class CoursewareContainer extends Component { } = this.props; this.props.checkBlockCompletion(courseId, sequenceId, routeUnitId); - history.push(`/course/${courseId}/${sequenceId}/${nextUnitId}`); }; handleNextSequenceClick = () => { const { course, - courseId, nextSequence, sequence, sequenceId, } = this.props; if (nextSequence !== null) { - history.push(`/course/${courseId}/${nextSequence.id}/first`); - const celebrateFirstSection = course && course.celebrations && course.celebrations.firstSection; if (celebrateFirstSection && sequence.sectionId !== nextSequence.sectionId) { handleNextSectionCelebration(sequenceId, nextSequence.id); @@ -276,12 +272,7 @@ class CoursewareContainer extends Component { } }; - handlePreviousSequenceClick = () => { - const { previousSequence, courseId } = this.props; - if (previousSequence !== null) { - history.push(`/course/${courseId}/${previousSequence.id}/last`); - } - }; + handlePreviousSequenceClick = () => {}; render() { const { diff --git a/src/courseware/CoursewareContainer.test.jsx b/src/courseware/CoursewareContainer.test.jsx index 72f2fe80..5df0deeb 100644 --- a/src/courseware/CoursewareContainer.test.jsx +++ b/src/courseware/CoursewareContainer.test.jsx @@ -185,7 +185,7 @@ describe('CoursewareContainer', () => { function assertSequenceNavigation(container, expectedUnitCount = 3) { // Ensure we had appropriate sequence navigation buttons. We should only have one unit. - const sequenceNavButtons = container.querySelectorAll('nav.sequence-navigation button'); + const sequenceNavButtons = container.querySelectorAll('nav.sequence-navigation a, nav.sequence-navigation button'); expect(sequenceNavButtons).toHaveLength(expectedUnitCount + 2); expect(sequenceNavButtons[0]).toHaveTextContent('Previous'); @@ -413,10 +413,10 @@ describe('CoursewareContainer', () => { history.push(`/course/${courseId}/${sequenceBlock.id}/${unitBlocks[0].id}`); const container = await waitFor(() => loadContainer()); - const sequenceNavButtons = container.querySelectorAll('nav.sequence-navigation button'); + const sequenceNavButtons = container.querySelectorAll('nav.sequence-navigation a, nav.sequence-navigation button'); const sequenceNextButton = sequenceNavButtons[4]; expect(sequenceNextButton).toHaveTextContent('Next'); - fireEvent.click(sequenceNavButtons[4]); + fireEvent.click(sequenceNextButton); expect(global.location.href).toEqual(`http://localhost/course/${courseId}/${sequenceBlock.id}/${unitBlocks[1].id}`); }); diff --git a/src/courseware/course/Course.test.jsx b/src/courseware/course/Course.test.jsx index 312a7862..6362c0dc 100644 --- a/src/courseware/course/Course.test.jsx +++ b/src/courseware/course/Course.test.jsx @@ -280,8 +280,8 @@ describe('Course', () => { loadUnit(); await waitFor(() => expect(screen.queryByText('Loading learning sequence...')).not.toBeInTheDocument()); - screen.getAllByRole('button', { name: /previous/i }).forEach(button => fireEvent.click(button)); - screen.getAllByRole('button', { name: /next/i }).forEach(button => fireEvent.click(button)); + screen.getAllByRole('link', { name: /previous/i }).forEach(link => fireEvent.click(link)); + screen.getAllByRole('link', { name: /next/i }).forEach(link => fireEvent.click(link)); // We are in the middle of the sequence, so no expect(previousSequenceHandler).not.toHaveBeenCalled(); diff --git a/src/courseware/course/sequence/Sequence.jsx b/src/courseware/course/sequence/Sequence.jsx index faa58c90..529c09b8 100644 --- a/src/courseware/course/sequence/Sequence.jsx +++ b/src/courseware/course/sequence/Sequence.jsx @@ -10,7 +10,6 @@ import { } from '@edx/frontend-platform/analytics'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { useSelector } from 'react-redux'; -import { history } from '@edx/frontend-platform'; import SequenceExamWrapper from '@edx/frontend-lib-special-exams'; import { breakpoints, useWindowSize } from '@edx/paragon'; @@ -139,9 +138,6 @@ const Sequence = ({ } const gated = sequence && sequence.gatedContent !== undefined && sequence.gatedContent.gated; - const goToCourseExitPage = () => { - history.push(`/course/${courseId}/course-end`); - }; const defaultContent = (
@@ -150,7 +146,7 @@ const Sequence = ({ sequenceId={sequenceId} unitId={unitId} className="mb-4" - nextSequenceHandler={() => { + nextHandler={() => { logEvent('edx.ui.lms.sequence.next_selected', 'top'); handleNext(); }} @@ -158,11 +154,10 @@ const Sequence = ({ logEvent('edx.ui.lms.sequence.tab_selected', 'top', destinationUnitId); handleNavigate(destinationUnitId); }} - previousSequenceHandler={() => { + previousHandler={() => { logEvent('edx.ui.lms.sequence.previous_selected', 'top'); handlePrevious(); }} - goToCourseExitPage={() => goToCourseExitPage()} /> {shouldDisplayNotificationTriggerInSequence && } @@ -186,7 +181,6 @@ const Sequence = ({ logEvent('edx.ui.lms.sequence.next_selected', 'bottom'); handleNext(); }} - goToCourseExitPage={() => goToCourseExitPage()} /> )}
diff --git a/src/courseware/course/sequence/Sequence.test.jsx b/src/courseware/course/sequence/Sequence.test.jsx index 8c07f989..716de869 100644 --- a/src/courseware/course/sequence/Sequence.test.jsx +++ b/src/courseware/course/sequence/Sequence.test.jsx @@ -46,6 +46,7 @@ describe('Sequence', () => { expect(screen.getByText('There is no content here.')).toBeInTheDocument(); expect(screen.queryByRole('button')).not.toBeInTheDocument(); + expect(screen.queryByRole('link')).not.toBeInTheDocument(); }); it('renders correctly for gated content', async () => { @@ -74,8 +75,10 @@ describe('Sequence', () => { ); await waitFor(() => expect(screen.queryByText('Loading locked content messaging...')).toBeInTheDocument()); - // `Previous`, `Active`, `Next`, `Prerequisite` and `Close Tray` buttons. - expect(screen.getAllByRole('button').length).toEqual(5); + // `Previous`, `Prerequisite` and `Close Tray` buttons. + expect(screen.getAllByRole('button').length).toEqual(3); + // `Active` and `Next` buttons. + expect(screen.getAllByRole('link').length).toEqual(2); expect(screen.getByText('Content Locked')).toBeInTheDocument(); const unitContainer = container.querySelector('.unit-container'); @@ -112,6 +115,7 @@ describe('Sequence', () => { // No normal content or navigation should be rendered. Just the above alert. expect(screen.queryAllByRole('button').length).toEqual(0); + expect(screen.queryAllByRole('link').length).toEqual(1); }); it('displays error message on sequence load failure', async () => { @@ -125,13 +129,16 @@ describe('Sequence', () => { it('handles loading unit', async () => { render(); expect(await screen.findByText('Loading learning sequence...')).toBeInTheDocument(); - // Renders navigation buttons plus one button for each unit. - expect(screen.getAllByRole('button')).toHaveLength(4 + unitBlocks.length); + // `Previous`, `Bookmark` and `Close Tray` buttons + expect(screen.getAllByRole('button')).toHaveLength(3); + // Renders `Next` button plus one button for each unit. + expect(screen.getAllByRole('link')).toHaveLength(1 + unitBlocks.length); loadUnit(); await waitFor(() => expect(screen.queryByText('Loading learning sequence...')).not.toBeInTheDocument()); // At this point there will be 2 `Previous` and 2 `Next` buttons. - expect(screen.getAllByRole('button', { name: /previous|next/i }).length).toEqual(4); + expect(screen.getAllByRole('button', { name: /previous/i }).length).toEqual(2); + expect(screen.getAllByRole('link', { name: /next/i }).length).toEqual(2); }); describe('sequence and unit navigation buttons', () => { @@ -163,7 +170,7 @@ describe('Sequence', () => { render(, { store: testStore }); expect(await screen.findByText('Loading learning sequence...')).toBeInTheDocument(); - const sequencePreviousButton = screen.getByRole('button', { name: /previous/i }); + const sequencePreviousButton = screen.getByRole('link', { name: /previous/i }); fireEvent.click(sequencePreviousButton); expect(testData.previousSequenceHandler).toHaveBeenCalledTimes(1); expect(sendTrackEvent).toHaveBeenCalledTimes(1); @@ -176,7 +183,7 @@ describe('Sequence', () => { loadUnit(); await waitFor(() => expect(screen.queryByText('Loading learning sequence...')).not.toBeInTheDocument()); - const unitPreviousButton = screen.getAllByRole('button', { name: /previous/i }) + const unitPreviousButton = screen.getAllByRole('link', { name: /previous/i }) .filter(button => button !== sequencePreviousButton)[0]; fireEvent.click(unitPreviousButton); expect(testData.previousSequenceHandler).toHaveBeenCalledTimes(2); @@ -199,7 +206,7 @@ describe('Sequence', () => { render(, { store: testStore }); expect(await screen.findByText('Loading learning sequence...')).toBeInTheDocument(); - const sequenceNextButton = screen.getByRole('button', { name: /next/i }); + const sequenceNextButton = screen.getByRole('link', { name: /next/i }); fireEvent.click(sequenceNextButton); expect(testData.nextSequenceHandler).toHaveBeenCalledTimes(1); expect(sendTrackEvent).toHaveBeenCalledWith('edx.ui.lms.sequence.next_selected', { @@ -211,7 +218,7 @@ describe('Sequence', () => { loadUnit(); await waitFor(() => expect(screen.queryByText('Loading learning sequence...')).not.toBeInTheDocument()); - const unitNextButton = screen.getAllByRole('button', { name: /next/i }) + const unitNextButton = screen.getAllByRole('link', { name: /next/i }) .filter(button => button !== sequenceNextButton)[0]; fireEvent.click(unitNextButton); expect(testData.nextSequenceHandler).toHaveBeenCalledTimes(2); @@ -237,11 +244,11 @@ describe('Sequence', () => { render(, { store: testStore }); await waitFor(() => expect(screen.queryByText('Loading learning sequence...')).toBeInTheDocument()); - fireEvent.click(screen.getByRole('button', { name: /previous/i })); + fireEvent.click(screen.getByRole('link', { name: /previous/i })); expect(testData.previousSequenceHandler).not.toHaveBeenCalled(); expect(testData.unitNavigationHandler).toHaveBeenCalledWith(unitBlocks[unitNumber - 1].id); - fireEvent.click(screen.getByRole('button', { name: /next/i })); + fireEvent.click(screen.getByRole('link', { name: /next/i })); expect(testData.nextSequenceHandler).not.toHaveBeenCalled(); // As `previousSequenceHandler` and `nextSequenceHandler` are mocked, we aren't really changing the position here. // Therefore the next unit will still be `the initial one + 1`. @@ -323,11 +330,11 @@ describe('Sequence', () => { loadUnit(); await waitFor(() => expect(screen.queryByText('Loading learning sequence...')).not.toBeInTheDocument()); - screen.getAllByRole('button', { name: /previous/i }).forEach(button => fireEvent.click(button)); + screen.getAllByRole('link', { name: /previous/i }).forEach(button => fireEvent.click(button)); expect(testData.previousSequenceHandler).toHaveBeenCalledTimes(2); expect(testData.unitNavigationHandler).not.toHaveBeenCalled(); - screen.getAllByRole('button', { name: /next/i }).forEach(button => fireEvent.click(button)); + screen.getAllByRole('link', { name: /next/i }).forEach(button => fireEvent.click(button)); expect(testData.nextSequenceHandler).toHaveBeenCalledTimes(2); expect(testData.unitNavigationHandler).not.toHaveBeenCalled(); @@ -370,7 +377,7 @@ describe('Sequence', () => { render(, { store: testStore }); await waitFor(() => expect(screen.queryByText('Loading learning sequence...')).toBeInTheDocument()); - fireEvent.click(screen.getByRole('button', { name: targetUnit.display_name })); + fireEvent.click(screen.getByRole('link', { name: targetUnit.display_name })); expect(testData.unitNavigationHandler).toHaveBeenCalledWith(targetUnit.id); expect(sendTrackEvent).toHaveBeenCalledWith('edx.ui.lms.sequence.tab_selected', { current_tab: currentTabNumber, diff --git a/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx b/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx index 76bbaf3f..df45753b 100644 --- a/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx +++ b/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx @@ -1,4 +1,5 @@ import React from 'react'; +import { Link } from 'react-router-dom'; import PropTypes from 'prop-types'; import { breakpoints, Button, useWindowSize } from '@edx/paragon'; import { ChevronLeft, ChevronRight } from '@edx/paragon/icons'; @@ -26,12 +27,13 @@ const SequenceNavigation = ({ sequenceId, className, onNavigate, - nextSequenceHandler, - previousSequenceHandler, - goToCourseExitPage, + nextHandler, + previousHandler, }) => { const sequence = useModel('sequences', sequenceId); - const { isFirstUnit, isLastUnit } = useSequenceNavigationMetadata(sequenceId, unitId); + const { + isFirstUnit, isLastUnit, nextLink, previousLink, + } = useSequenceNavigationMetadata(sequenceId, unitId); const { courseId, sequenceStatus, @@ -63,27 +65,49 @@ const SequenceNavigation = ({ ); }; + const renderPreviousButton = () => { + const disabled = isFirstUnit; + const prevArrow = isRtl(getLocale()) ? ChevronRight : ChevronLeft; + + return ( + + ); + }; + const renderNextButton = () => { const { exitActive, exitText } = GetCourseExitNavigation(courseId, intl); - const buttonOnClick = isLastUnit ? goToCourseExitPage : nextSequenceHandler; const buttonText = (isLastUnit && exitText) ? exitText : intl.formatMessage(messages.nextButton); const disabled = isLastUnit && !exitActive; const nextArrow = isRtl(getLocale()) ? ChevronLeft : ChevronRight; return ( - ); }; - const prevArrow = isRtl(getLocale()) ? ChevronRight : ChevronLeft; - return sequenceStatus === LOADED && (