Fix SelectNext focus inconsistencies when mixing mouse and keyboard i…

…nteraction
hypothesis · Oct 5, 2023 · c03227d · c03227d
1 parent f3ac2d1
commit c03227d
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 11 deletions.
diff --git 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
@@ -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
@@ -386,15 +386,16 @@ describe('useArrowKeyNavigation', () => {
     assert.equal(document.activeElement, document.body);
 
     // Once we open the toolbar, the first item will be focused
-    toggleToolbar();
+    await act(() => toggleToolbar());
     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
+    await act(() => toggleToolbar()); // Close toolbar
+    await act(() => toggleToolbar()); // Open toolbar again
+
     await waitFor(
       () => document.activeElement === findElementByTestId('underline'),
     );

diff --git 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);
       }
     });