Fix possible role relation a11y issues in transcript display (#821)

Dananji · web-flow · commit 07c656f2d487 · 2025-05-27T11:34:11.000-07:00
diff --git a/src/components/Transcript/Transcript.js b/src/components/Transcript/Transcript.js
@@ -11,7 +11,7 @@ import {
   useSearchCounts
 } from '@Services/search';
 import { useTranscripts } from '@Services/ramp-hooks';
-import { autoScroll, screenReaderFriendlyTime, timeToHHmmss } from '@Services/utility-helpers';
+import { autoScroll, screenReaderFriendlyText, screenReaderFriendlyTime, timeToHHmmss } from '@Services/utility-helpers';
 import Spinner from '@Components/Spinner';
 import './Transcript.scss';
 
@@ -114,6 +114,20 @@ const TranscriptLine = memo(({
   const onClick = (e) => {
     e.preventDefault();
     e.stopPropagation();
+
+    // Handle click on a link in the cue text in the same tab
+    if (e.target.tagName == 'A') {
+      // Check if the href value is a valid URL before navigation
+      const urlRegex = /https?:\/\/[^\s/$.?#].[^\s]*/gi;
+      const href = e.target.getAttribute('href');
+      if (!href?.match(urlRegex)) {
+        e.preventDefault();
+      } else {
+        window.open(href, '_self');
+        return;
+      }
+    }
+
     if (item.match && focusedMatchId !== item.id) {
       setFocusedMatchId(item.id);
     } else if (focusedMatchId !== null && item.tag === TRANSCRIPT_CUE_TYPES.timedCue) {
@@ -138,7 +152,7 @@ const TranscriptLine = memo(({
 
   const cueText = useMemo(() => {
     return buildSpeakerText(item, item.tag === TRANSCRIPT_CUE_TYPES.nonTimedLine);
-  }, [item?.tag]);
+  }, [item]);
 
   /** Build text portion of the transcript cue element */
   const cueTextElement = useMemo(() => {
@@ -171,22 +185,30 @@ const TranscriptLine = memo(({
 
   return (
     <span
-      role="radio"
-      tabIndex={isFirstItem ? 0 : -1}
-      aria-checked={isActive}
       ref={itemRef}
-      onClick={onClick}
-      onKeyDown={handleKeyDown}
       className={cx(
         'ramp--transcript_item',
         isActive && 'active',
-        isFocused && 'focused'
+        isFocused && 'focused',
+        item.tag != TRANSCRIPT_CUE_TYPES.timedCue && 'untimed',
       )}
       data-testid={testId}
-      aria-label={`${screenReaderFriendlyTime(item.begin)}, ${cueText}`}
+      /* For untimed cues,
+       - set tabIndex for keyboard navigation
+       - onClick handler to scroll them to top on click
+       - set aria-label with full cue text */
+      tabIndex={isFirstItem && item.begin == undefined ? 0 : -1}
+      onClick={item.begin == undefined ? onClick : null}
+      aria-label={item.begin == undefined && screenReaderFriendlyText(cueText)}
     >
       {item.tag === TRANSCRIPT_CUE_TYPES.timedCue && typeof item.begin === 'number' && (
-        <span className="ramp--transcript_time" data-testid="transcript_time">
+        <span className='ramp--transcript_time' data-testid='transcript_time'
+          role='button'
+          onClick={onClick}
+          onKeyDown={handleKeyDown}
+          tabIndex={isFirstItem ? 0 : -1}
+          aria-label={`${screenReaderFriendlyTime(item.begin)}, ${screenReaderFriendlyText(cueText)}`}
+        >
           [{timeToHHmmss(item.begin, true)}]
         </span>
       )}
@@ -247,19 +269,24 @@ const TranscriptList = memo(({
    * @param {Event} e keyboard event
    */
   const handleKeyDown = (e) => {
-    const cues = transcriptListRef.current.children;
-    if (cues?.length > 0) {
+    // Get the timestamp for each cue for timed transcript, as these are focusable
+    const cueTimes = transcriptListRef.current.querySelectorAll('.ramp--transcript_time');
+    // Get the non-empty cues for untimed transcript
+    const cueList = Array.from(transcriptListRef.current.children).filter((c) => c.textContent?.length > 0);
+
+    const cueLength = cueTimes?.length || cueList?.length || 0;
+    if (cueLength > 0) {
       let nextIndex = currentIndex.current;
       /**
        * Default behavior is prevented (e.preventDefault()) only for the handled 
        * key combinations to allow other keyboard shortcuts to work as expected.
        */
       if (e.key === 'ArrowDown') {
         // Wraps focus back to first cue when the end of transcript is reached
-        nextIndex = (currentIndex.current + 1) % cues.length;
+        nextIndex = (currentIndex.current + 1) % cueLength;
         e.preventDefault();
       } else if (e.key === 'ArrowUp') {
-        nextIndex = (currentIndex.current - 1 + cues.length) % cues.length;
+        nextIndex = (currentIndex.current - 1 + cueLength) % cueLength;
         e.preventDefault();
       } else if (e.key === 'Tab' && e.shiftKey) {
         // Returns focus to parent container on (Shift + Tab) key combination press
@@ -268,11 +295,21 @@ const TranscriptList = memo(({
         return;
       }
       if (nextIndex !== currentIndex.current) {
-        cues[currentIndex.current].tabIndex = -1;
-        cues[nextIndex].tabIndex = 0;
-        cues[nextIndex].focus();
-        // Scroll the cue into view
-        autoScroll(cues[nextIndex], transcriptContainerRef);
+        if (cueTimes?.length > 0) {
+          // Use timestamps of timed cues for navigation
+          cueTimes[currentIndex.current].tabIndex = -1;
+          cueTimes[nextIndex].tabIndex = 0;
+          cueTimes[nextIndex].focus();
+          // Scroll the cue into view
+          autoScroll(cueTimes[nextIndex], transcriptContainerRef);
+        } else if (cueList?.length > 0) {
+          // Use whole cues for navigation for untimed cues
+          cueList[currentIndex.current].tabIndex = -1;
+          cueList[nextIndex].tabIndex = 0;
+          cueList[nextIndex].focus();
+          // Scroll the cue to the top of container
+          autoScroll(cueList[nextIndex], transcriptContainerRef, true);
+        }
         setCurrentIndex(nextIndex);
       }
     }
@@ -292,7 +329,6 @@ const TranscriptList = memo(({
     return (
       <div
         data-testid={`transcript_${testId}`}
-        role="radiogroup"
         onKeyDown={handleKeyDown}
         ref={transcriptListRef}
         aria-label='Scrollable transcript cues'
diff --git a/src/components/Transcript/Transcript.scss b/src/components/Transcript/Transcript.scss
@@ -40,19 +40,24 @@ p.ramp--transcript_untimed_item {
 span.ramp--transcript_item {
   display: flex;
   margin: 10px 10px 10px 10px;
-  cursor: pointer;
   text-decoration: none;
   transition: background-color 0.2s ease-in;
+  align-items: center;
+
+  &.untimed {
+    // Enable pointer events for untimed items to allow scrolling on click
+    cursor: pointer;
+    // Highlight entire cue on hover
+    &:focus,
+    &:hover {
+      background-color: $primaryGreenLight;
+    }
+  }
 
   &.active {
     background-color: $primaryLighter;
   }
 
-  &:hover,
-  &:focus {
-    background-color: $primaryGreenLight;
-  }
-
   &.disabled {
     cursor: default;
   }
@@ -71,8 +76,13 @@ span.ramp--transcript_item {
   }
 
   .ramp--transcript_time {
-    margin-right: 15px;
+    cursor: pointer;
+    margin-right: 1em;
     color: $primaryGreenDark;
+
+    &:hover {
+      text-decoration: underline;
+    }
   }
 
   .ramp--transcript_text {
diff --git a/src/components/Transcript/Transcript.test.js b/src/components/Transcript/Transcript.test.js
@@ -110,13 +110,26 @@ describe('Transcript component', () => {
         });
       });
 
-      test('highlights transcript item on click', async () => {
+      test('highlights cue when clicking on cue\'s timestamp', async () => {
         await waitFor(() => {
           const transcriptItem = screen.queryAllByTestId('transcript_item')[0];
-          fireEvent.click(transcriptItem);
+          expect(transcriptItem.children).toHaveLength(2);
+          expect(transcriptItem.children[0].textContent).toEqual('[00:00:01]');
+          expect(transcriptItem.children[1].textContent).toEqual('[music]');
+          expect(transcriptItem.classList.contains('active')).toBeFalsy();
+          // Click on the cue's timestamp
+          fireEvent.click(transcriptItem.children[0]);
           expect(transcriptItem.classList.contains('active')).toBeTruthy();
         });
       });
+
+      test('does nothing when clicking on the cue\'s text', async () => {
+        await waitFor(() => {
+          const transcriptItem = screen.queryAllByTestId('transcript_item')[0];
+          fireEvent.click(transcriptItem.children[1]);
+          expect(transcriptItem.classList.contains('active')).toBeFalsy();
+        });
+      });
     });
 
     describe('with WebVTT including a header block', () => {
@@ -190,13 +203,41 @@ describe('Transcript component', () => {
         });
       });
 
-      test('renders the rest of the cue with timestamp', async () => {
+      test('renders the rest of the cues with timestamps', async () => {
+        await waitFor(() => {
+          const transcriptItem0 = screen.queryAllByTestId('transcript_item')[0];
+          expect(transcriptItem0.children).toHaveLength(2);
+          expect(transcriptItem0.children[0].textContent).toEqual('[00:00:01]');
+          expect(transcriptItem0.children[1].textContent).toEqual('[music]');
+          expect(transcriptItem0.classList.contains('active')).toBeFalsy();
+
+          const transcriptItem1 = screen.queryAllByTestId('transcript_item')[1];
+          expect(transcriptItem1.children).toHaveLength(2);
+          expect(transcriptItem1.children[0].textContent).toEqual('[00:00:22]');
+          expect(transcriptItem1.children[1].textContent).toEqual('transcript text 1');
+          expect(transcriptItem1.classList.contains('active')).toBeFalsy();
+        });
+      });
+
+      test('highlights cue when clicking on the cue\'s timestamp', async () => {
         await waitFor(() => {
           const transcriptItem = screen.queryAllByTestId('transcript_item')[1];
-          fireEvent.click(transcriptItem);
+          expect(transcriptItem.children).toHaveLength(2);
+          expect(transcriptItem.children[0].textContent).toEqual('[00:00:22]');
+          expect(transcriptItem.children[1].textContent).toEqual('transcript text 1');
+          // Click on the cue's timestamp
+          fireEvent.click(transcriptItem.children[0]);
           expect(transcriptItem.classList.contains('active')).toBeTruthy();
         });
       });
+
+      test('does nothing when clicking on the cue\'s text', async () => {
+        await waitFor(() => {
+          const transcriptItem = screen.queryAllByTestId('transcript_item')[1];
+          fireEvent.click(transcriptItem.children[1]);
+          expect(transcriptItem.classList.contains('active')).toBeFalsy();
+        });
+      });
     });
 
     describe('with WebVTT with NOTE comment', () => {
diff --git a/src/services/annotations-parser.js b/src/services/annotations-parser.js
@@ -365,16 +365,18 @@ function parseAnnotationBody(annotationBody, motivations) {
 export async function parseExternalAnnotationResource(annotation) {
   const { canvasId, format, id, motivation, url } = annotation;
   const { tData } = await parseTranscriptData(url, format);
-  return tData.map((data) => {
-    const { begin, end, text } = data;
-    return {
-      canvasId,
-      id,
-      motivation,
-      time: { start: begin, end },
-      value: [{ format: 'text/plain', purpose: motivation, value: text }],
-    };
-  });
+  if (tData) {
+    return tData.map((data) => {
+      const { begin, end, text } = data;
+      return {
+        canvasId,
+        id,
+        motivation,
+        time: { start: begin, end },
+        value: [{ format: 'text/plain', purpose: motivation, value: text }],
+      };
+    });
+  }
 }
 
 /**
diff --git a/src/services/utility-helpers.js b/src/services/utility-helpers.js
@@ -109,6 +109,17 @@ export function screenReaderFriendlyTime(time) {
   }
 };
 
+/**
+ * Convert a given text with HTML tags to a string read as a human
+ * @param {String} html text with HTML tags
+ * @returns {String} text without HTML tags
+ */
+export function screenReaderFriendlyText(html) {
+  const tempElement = document.createElement('div');
+  tempElement.innerHTML = html;
+  return tempElement.textContent || tempElement.innerText || "";
+}
+
 /**
  * Convert time from hh:mm:ss.ms/mm:ss.ms string format to int
  * @function Utils#timeToS
@@ -627,14 +638,14 @@ export function playerHotKeys(event, player, canvasIsEmpty) {
   let isCombKeyPress = event.ctrlKey || event.metaKey || event.altKey || event.shiftKey;
 
   // CSS classes of active buttons to skip
-  let buttonClassesToCheck = ['ramp--transcript_item', 'ramp--structured-nav__section-title',
+  let buttonClassesToCheck = ['ramp--transcript_time', 'ramp--structured-nav__section-title',
     'ramp--structured-nav__item-link', 'ramp--structured-nav__collapse-all-btn',
     'ramp--annotations__multi-select-header', 'ramp--annotations__show-more-tags',
     'ramp--annotations__show-more-less'
   ];
 
   // Determine the focused element and pressed key combination needs to be skipped
-  let skipActionOnFocus = (
+  let skipActionWithButtonFocus = (
     activeElement?.role === 'button'
     && (
       (
@@ -679,7 +690,9 @@ export function playerHotKeys(event, player, canvasIsEmpty) {
         inputs.indexOf(activeElement.tagName.toLowerCase()) !== -1
         || (activeElement.role === 'tab' && (pressedKey === 37 || pressedKey === 39))
         || (activeElement.role === 'switch' && (pressedKey === 13 || pressedKey === 32))
-        || skipActionOnFocus
+        || (activeElement?.classList?.contains('transcript_content') && (pressedKey === 38 || pressedKey === 40))
+        || (activeElement?.classList?.contains('ramp--transcript_item')) && (pressedKey === 38 || pressedKey === 40)
+        || skipActionWithButtonFocus
       )
       && !focusedWithinPlayer)
     || isCombKeyPress || canvasIsEmpty
