From 3eafebd9b038d9cb2f537f01442ccc989222c877 Mon Sep 17 00:00:00 2001 From: Navin Moorthy Date: Fri, 20 Nov 2020 16:12:58 +0530 Subject: [PATCH] =?UTF-8?q?fix(select):=20=F0=9F=90=9B=20=20focus=20the=20?= =?UTF-8?q?popover=20on=20loop=20from=20both=20ends=20(#159)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(select): 🐛 focus the popover on loop from both ends * test(select): ✅ update tests for default Selected --- src/select/Select.ts | 101 +++++++++++++----- src/select/SelectListState.ts | 10 +- src/select/SelectPopoverState.ts | 12 +-- src/select/__tests__/Select.test.tsx | 7 +- .../__snapshots__/Select.test.tsx.snap | 2 + 5 files changed, 89 insertions(+), 43 deletions(-) diff --git a/src/select/Select.ts b/src/select/Select.ts index 0369bb288..dcbd06b13 100644 --- a/src/select/Select.ts +++ b/src/select/Select.ts @@ -21,8 +21,21 @@ export const useSelect = createHook({ 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,40 +48,70 @@ export const useSelect = createHook({ 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)), @@ -76,6 +119,14 @@ export const useSelect = createHook({ ...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 & { /** diff --git a/src/select/SelectListState.ts b/src/select/SelectListState.ts index 954e3bf11..c778739b4 100644 --- a/src/select/SelectListState.ts +++ b/src/select/SelectListState.ts @@ -14,15 +14,11 @@ import { export function useSelectListState( initialState: SealedInitialState = {}, ): 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, diff --git a/src/select/SelectPopoverState.ts b/src/select/SelectPopoverState.ts index 168106866..3b6d9ac32 100644 --- a/src/select/SelectPopoverState.ts +++ b/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 { diff --git a/src/select/__tests__/Select.test.tsx b/src/select/__tests__/Select.test.tsx index 70fcd1723..49ae69035 100644 --- a/src/select/__tests__/Select.test.tsx +++ b/src/select/__tests__/Select.test.tsx @@ -98,14 +98,14 @@ describe("Select", () => { }); it("should behave properly with default selected", () => { - render(); + render(); 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(); @@ -113,10 +113,9 @@ describe("Select", () => { 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", () => { diff --git a/src/select/__tests__/__snapshots__/Select.test.tsx.snap b/src/select/__tests__/__snapshots__/Select.test.tsx.snap index 198c9d305..d5cfcdbfe 100644 --- a/src/select/__tests__/__snapshots__/Select.test.tsx.snap +++ b/src/select/__tests__/__snapshots__/Select.test.tsx.snap @@ -14,6 +14,7 @@ exports[`Select should render correctly 1`] = ` Select a fruit