From 80ca5e976f0e1818b25d87a85da10232d89c6dee Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro.celaya@hypothes.is>
Date: Wed, 4 Oct 2023 16:39:43 +0200
Subject: [PATCH] Fix SelectNext focus inconsistencies when mixing mouse and
 keyboard interaction

---
 src/components/input/Button.tsx               | 14 ++++++++++--
 src/components/input/SelectNext.tsx           |  1 +
 .../test/use-arrow-key-navigation-test.js     | 22 ++++++++++++++++---
 src/hooks/use-arrow-key-navigation.ts         | 13 ++++++-----
 4 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/src/components/input/Button.tsx b/src/components/input/Button.tsx
index 8404ff53c..26887da5b 100644
--- a/src/components/input/Button.tsx
+++ b/src/components/input/Button.tsx
@@ -27,6 +27,12 @@ type ComponentProps = {
    */
   title?: string;
 
+  /**
+   * Whether the focus ring should be applied to `:focus` or `:focus-visible`.
+   * Defaults to focus-visible.
+   */
+  focusRing?: 'focus' | 'focus-visible';
+
   /** Use `expanded` prop instead */
   'aria-expanded'?: never;
   /** Use `pressed` prop instead */
@@ -64,6 +70,7 @@ export default function Button({
   size = 'md',
   variant = 'secondary',
   unstyled = false,
+  focusRing = 'focus-visible',
 
   role,
   ...htmlAttributes
@@ -96,8 +103,11 @@ export default function Button({
       {...htmlAttributes}
       className={classnames(
         {
-          'focus-visible-ring transition-colors whitespace-nowrap flex items-center':
-            styled,
+          'transition-colors whitespace-nowrap flex items-center': styled,
+        },
+        styled && {
+          'focus-visible-ring': focusRing === 'focus-visible',
+          'focus:ring outline-none': focusRing === 'focus',
         },
         themed && {
           'font-semibold rounded': true,
diff --git a/src/components/input/SelectNext.tsx b/src/components/input/SelectNext.tsx
index 50567f1e7..f84afb9e5 100644
--- a/src/components/input/SelectNext.tsx
+++ b/src/components/input/SelectNext.tsx
@@ -48,6 +48,7 @@ function SelectOption<T>({
   return (
     <Button
       variant="custom"
+      focusRing="focus"
       classes={classnames(
         'w-full ring-inset rounded-none !p-0',
         'border-t first:border-t-0 bg-transparent',
diff --git a/src/hooks/test/use-arrow-key-navigation-test.js b/src/hooks/test/use-arrow-key-navigation-test.js
index 3c1ba74c7..1987fa6fa 100644
--- a/src/hooks/test/use-arrow-key-navigation-test.js
+++ b/src/hooks/test/use-arrow-key-navigation-test.js
@@ -41,6 +41,11 @@ function ToolbarWithToggle({ navigationOptions = {} }) {
       <button data-testid="toggle" onClick={toggleVisible}>
         Toggle
       </button>
+      {visible && (
+        <span data-testid="toolbar-visibility-control">
+          Control element which helps know when re-renders finished
+        </span>
+      )}
       <Toolbar navigationOptions={options} />
     </>
   );
@@ -381,20 +386,31 @@ describe('useArrowKeyNavigation', () => {
       ),
     );
     const toggleToolbar = () => findElementByTestId('toggle').click();
+    const openToolbar = async () => {
+      toggleToolbar();
+      await waitFor(() => !!findElementByTestId('toolbar-visibility-control'));
+    };
+    const closeToolbar = async () => {
+      toggleToolbar();
+      await waitFor(() => !findElementByTestId('toolbar-visibility-control'));
+    };
 
     // No button should be initially focused
     assert.equal(document.activeElement, document.body);
 
     // Once we open the toolbar, the first item will be focused
-    toggleToolbar();
+    await openToolbar();
     await waitFor(() => document.activeElement === findElementByTestId('bold'));
 
     // If we then focus another toolbar item, then close the toolbar and open it
     // again, that same element should be focused again
     pressKey('ArrowDown'); // "italic" is focused
     pressKey('ArrowDown'); // "underline" is focused
-    toggleToolbar(); // Close toolbar
-    toggleToolbar(); // Open toolbar again
+
+    // Close toolbar and then open it again
+    await closeToolbar();
+    await openToolbar();
+
     await waitFor(
       () => document.activeElement === findElementByTestId('underline'),
     );
diff --git a/src/hooks/use-arrow-key-navigation.ts b/src/hooks/use-arrow-key-navigation.ts
index 45e748294..97e2a9275 100644
--- a/src/hooks/use-arrow-key-navigation.ts
+++ b/src/hooks/use-arrow-key-navigation.ts
@@ -94,6 +94,10 @@ export function useArrowKeyNavigation(
   const lastFocusedItem = useRef<HTMLElement | null>(null);
 
   useEffect(() => {
+    if (!containerVisible) {
+      return () => {};
+    }
+
     if (!containerRef.current) {
       throw new Error('Container ref not set');
     }
@@ -196,11 +200,7 @@ export function useArrowKeyNavigation(
     const initialIndex = lastFocusedItem.current
       ? navigableElements.indexOf(lastFocusedItem.current)
       : 0;
-    updateTabIndexes(
-      navigableElements,
-      initialIndex,
-      containerVisible && autofocus,
-    );
+    updateTabIndexes(navigableElements, initialIndex, autofocus);
 
     const listeners = new ListenerCollection();
 
@@ -215,10 +215,11 @@ export function useArrowKeyNavigation(
         lastFocusedItem.current.focus();
         return;
       }
+
       const elements = getNavigableElements();
       const targetIndex = elements.indexOf(event.target as HTMLElement);
       if (targetIndex >= 0) {
-        updateTabIndexes(elements, targetIndex);
+        updateTabIndexes(elements, targetIndex, autofocus);
       }
     });