Fix SelectNext focus inconsistencies when mixing mouse and keyboard interaction

Author: acelaya

src/components/input/SelectNext.tsx

@@ -46,15 +46,20 @@ function SelectOption<T>({
   const selected = !disabled && currentValue === value;
 
   return (
-    <Button
-      variant="custom"
-      classes={classnames(
-        'w-full ring-inset rounded-none !p-0',
-        'border-t first:border-t-0 bg-transparent',
+    <li
+      className={classnames(
+        'w-full ring-inset focus:ring outline-none rounded-none cursor-pointer',
+        'border-t first:border-t-0 transition-colors whitespace-nowrap',
         { 'hover:bg-grey-1': !disabled },
         classes,
       )}
       onClick={() => selectValue(value)}
+      onKeyPress={e => {
+        if (e.code === 'Enter' || e.code === 'Space') {
+          e.preventDefault();
+          selectValue(value);
+        }
+      }}
       role="option"
       disabled={disabled}
       aria-selected={selected}
@@ -69,7 +74,7 @@ function SelectOption<T>({
       >
         {children({ selected, disabled })}
       </div>
-    </Button>
+    </li>
   );
 }
 
@@ -139,7 +144,7 @@ function SelectMain<T>({
   const [listboxOpen, setListboxOpen] = useState(false);
   const closeListbox = useCallback(() => setListboxOpen(false), []);
   const wrapperRef = useRef<HTMLDivElement | null>(null);
-  const listboxRef = useRef<HTMLDivElement | null>(null);
+  const listboxRef = useRef<HTMLUListElement | null>(null);
   const listboxId = useId();
   const buttonRef = useSyncedRef(elementRef);
   const defaultButtonId = useId();
@@ -168,6 +173,7 @@ function SelectMain<T>({
     loop: false,
     autofocus: true,
     containerVisible: listboxOpen,
+    selector: '[role="option"]',
   });
 
   useLayoutEffect(() => {
@@ -209,7 +215,7 @@ function SelectMain<T>({
         </div>
       </Button>
       <SelectContext.Provider value={{ selectValue, value }}>
-        <div
+        <ul
           className={classnames(
             'absolute z-5 w-full max-h-80 overflow-y-auto',
             'rounded border bg-white shadow hover:shadow-md focus-within:shadow-md',
@@ -231,7 +237,7 @@ function SelectMain<T>({
           data-testid="select-listbox"
         >
           {children}
-        </div>
+        </ul>
       </SelectContext.Provider>
     </div>
   );

src/components/input/test/SelectNext-test.js

@@ -68,17 +68,41 @@ describe('SelectNext', () => {
   it('changes selected value when an option is clicked', () => {
     const onChange = sinon.stub();
     const wrapper = createComponent({ onChange });
+    const clickOption = index =>
+      wrapper.find(`[data-testid="option-${index}"]`).simulate('click');
 
-    wrapper.find('[data-testid="option-3"]').simulate('click');
+    clickOption(3);
     assert.calledWith(onChange.lastCall, items[2]);
 
-    wrapper.find('[data-testid="option-5"]').simulate('click');
+    clickOption(5);
     assert.calledWith(onChange.lastCall, items[4]);
 
-    wrapper.find('[data-testid="option-1"]').simulate('click');
+    clickOption(1);
     assert.calledWith(onChange.lastCall, items[0]);
   });
 
+  ['Enter', 'Space'].forEach(code => {
+    it(`changes selected value when ${code} is pressed in option`, () => {
+      const onChange = sinon.stub();
+      const wrapper = createComponent({ onChange });
+      const pressKeyInOption = index =>
+        wrapper
+          .find(`[data-testid="option-${index}"]`)
+          .getDOMNode()
+          .closest('[role="option"]')
+          .dispatchEvent(new KeyboardEvent('keypress', { code }));
+
+      pressKeyInOption(3);
+      assert.calledWith(onChange.lastCall, items[2]);
+
+      pressKeyInOption(5);
+      assert.calledWith(onChange.lastCall, items[4]);
+
+      pressKeyInOption(1);
+      assert.calledWith(onChange.lastCall, items[0]);
+    });
+  });
+
   it('marks the right item as selected', () => {
     const wrapper = createComponent({ value: items[2] });
     const isOptionSelected = id =>
@@ -177,7 +201,7 @@ describe('SelectNext', () => {
     wrapper
       .find('[data-testid="option-3"]')
       .getDOMNode()
-      .closest('button')
+      .closest('[role="option"]')
       .focus();
     toggleListbox(wrapper);
     wrapper.update();

src/hooks/test/use-arrow-key-navigation-test.js

@@ -380,21 +380,23 @@ describe('useArrowKeyNavigation', () => {
         container,
       ),
     );
-    const toggleToolbar = () => findElementByTestId('toggle').click();
+    const toggleToolbar = () =>
+      act(() => findElementByTestId('toggle').click());
 
     // 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 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 toggleToolbar(); // Close toolbar
+    await toggleToolbar(); // Open toolbar again
+
     await waitFor(
       () => document.activeElement === findElementByTestId('underline'),
     );

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