fix: make nav buttons use links for accessibility (#1137)

openedx · Aug 14, 2023 · 5ee6190 · 5ee6190
1 parent 6232b0c
commit 5ee6190
Show file tree

Hide file tree

Showing 15 changed files with 172 additions and 113 deletions.
diff --git 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,34 +254,25 @@ 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);
       }
     }
   };
 
-  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
@@ -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
@@ -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
@@ -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 = (
     <div className="sequence-container d-inline-flex flex-row">
@@ -150,19 +146,18 @@ const Sequence = ({
           sequenceId={sequenceId}
           unitId={unitId}
           className="mb-4"
-          nextSequenceHandler={() => {
+          nextHandler={() => {
             logEvent('edx.ui.lms.sequence.next_selected', 'top');
             handleNext();
           }}
           onNavigate={(destinationUnitId) => {
             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 && <SidebarTriggers />}
 
@@ -186,7 +181,6 @@ const Sequence = ({
               logEvent('edx.ui.lms.sequence.next_selected', 'bottom');
               handleNext();
             }}
-            goToCourseExitPage={() => goToCourseExitPage()}
           />
           )}
         </div>

diff --git 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(<Sequence {...mockData} />);
     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(<Sequence {...testData} />, { 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(<Sequence {...testData} />, { 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(<Sequence {...testData} />, { 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(<Sequence {...testData} />, { 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
@@ -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 (
+      <Button
+        variant="link"
+        className="previous-btn"
+        onClick={previousHandler}
+        disabled={disabled}
+        iconBefore={prevArrow}
+        as={disabled ? undefined : Link}
+        to={disabled ? undefined : previousLink}
+      >
+        {shouldDisplayNotificationTriggerInSequence ? null : intl.formatMessage(messages.previousButton)}
+      </Button>
+    );
+  };
+
   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 (
-      <Button variant="link" className="next-btn" onClick={buttonOnClick} disabled={disabled} iconAfter={nextArrow}>
+      <Button
+        variant="link"
+        className="next-btn"
+        onClick={nextHandler}
+        disabled={disabled}
+        iconAfter={nextArrow}
+        as={disabled ? undefined : Link}
+        to={disabled ? undefined : nextLink}
+      >
         {shouldDisplayNotificationTriggerInSequence ? null : buttonText}
       </Button>
     );
   };
 
-  const prevArrow = isRtl(getLocale()) ? ChevronRight : ChevronLeft;
-
   return sequenceStatus === LOADED && (
     <nav id="courseware-sequenceNavigation" className={classNames('sequence-navigation', className)} style={{ width: shouldDisplayNotificationTriggerInSequence ? '90%' : null }}>
-      <Button variant="link" className="previous-btn" onClick={previousSequenceHandler} disabled={isFirstUnit} iconBefore={prevArrow}>
-        {shouldDisplayNotificationTriggerInSequence ? null : intl.formatMessage(messages.previousButton)}
-      </Button>
+      {renderPreviousButton()}
       {renderUnitButtons()}
       {renderNextButton()}
 
@@ -97,9 +121,8 @@ SequenceNavigation.propTypes = {
   unitId: PropTypes.string,
   className: PropTypes.string,
   onNavigate: PropTypes.func.isRequired,
-  nextSequenceHandler: PropTypes.func.isRequired,
-  previousSequenceHandler: PropTypes.func.isRequired,
-  goToCourseExitPage: PropTypes.func.isRequired,
+  nextHandler: PropTypes.func.isRequired,
+  previousHandler: PropTypes.func.isRequired,
 };
 
 SequenceNavigation.defaultProps = {