refactor after review

Author: filippovskii09

src/courseware/course/JumpNavMenuItem.test.jsx

@@ -11,7 +11,6 @@ const mockData = {
   sectionId: 'block-v1:edX+DemoX+Demo_Course+type@chapter+block@interactive_demonstrations',
   sequenceId: 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@basic_questions',
 
-  currentSequence: 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@basic_questions',
   title: 'Demo Menu Item',
   courseId: 'course-v1:edX+DemoX+Demo_Course',
   currentUnit: 'block-v1:edX+DemoX+Demo_Course+type@chapter+block@interactive_demonstrations',

src/courseware/course/sidebar/common/Sidebar.test.jsx

@@ -1,5 +1,5 @@
-import React from 'react';
 import { Factory } from 'rosie';
+
 import {
   initializeTestStore,
   render,
@@ -8,9 +8,10 @@ import {
   waitFor,
 } from '@src/setupTest';
 import userEvent from '@testing-library/user-event';
+import { createRef } from 'react';
+import messages from '@src/courseware/course/messages';
 import SidebarContext from '../SidebarContext';
 import SidebarBase from './SidebarBase';
-import messages from '../../messages';
 import { useSidebarFocusAndKeyboard } from './hooks/useSidebarFocusAndKeyboard';
 
 jest.mock('./hooks/useSidebarFocusAndKeyboard');
@@ -19,7 +20,7 @@ const SIDEBAR_ID = 'test-sidebar';
 
 const mockUseSidebarFocusAndKeyboard = useSidebarFocusAndKeyboard;
 
-describe('SidebarBase (Refactored)', () => {
+describe('SidebarBase', () => {
   let mockContextValue;
   const courseMetadata = Factory.build('courseMetadata');
   const user = userEvent.setup();
@@ -61,8 +62,8 @@ describe('SidebarBase (Refactored)', () => {
       currentSidebar: null,
     };
 
-    mockCloseBtnRef = React.createRef();
-    mockBackBtnRef = React.createRef();
+    mockCloseBtnRef = createRef();
+    mockBackBtnRef = createRef();
     mockHandleClose = jest.fn();
     mockHandleKeyDown = jest.fn();
     mockHandleBackBtnKeyDown = jest.fn();

src/courseware/course/sidebar/common/SidebarBase.jsx

@@ -8,7 +8,7 @@ import {
 } from 'react';
 
 import { useEventListener } from '@src/generic/hooks';
-import messages from '../../messages';
+import messages from '@src/courseware/course/messages';
 import SidebarContext from '../SidebarContext';
 import { useSidebarFocusAndKeyboard } from './hooks/useSidebarFocusAndKeyboard';
 
@@ -43,7 +43,6 @@ const SidebarBase = ({
     if (type === 'learning.events.sidebar.close') {
       toggleSidebar(null);
     }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [toggleSidebar]);
 
   useEventListener('message', receiveMessage);

src/courseware/course/sidebar/common/hooks/useSidebarFocusAndKeyboard.js

@@ -1,9 +1,19 @@
 import {
   useCallback, useContext, useEffect, useRef,
 } from 'react';
+
 import { tryFocusAndPreventDefault } from '../../utils';
 import SidebarContext from '../../SidebarContext';
 
+/**
+ * Manages accessibility interactions for the SidebarBase component, including:
+ * 1. Setting initial focus when the sidebar opens.
+ * 2. Handling sidebar closing and returning focus to the trigger.
+ * 3. Managing keyboard navigation (Tab/Shift+Tab) for focus trapping/guidance.
+ *
+ * @param {string} sidebarId The unique ID of this sidebar.
+ * @param {string} [triggerButtonSelector] The CSS selector for the trigger button
+ */
 export const useSidebarFocusAndKeyboard = (sidebarId, triggerButtonSelector = '.sidebar-trigger-btn') => {
   const {
     toggleSidebar,
@@ -29,12 +39,10 @@ export const useSidebarFocusAndKeyboard = (sidebarId, triggerButtonSelector = '.
 
   const focusSidebarTriggerBtn = useCallback(() => {
     requestAnimationFrame(() => {
-      requestAnimationFrame(() => {
-        const sidebarTriggerBtn = document.querySelector(triggerButtonSelector);
-        if (sidebarTriggerBtn) {
-          sidebarTriggerBtn.focus();
-        }
-      });
+      const sidebarTriggerBtn = document.querySelector(triggerButtonSelector);
+      if (sidebarTriggerBtn) {
+        sidebarTriggerBtn.focus();
+      }
     });
   }, [triggerButtonSelector]);
 

src/courseware/course/sidebar/common/hooks/useSidebarFocusAndKeyboard.test.jsx

@@ -1,13 +1,8 @@
 import { renderHook, act } from '@testing-library/react';
+
 import SidebarContext from '../../SidebarContext';
 import { useSidebarFocusAndKeyboard } from './useSidebarFocusAndKeyboard';
 
-import { tryFocusAndPreventDefault } from '../../utils';
-
-jest.mock('../../utils', () => ({
-  tryFocusAndPreventDefault: jest.fn(),
-}));
-
 const SIDEBAR_ID = 'test-sidebar';
 const TRIGGER_SELECTOR = '.sidebar-trigger-btn';
 
@@ -131,13 +126,20 @@ describe('useSidebarFocusAndKeyboard', () => {
 
   describe('handleKeyDown (Standard Close Button)', () => {
     let mockEvent;
+    let mockOutlineTrigger;
+    let mockPrevButton;
+    let mockNextButton;
 
     beforeEach(() => {
       mockEvent = {
         key: 'Tab',
         shiftKey: false,
         preventDefault: jest.fn(),
       };
+
+      mockOutlineTrigger = { focus: jest.fn(), disabled: false };
+      mockPrevButton = { focus: jest.fn(), disabled: false };
+      mockNextButton = { focus: jest.fn(), disabled: false };
     });
 
     it('should do nothing if key is not Tab', () => {
@@ -151,7 +153,6 @@ describe('useSidebarFocusAndKeyboard', () => {
 
       expect(mockEvent.preventDefault).not.toHaveBeenCalled();
       expect(triggerButtonMock.focus).not.toHaveBeenCalled();
-      expect(tryFocusAndPreventDefault).not.toHaveBeenCalled();
     });
 
     it('should call focusSidebarTriggerBtn on Shift+Tab', () => {
@@ -164,46 +165,44 @@ describe('useSidebarFocusAndKeyboard', () => {
       });
 
       expect(mockEvent.preventDefault).toHaveBeenCalledTimes(1);
-      act(() => { jest.runAllTimers(); });
+      act(() => jest.runAllTimers());
       expect(triggerButtonMock.focus).toHaveBeenCalledTimes(1);
-      expect(tryFocusAndPreventDefault).not.toHaveBeenCalled();
     });
 
     it('should attempt to focus elements sequentially on Tab', () => {
       mockContextValue = getMockContext(SIDEBAR_ID);
       const { result } = renderHookWithContext(mockContextValue);
-      mockEvent.shiftKey = false;
 
-      (tryFocusAndPreventDefault).mockImplementation((event, selector) => {
-        if (selector === '.previous-button') {
-          event.preventDefault();
-          return true;
-        }
-        return false;
+      mockQuerySelector.mockImplementation((selector) => {
+        if (selector === '#courseOutlineSidebarTrigger') { return mockOutlineTrigger; }
+        if (selector === '.previous-button') { return mockPrevButton; }
+        if (selector === '.next-button') { return mockNextButton; }
+
+        return null;
       });
 
       act(() => {
         result.current.handleKeyDown(mockEvent);
       });
 
-      expect(tryFocusAndPreventDefault).toHaveBeenCalledWith(mockEvent, '#courseOutlineSidebarTrigger');
-      expect(tryFocusAndPreventDefault).toHaveBeenCalledWith(mockEvent, '.previous-button');
-      expect(tryFocusAndPreventDefault).not.toHaveBeenCalledWith(mockEvent, '.next-button');
       expect(mockEvent.preventDefault).toHaveBeenCalledTimes(1);
+      expect(mockOutlineTrigger.focus).toHaveBeenCalledTimes(1);
+      expect(mockPrevButton.focus).not.toHaveBeenCalled();
     });
 
     it('should allow default Tab if no elements are focused by tryFocusAndPreventDefault', () => {
       mockContextValue = getMockContext(SIDEBAR_ID);
       const { result } = renderHookWithContext(mockContextValue);
-      mockEvent.shiftKey = false;
 
-      (tryFocusAndPreventDefault).mockReturnValue(false);
+      mockQuerySelector.mockImplementation((selector) => {
+        if (selector === TRIGGER_SELECTOR) { return triggerButtonMock; }
+        return null;
+      });
 
       act(() => {
         result.current.handleKeyDown(mockEvent);
       });
 
-      expect(tryFocusAndPreventDefault).toHaveBeenCalledTimes(3);
       expect(mockEvent.preventDefault).not.toHaveBeenCalled();
     });
   });
@@ -237,7 +236,7 @@ describe('useSidebarFocusAndKeyboard', () => {
       });
 
       expect(mockToggleSidebar).toHaveBeenCalledWith(null);
-      act(() => { jest.runAllTimers(); });
+      act(() => jest.runAllTimers());
       expect(triggerButtonMock.focus).toHaveBeenCalledTimes(1);
     });
 

src/courseware/course/sidebar/sidebars/notifications/NotificationTray.test.jsx

@@ -5,7 +5,7 @@ import { breakpoints } from '@openedx/paragon';
 
 import MockAdapter from 'axios-mock-adapter';
 import { Factory } from 'rosie';
-import messages from '../../../messages';
+import messages from '@src/courseware/course/messages';
 import {
   fireEvent, initializeMockApp, render, screen, waitFor,
 } from '../../../../../setupTest';

src/courseware/course/sidebar/sidebars/notifications/NotificationTrigger.test.jsx

@@ -1,7 +1,8 @@
 import PropTypes from 'prop-types';
 import { Factory } from 'rosie';
+import userEvent from '@testing-library/user-event';
 
-import messages from '../../../messages';
+import messages from '@src/courseware/course/messages';
 import {
   fireEvent, initializeTestStore, render, screen,
 } from '../../../../../setupTest';
@@ -139,14 +140,14 @@ describe('Notification Trigger', () => {
     expect(localStorage.getItem(`notificationStatus.${courseMetadataSecondCourse.id}`)).toBe('"inactive"');
   });
 
-  it('should call toggleSidebar and onClick prop on click', () => {
+  it('should call toggleSidebar and onClick prop on click', async () => {
     const externalOnClick = jest.fn();
     renderWithProvider({}, externalOnClick);
 
     const triggerButton = screen.getByRole('button', {
       name: messages.openNotificationTrigger.defaultMessage,
     });
-    fireEvent.click(triggerButton);
+    await userEvent.click(triggerButton);
 
     expect(mockData.toggleSidebar).toHaveBeenCalledTimes(1);
     expect(mockData.toggleSidebar).toHaveBeenCalledWith(ID);
@@ -175,34 +176,29 @@ describe('Notification Trigger', () => {
       querySelectorSpy.mockRestore();
     });
 
-    it('should focus the close button and prevent default behavior if sidebar is open', () => {
+    it('should focus the close button and prevent default behavior if sidebar is open', async () => {
       renderWithProvider({ currentSidebar: ID });
       triggerButton = screen.getByRole('button', {
         name: messages.openNotificationTrigger.defaultMessage,
       });
 
-      const defaultPrevented = !fireEvent.keyDown(triggerButton, {
-        key: 'Tab',
-        shiftKey: false,
-      });
+      triggerButton.focus();
+      expect(document.activeElement).toBe(triggerButton);
+
+      await userEvent.tab();
 
-      expect(defaultPrevented).toBe(true);
       expect(querySelectorSpy).toHaveBeenCalledWith('.sidebar-close-btn');
       expect(mockCloseButtonFocus).toHaveBeenCalledTimes(1);
     });
 
-    it('should do nothing (allow default Tab behavior) if sidebar is closed', () => {
+    it('should do nothing (allow default Tab behavior) if sidebar is closed', async () => {
       renderWithProvider({ currentSidebar: null });
       triggerButton = screen.getByRole('button', {
         name: messages.openNotificationTrigger.defaultMessage,
       });
 
-      const defaultPrevented = !fireEvent.keyDown(triggerButton, {
-        key: 'Tab',
-        shiftKey: false,
-      });
+      await userEvent.tab();
 
-      expect(defaultPrevented).toBe(false);
       expect(querySelectorSpy).not.toHaveBeenCalledWith('.sidebar-close-btn');
       expect(mockCloseButtonFocus).not.toHaveBeenCalled();
     });

src/courseware/course/sidebar/utils.js

@@ -1,3 +1,13 @@
+/**
+ * Attempts to find an interactive element by its selector and focus it.
+ * If the element is found and is not disabled, it prevents the default
+ * behavior of the event (e.g., standard Tab) and moves focus to the element.
+ *
+ * @param {Event} event - The keyboard event object (e.g., from a 'keydown' listener).
+ * @param {string} selector - The CSS selector for the target element to focus.
+ * @returns {boolean} - Returns `true` if the element was found, enabled, and focused.
+ * Returns `false` if the element was not found or was disabled.
+ */
 export const tryFocusAndPreventDefault = (event, selector) => {
   const element = document.querySelector(selector);
   if (element && !element.disabled) {