fix(select): 🐛 focus the popover on loop from both ends (#159)

* fix(select): 🐛  focus the popover on loop from both ends

* test(select): ✅  update tests for default Selected

src/select/Select.ts

@@ -21,8 +21,21 @@ export const useSelect = createHook<SelectOptions, SelectHTMLProps>({
     return { menuRole, hideOnEsc, ...options };
   },
 
-  useProps(options, { onKeyDown: htmlOnKeyDown, ...htmlProps }) {
+  useProps(
+    options,
+    {
+      onClick: htmlOnClick,
+      onKeyDown: htmlOnKeyDown,
+      onMouseDown: htmlOnMouseDown,
+      ...htmlProps
+    },
+  ) {
+    const disabled = options.disabled || htmlProps["aria-disabled"];
+    const hasPressedMouse = React.useRef(false);
+
     const onKeyDownRef = useLiveRef(htmlOnKeyDown);
+    const onClickRef = useLiveRef(htmlOnClick);
+    const onMouseDownRef = useLiveRef(htmlOnMouseDown);
 
     // Reference:
     // https://github.com/chakra-ui/chakra-ui/blob/83eec5b140bd9a69821d8e4df3e69bff0768dcca/packages/menu/src/use-menu.ts#L228-L253
@@ -35,47 +48,85 @@ export const useSelect = createHook<SelectOptions, SelectHTMLProps>({
         onKeyDownRef.current?.(event);
         if (event.defaultPrevented) return;
 
-        // setTimeout on show prevents scroll jump on ArrowUp & ArrowDown
-        const first = () => {
-          if (!options.visible) options.show && setTimeout(options.show);
-          if (!options.selectedValue) options.first?.();
-        };
-        const last = () => {
-          if (!options.visible) options.show && setTimeout(options.show);
-          if (!options.selectedValue) options.last?.();
-        };
-
-        const keyMap = {
-          Enter: first,
-          " ": first,
-          ArrowUp: last,
-          ArrowDown: first,
-        };
-
-        const action = keyMap[event.key as keyof typeof keyMap];
-        action?.();
+        if (event.key === "Escape") {
+          // Doesn't prevent default on Escape, otherwise we can't close
+          // dialogs when MenuButton is focused
+          options.hide?.();
+        } else if (!disabled) {
+          const keyMap = {
+            Enter: options.first,
+            " ": options.first,
+            ArrowUp: options.last,
+            ArrowDown: options.first,
+          };
+
+          const action = keyMap[event.key as keyof typeof keyMap];
+          if (action) {
+            event.preventDefault();
+            event.stopPropagation();
+
+            // setTimeout prevents scroll jump
+            options.show && setTimeout(options.show);
+            if (!options.selectedValue) action();
+            return;
+          }
+        }
       },
       // eslint-disable-next-line react-hooks/exhaustive-deps
       [
-        options.visible,
+        disabled,
         options.show,
-        options.last,
+        options.hide,
         options.first,
-        options.values,
+        options.last,
         options.selectedValue,
-        options.setSelectedValue,
       ],
     );
 
+    const onMouseDown = React.useCallback((event: React.MouseEvent) => {
+      // If the Select has been clicked using mouse or keyboard. On mouse click,
+      // we don't automatically focus the first menu item.
+      hasPressedMouse.current = true;
+      onMouseDownRef.current?.(event);
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, []);
+
+    const onClick = React.useCallback(
+      (event: React.MouseEvent) => {
+        onClickRef.current?.(event);
+        if (event.defaultPrevented) return;
+
+        options.toggle?.();
+
+        // Focus the SelectPopover when it's opened with mouse click.
+        if (hasPressedMouse.current && !options.visible) {
+          options.move?.(null);
+        }
+        hasPressedMouse.current = false;
+      },
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+      [options.show, options.toggle, options.visible, options.move],
+    );
+
     return {
       "aria-haspopup": options.menuRole,
+      onClick,
+      onMouseDown,
       onKeyDown: callAllHandlers(
         onKeyDown,
         onCharacterPress(handleCharacterPress(options)),
       ),
       ...htmlProps,
     };
   },
+
+  useComposeOptions(options) {
+    return {
+      ...options,
+      // Toggling is handled by Select OnClick above
+      toggle: noop,
+    };
+  },
 });
 
 export const Select = createComponent({
@@ -105,6 +156,8 @@ const handleCharacterPress = (options: SelectOptions) => (
   if (nextItem) options.setSelectedValue(nextItem);
 };
 
+const noop = () => {};
+
 export type SelectOptions = PopoverDisclosureOptions &
   SelectStateReturn & {
     /**

src/select/SelectListState.ts

@@ -14,15 +14,11 @@ import {
 export function useSelectListState(
   initialState: SealedInitialState<SelectListInitialState> = {},
 ): SelectListStateReturn {
-  const {
-    currentId = null,
-    orientation = "vertical",
-    loop = true,
-    ...sealed
-  } = useSealedState(initialState);
+  const { orientation = "vertical", loop = true, ...sealed } = useSealedState(
+    initialState,
+  );
 
   const composite = useCompositeState({
-    currentId,
     orientation,
     loop,
     ...sealed,

src/select/SelectPopoverState.ts

@@ -21,14 +21,10 @@ export function useSelectPopoverState(
   const visible = popover.visible;
 
   React.useEffect(() => {
-    if (visible && selectedId) {
-      move?.(selectedId);
-    }
-
-    if (!visible) {
-      // We need to reset select.moves
-      reset();
-    }
+    if (visible && selectedId) move?.(selectedId);
+
+    // We need to reset select.moves
+    if (!visible) reset();
   }, [visible, reset, selectedId, move]);
 
   return {

src/select/__tests__/Select.test.tsx

@@ -98,25 +98,24 @@ describe("Select", () => {
   });
 
   it("should behave properly with default selected", () => {
-    render(<SelectComponent selectedValue={"orange"} />);
+    render(<SelectComponent selectedValue="Orange" />);
 
     const popover = screen.getByTestId("popover");
     const popoverButton = screen.getByRole("button", {
       name: /fruit/i,
     });
 
-    expect(popoverButton).toHaveTextContent(/orange/i);
+    expect(popoverButton).toHaveTextContent(/Orange/i);
     expect(popover).not.toBeVisible();
 
     press.Tab();
     press.ArrowDown();
     jest.runAllTimers();
     expect(popover).toBeVisible();
 
-    press.Tab();
     press.Enter();
     expect(popover).not.toBeVisible();
-    expect(popoverButton).toHaveTextContent(/orange/i);
+    expect(popoverButton).toHaveTextContent(/Orange/i);
   });
 
   test("typeahead should work properly when popover is not open", () => {

src/select/__tests__/__snapshots__/Select.test.tsx.snap

@@ -14,6 +14,7 @@ exports[`Select should render correctly 1`] = `
       Select a fruit
     </button>
     <div
+      aria-activedescendant="select-apple"
       aria-label="Fruits"
       data-dialog="true"
       data-testid="popover"
@@ -24,6 +25,7 @@ exports[`Select should render correctly 1`] = `
       tabindex="0"
     >
       <div
+        aria-selected="true"
         id="select-apple"
         role="option"
         tabindex="-1"