From 82091b3337824d6fbc2c1ccdfe748b624f9a397d Mon Sep 17 00:00:00 2001
From: Roy Johnson <roy.e.johnson@rice.edu>
Date: Wed, 10 Apr 2024 14:15:43 -0500
Subject: [PATCH] Trap focus in EditCard (#2210)

[DISCO-22]

Escape to get out of EditCard

Co-authored-by: staxly[bot] <35789409+staxly[bot]@users.noreply.github.com>
---
 .../highlights/components/CardWrapper.tsx     |  3 ++
 .../highlights/components/EditCard.tsx        |  8 +++--
 src/app/reactUtils.spec.tsx                   |  6 +++-
 src/app/reactUtils.ts                         | 35 +++++++++++--------
 4 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/src/app/content/highlights/components/CardWrapper.tsx b/src/app/content/highlights/components/CardWrapper.tsx
index 171b0c1f55..fd56d6c822 100644
--- a/src/app/content/highlights/components/CardWrapper.tsx
+++ b/src/app/content/highlights/components/CardWrapper.tsx
@@ -56,6 +56,9 @@ const Wrapper = ({highlights, className, container, highlighter}: WrapperProps)
 
   useKeyCombination(highlightKeyCombination, moveFocus, noopKeyCombinationHandler([container, element]));
 
+  // Allow to move back to highlight from EditCard using Escape key
+  useKeyCombination({key: 'Escape'}, moveFocus);
+
   // Clear shouldFocusCard when focus is lost from the CardWrapper.
   // If we don't do this then card related for the focused highlight will be focused automatically.
   useFocusLost(element, shouldFocusCard, React.useCallback(() => setShouldFocusCard(false), [setShouldFocusCard]));
diff --git a/src/app/content/highlights/components/EditCard.tsx b/src/app/content/highlights/components/EditCard.tsx
index 78864ec1f1..043959d374 100644
--- a/src/app/content/highlights/components/EditCard.tsx
+++ b/src/app/content/highlights/components/EditCard.tsx
@@ -8,7 +8,7 @@ import styled, { css } from 'styled-components/macro';
 import { useAnalyticsEvent } from '../../../../helpers/analytics';
 import * as selectAuth from '../../../auth/selectors';
 import Button, { ButtonGroup } from '../../../components/Button';
-import { useFocusElement, useOnEsc } from '../../../reactUtils';
+import { useFocusElement, useOnEsc, useTrapTabNavigation } from '../../../reactUtils';
 import theme from '../../../theme';
 import { assertDefined, assertWindow, mergeRefs } from '../../../utils';
 import { highlightStyles } from '../../constants';
@@ -174,8 +174,12 @@ function ActiveEditCard({
   const onColorChange = useOnColorChange(props);
   const saveAnnotation = useSaveAnnotation(props, element, pendingAnnotation);
 
+  const ref = React.useRef<HTMLElement>(null);
+
+  useTrapTabNavigation(ref, editingAnnotation);
+
   return (
-    <HiddenOnMobile>
+    <HiddenOnMobile ref={ref}>
       <ColorPicker
         color={props.data?.color}
         onChange={onColorChange}
diff --git a/src/app/reactUtils.spec.tsx b/src/app/reactUtils.spec.tsx
index 741cba33b5..8942e492f1 100644
--- a/src/app/reactUtils.spec.tsx
+++ b/src/app/reactUtils.spec.tsx
@@ -272,7 +272,6 @@ describe('useTrapTabNavigation', () => {
   it('attaches listeners', () => {
     const tr = renderer.create(<Component />);
 
-    expect(addEventListener).toHaveBeenCalled();
     renderer.act(
       () => {
         dispatchKeyDownEvent({key: 'Tab'});
@@ -341,7 +340,12 @@ describe('createTrapTab', () => {
     expect(b.focus).toHaveBeenCalled();
     expect(preventDefault).toHaveBeenCalled();
    });
+   it('short circuits when no focusable elements', () => {
+    const emptyEl = assertDocument().createElement('div');
 
+    trapTab = utils.createTrapTab(emptyEl);
+    trapTab({key: 'Tab', shiftKey: true, preventDefault} as unknown as KeyboardEvent);
+   });
 });
 
 describe('onKeyHandler', () => {
diff --git a/src/app/reactUtils.ts b/src/app/reactUtils.ts
index af1409e2d1..efd90cc70e 100644
--- a/src/app/reactUtils.ts
+++ b/src/app/reactUtils.ts
@@ -86,21 +86,26 @@ export function createTrapTab(...elements: HTMLElement[]) {
   };
 }
 
-export function useTrapTabNavigation(ref: React.MutableRefObject<HTMLElement | null>) {
-  React.useEffect(
-    () => {
-      const el = ref.current;
-      if (!el) {
-        return;
-      }
-      const trapTab = createTrapTab(el);
-
-      document?.addEventListener('keydown', trapTab, true);
-
-      return () => document?.removeEventListener('keydown', trapTab, true);
-    },
-    [ref]
-  );
+  // Supply otherDep in cases where the focusable elements might change due to
+  // additional dependencies (combine them into one variable): see EditCard
+  export function useTrapTabNavigation(
+    ref: React.MutableRefObject<HTMLElement | null>,
+    otherDep?: unknown
+  ) {
+    React.useEffect(
+      () => {
+        const el = ref.current;
+        if (!el?.addEventListener) {
+          return;
+        }
+        const trapTab = createTrapTab(el);
+
+        el.addEventListener('keydown', trapTab, true);
+
+        return () => el.removeEventListener('keydown', trapTab, true);
+      },
+      [ref, otherDep]
+    );
 }
 
 export const onFocusInOrOutHandler = (