diff --git a/src/hooks/__tests__/utils.test.js b/src/hooks/__tests__/utils.test.js index 5fb8544b..7fc62bbe 100644 --- a/src/hooks/__tests__/utils.test.js +++ b/src/hooks/__tests__/utils.test.js @@ -149,7 +149,7 @@ describe('utils', () => { ) expect(result.current).toEqual({ - current: {isMouseDown: false, isTouchMove: false}, + current: {isMouseDown: false, isTouchMove: false, isTouchEnd: false}, }) }) }) diff --git a/src/hooks/useCombobox/__tests__/getItemProps.test.js b/src/hooks/useCombobox/__tests__/getItemProps.test.js index a8bb88fc..190f4004 100644 --- a/src/hooks/useCombobox/__tests__/getItemProps.test.js +++ b/src/hooks/useCombobox/__tests__/getItemProps.test.js @@ -280,6 +280,37 @@ describe('getItemProps', () => { await mouseMoveItemAtIndex(disabledIndex) expect(input).toHaveAttribute('aria-activedescendant', '') }) + + // Test that we don't call the mouse move handler on mobile. + test('does not set highlight the item if a touch event ocurred', async () => { + let touchEndHandler + const index = 1 + + renderCombobox({ + isOpen: true, + environment: { + addEventListener: (name, handler) => { + // eslint-disable-next-line jest/no-conditional-in-test + if (name === 'touchend') { + touchEndHandler = handler + } + }, + removeEventListener: () => {}, + document: { + createElement: () => {}, + getElementById: () => {}, + activeElement: () => {}, + body: {}, + }, + Node: () => {}, + }, + }) + + act(() => touchEndHandler({target: null})) + await mouseMoveItemAtIndex(index) + + expect(getInput()).toHaveAttribute('aria-activedescendant', '') + }) }) describe('on click', () => { diff --git a/src/hooks/useCombobox/__tests__/props.test.js b/src/hooks/useCombobox/__tests__/props.test.js index f1e8101c..03cd3cfb 100644 --- a/src/hooks/useCombobox/__tests__/props.test.js +++ b/src/hooks/useCombobox/__tests__/props.test.js @@ -93,6 +93,7 @@ describe('props', () => { describe('selectedItemChanged', () => { test('props update of selectedItem will update inputValue state with default selectedItemChanged referential equality check', () => { + const initialSelectedItem = {id: 3, value: 'init'} const selectedItem = {id: 1, value: 'wow'} const newSelectedItem = {id: 1, value: 'not wow'} function itemToString(item) { @@ -103,6 +104,14 @@ describe('props', () => { .mockImplementation((_state, {changes}) => changes) const {rerender} = renderCombobox({ + stateReducer, + itemToString, + selectedItem: initialSelectedItem, + }) + + expect(stateReducer).not.toHaveBeenCalled() // won't get called on first render + + rerender({ stateReducer, itemToString, selectedItem, @@ -111,7 +120,7 @@ describe('props', () => { expect(stateReducer).toHaveBeenCalledTimes(1) expect(stateReducer).toHaveBeenCalledWith( { - inputValue: itemToString(selectedItem), + inputValue: itemToString(initialSelectedItem), selectedItem, highlightedIndex: -1, isOpen: false, @@ -155,15 +164,15 @@ describe('props', () => { expect(getInput()).toHaveValue(itemToString(newSelectedItem)) }) - test('props update of selectedItem will not update inputValue state', () => { + test('props update of selectedItem will not update inputValue state if selectedItemChanged returns false', () => { + const initialSelectedItem = {id: 1, value: 'hmm'} const selectedItem = {id: 1, value: 'wow'} - const newSelectedItem = {id: 1, value: 'not wow'} function itemToString(item) { return item.value } const selectedItemChanged = jest .fn() - .mockReturnValue((prev, next) => prev.id !== next.id) + .mockImplementation((prev, next) => prev.id !== next.id) const stateReducer = jest .fn() .mockImplementation((_state, {changes}) => changes) @@ -171,30 +180,23 @@ describe('props', () => { const {rerender} = renderCombobox({ selectedItemChanged, stateReducer, - selectedItem, + selectedItem: initialSelectedItem, itemToString, }) - expect(getInput()).toHaveValue(itemToString(selectedItem)) - expect(selectedItemChanged).toHaveBeenCalledTimes(1) - expect(selectedItemChanged).toHaveBeenCalledWith(undefined, selectedItem) - - stateReducer.mockReset() - selectedItemChanged.mockReset() rerender({ + selectedItemChanged, stateReducer, + selectedItem, itemToString, - selectedItem: newSelectedItem, - selectedItemChanged, }) + expect(getInput()).toHaveValue(itemToString(initialSelectedItem)) expect(selectedItemChanged).toHaveBeenCalledTimes(1) expect(selectedItemChanged).toHaveBeenCalledWith( + initialSelectedItem, selectedItem, - newSelectedItem, ) - expect(stateReducer).not.toHaveBeenCalled() - expect(getInput()).toHaveValue(itemToString(selectedItem)) }) }) diff --git a/src/hooks/useCombobox/index.js b/src/hooks/useCombobox/index.js index a1d4c82d..b932d9e7 100644 --- a/src/hooks/useCombobox/index.js +++ b/src/hooks/useCombobox/index.js @@ -11,7 +11,8 @@ import { useElementIds, getItemAndIndex, getInitialValue, - isDropdownsStateEqual + isDropdownsStateEqual, + useIsInitialMount, } from '../utils' import { getInitialState, @@ -44,7 +45,7 @@ function useCombobox(userProps = {}) { downshiftUseComboboxReducer, props, getInitialState, - isDropdownsStateEqual + isDropdownsStateEqual, ) const {isOpen, highlightedIndex, selectedItem, inputValue} = state @@ -53,7 +54,8 @@ function useCombobox(userProps = {}) { const itemRefs = useRef({}) const inputRef = useRef(null) const toggleButtonRef = useRef(null) - const isInitialMountRef = useRef(true) + const isInitialMount = useIsInitialMount() + // prevent id re-generation between renders. const elementIds = useElementIds(props) // used to keep track of how many items we had on previous cycle. @@ -72,7 +74,6 @@ function useCombobox(userProps = {}) { getA11yStatusMessage, [isOpen, highlightedIndex, inputValue, items], { - isInitialMount: isInitialMountRef.current, previousResultCount: previousResultCountRef.current, items, environment, @@ -82,7 +83,6 @@ function useCombobox(userProps = {}) { ) // Sets a11y status message on changes in selectedItem. useA11yMessageSetter(getA11ySelectionMessage, [selectedItem], { - isInitialMount: isInitialMountRef.current, previousResultCount: previousResultCountRef.current, items, environment, @@ -99,7 +99,6 @@ function useCombobox(userProps = {}) { getItemNodeFromIndex, }) useControlPropsValidator({ - isInitialMount: isInitialMountRef.current, props, state, }) @@ -113,11 +112,9 @@ function useCombobox(userProps = {}) { // eslint-disable-next-line react-hooks/exhaustive-deps }, []) useEffect(() => { - if (isInitialMountRef.current) { - return + if (!isInitialMount) { + previousResultCountRef.current = items.length } - - previousResultCountRef.current = items.length }) // Add mouse/touch events to document. const mouseAndTouchTrackersRef = useMouseAndTouchTracker( @@ -135,14 +132,6 @@ function useCombobox(userProps = {}) { 'getInputProps', 'getMenuProps', ) - // Make initial ref false. - useEffect(() => { - isInitialMountRef.current = false - - return () => { - isInitialMountRef.current = true - } - }, []) // Reset itemRefs on close. useEffect(() => { if (!isOpen) { @@ -319,10 +308,15 @@ function useCombobox(userProps = {}) { : onClick const itemHandleMouseMove = () => { - if (index === latestState.highlightedIndex) { + if ( + mouseAndTouchTrackersRef.current.isTouchEnd || + index === latestState.highlightedIndex + ) { return } + shouldScrollRef.current = false + dispatch({ type: stateChangeTypes.ItemMouseMove, index, @@ -358,7 +352,8 @@ function useCombobox(userProps = {}) { ...rest, } }, - [dispatch, latest, shouldScrollRef, elementIds], + + [dispatch, elementIds, latest, mouseAndTouchTrackersRef, shouldScrollRef], ) const getToggleButtonProps = useCallback( diff --git a/src/hooks/useCombobox/utils.js b/src/hooks/useCombobox/utils.js index ab75e226..b8aa9ba6 100644 --- a/src/hooks/useCombobox/utils.js +++ b/src/hooks/useCombobox/utils.js @@ -11,6 +11,7 @@ import { defaultProps as defaultPropsCommon, getInitialState as getInitialStateCommon, useEnhancedReducer, + useIsInitialMount, } from '../utils' import {ControlledPropUpdatedSelectedItem} from './stateChangeTypes' @@ -61,22 +62,28 @@ const propTypes = { * @param {Function} isStateEqual Function that checks if a previous state is equal to the next. * @returns {Array} An array with the state and an action dispatcher. */ -export function useControlledReducer(reducer, props, createInitialState, isStateEqual) { +export function useControlledReducer( + reducer, + props, + createInitialState, + isStateEqual, +) { const previousSelectedItemRef = useRef() const [state, dispatch] = useEnhancedReducer( reducer, props, createInitialState, - isStateEqual + isStateEqual, ) + const isInitialMount = useIsInitialMount() - // ToDo: if needed, make same approach as selectedItemChanged from Downshift. useEffect(() => { if (!isControlledProp(props, 'selectedItem')) { return } if ( + !isInitialMount && // on first mount we already have the proper inputValue for a initial selected item. props.selectedItemChanged( previousSelectedItemRef.current, props.selectedItem, diff --git a/src/hooks/useMultipleSelection/index.js b/src/hooks/useMultipleSelection/index.js index 42fdbcc4..e8e95186 100644 --- a/src/hooks/useMultipleSelection/index.js +++ b/src/hooks/useMultipleSelection/index.js @@ -8,13 +8,14 @@ import { useLatestRef, useControlPropsValidator, getItemAndIndex, + useIsInitialMount, } from '../utils' import { getInitialState, defaultProps, isKeyDownOperationPermitted, validatePropTypes, - isStateEqual + isStateEqual, } from './utils' import downshiftMultipleSelectionReducer from './reducer' import * as stateChangeTypes from './stateChangeTypes' @@ -41,12 +42,12 @@ function useMultipleSelection(userProps = {}) { downshiftMultipleSelectionReducer, props, getInitialState, - isStateEqual + isStateEqual, ) const {activeIndex, selectedItems} = state // Refs. - const isInitialMountRef = useRef(true) + const isInitialMount = useIsInitialMount() const dropdownRef = useRef(null) const previousSelectedItemsRef = useRef(selectedItems) const selectedItemRefs = useRef() @@ -56,7 +57,7 @@ function useMultipleSelection(userProps = {}) { // Effects. /* Sets a11y status message on changes in selectedItem. */ useEffect(() => { - if (isInitialMountRef.current || isReactNative || !environment?.document) { + if (isInitialMount || isReactNative || !environment?.document) { return } @@ -83,7 +84,7 @@ function useMultipleSelection(userProps = {}) { }, [selectedItems.length]) // Sets focus on active item. useEffect(() => { - if (isInitialMountRef.current) { + if (isInitialMount) { return } @@ -92,21 +93,12 @@ function useMultipleSelection(userProps = {}) { } else if (selectedItemRefs.current[activeIndex]) { selectedItemRefs.current[activeIndex].focus() } - }, [activeIndex]) + }, [activeIndex, isInitialMount]) useControlPropsValidator({ - isInitialMount: isInitialMountRef.current, props, state, }) const setGetterPropCallInfo = useGetterPropsCalledChecker('getDropdownProps') - // Make initial ref false. - useEffect(() => { - isInitialMountRef.current = false - - return () => { - isInitialMountRef.current = true - } - }, []) // Event handler functions. const selectedItemKeyDownHandlers = useMemo( diff --git a/src/hooks/useSelect/__tests__/getItemProps.test.js b/src/hooks/useSelect/__tests__/getItemProps.test.js index 2f1492da..4643f256 100644 --- a/src/hooks/useSelect/__tests__/getItemProps.test.js +++ b/src/hooks/useSelect/__tests__/getItemProps.test.js @@ -9,8 +9,9 @@ import { getItems, keyDownOnToggleButton, clickOnToggleButton, + items, + defaultIds, } from '../testUtils' -import {items, defaultIds} from '../../testUtils' import useSelect from '..' describe('getItemProps', () => { @@ -305,6 +306,37 @@ describe('getItemProps', () => { await mouseMoveItemAtIndex(disabledIndex) expect(toggleButton).toHaveAttribute('aria-activedescendant', '') }) + + // Test that we don't call the mouse move handler on mobile. + test('does not set highlight the item if a touch event ocurred', async () => { + let touchEndHandler + const index = 1 + + renderSelect({ + isOpen: true, + environment: { + addEventListener: (name, handler) => { + // eslint-disable-next-line jest/no-conditional-in-test + if (name === 'touchend') { + touchEndHandler = handler + } + }, + removeEventListener: () => {}, + document: { + createElement: () => {}, + getElementById: () => {}, + activeElement: () => {}, + body: {}, + }, + Node: () => {}, + }, + }) + + act(() => touchEndHandler({target: null})) + await mouseMoveItemAtIndex(index) + + expect(getToggleButton()).toHaveAttribute('aria-activedescendant', '') + }) }) describe('on click', () => { diff --git a/src/hooks/useSelect/index.js b/src/hooks/useSelect/index.js index 506c0e10..35be7cd8 100644 --- a/src/hooks/useSelect/index.js +++ b/src/hooks/useSelect/index.js @@ -13,6 +13,7 @@ import { getItemAndIndex, getInitialValue, isDropdownsStateEqual, + useIsInitialMount, } from '../utils' import { callAllEventHandlers, @@ -60,7 +61,7 @@ function useSelect(userProps = {}) { const elementIds = useElementIds(props) // used to keep track of how many items we had on previous cycle. const previousResultCountRef = useRef() - const isInitialMountRef = useRef(true) + const isInitialMount = useIsInitialMount() // utility callback to get item element. const latest = useLatestRef({ state, @@ -79,7 +80,6 @@ function useSelect(userProps = {}) { getA11yStatusMessage, [isOpen, highlightedIndex, inputValue, items], { - isInitialMount: isInitialMountRef.current, previousResultCount: previousResultCountRef.current, items, environment, @@ -89,7 +89,6 @@ function useSelect(userProps = {}) { ) // Sets a11y status message on changes in selectedItem. useA11yMessageSetter(getA11ySelectionMessage, [selectedItem], { - isInitialMount: isInitialMountRef.current, previousResultCount: previousResultCountRef.current, items, environment, @@ -132,12 +131,11 @@ function useSelect(userProps = {}) { }, [dispatch, inputValue]) useControlPropsValidator({ - isInitialMount: isInitialMountRef.current, props, state, }) useEffect(() => { - if (isInitialMountRef.current) { + if (isInitialMount) { return } @@ -167,14 +165,6 @@ function useSelect(userProps = {}) { 'getMenuProps', 'getToggleButtonProps', ) - // Make initial ref false. - useEffect(() => { - isInitialMountRef.current = false - - return () => { - isInitialMountRef.current = true - } - }, []) // Reset itemRefs on close. useEffect(() => { if (!isOpen) { @@ -481,7 +471,10 @@ function useSelect(userProps = {}) { const disabled = latestProps.isItemDisabled(item, index) const itemHandleMouseMove = () => { - if (index === latestState.highlightedIndex) { + if ( + mouseAndTouchTrackersRef.current.isTouchEnd || + index === latestState.highlightedIndex + ) { return } shouldScrollRef.current = false @@ -532,7 +525,7 @@ function useSelect(userProps = {}) { return itemProps }, - [latest, elementIds, shouldScrollRef, dispatch], + [latest, elementIds, mouseAndTouchTrackersRef, shouldScrollRef, dispatch], ) return { diff --git a/src/hooks/utils.js b/src/hooks/utils.js index ea97d3f3..d620d5f2 100644 --- a/src/hooks/utils.js +++ b/src/hooks/utils.js @@ -220,17 +220,12 @@ function useEnhancedReducer(reducer, props, createInitialState, isStateEqual) { const action = actionRef.current useEffect(() => { + const prevState = getState(prevStateRef.current, action?.props) const shouldCallOnChangeProps = - action && - prevStateRef.current && - !isStateEqual(prevStateRef.current, state) + action && prevStateRef.current && !isStateEqual(prevState, state) if (shouldCallOnChangeProps) { - callOnChangeProps( - action, - getState(prevStateRef.current, action.props), - state, - ) + callOnChangeProps(action, prevState, state) } prevStateRef.current = state @@ -371,6 +366,7 @@ function useMouseAndTouchTracker( const mouseAndTouchTrackersRef = useRef({ isMouseDown: false, isTouchMove: false, + isTouchEnd: false, }) useEffect(() => { @@ -381,10 +377,12 @@ function useMouseAndTouchTracker( // The same strategy for checking if a click occurred inside or outside downshift // as in downshift.js. const onMouseDown = () => { + mouseAndTouchTrackersRef.current.isTouchEnd = false // reset this one. mouseAndTouchTrackersRef.current.isMouseDown = true } const onMouseUp = event => { mouseAndTouchTrackersRef.current.isMouseDown = false + if ( isOpen && !targetWithinDownshift( @@ -397,12 +395,15 @@ function useMouseAndTouchTracker( } } const onTouchStart = () => { + mouseAndTouchTrackersRef.current.isTouchEnd = false mouseAndTouchTrackersRef.current.isTouchMove = false } const onTouchMove = () => { mouseAndTouchTrackersRef.current.isTouchMove = true } const onTouchEnd = event => { + mouseAndTouchTrackersRef.current.isTouchEnd = true + if ( isOpen && !mouseAndTouchTrackersRef.current.isTouchMove && @@ -501,8 +502,9 @@ if (process.env.NODE_ENV !== 'production') { function useA11yMessageSetter( getA11yMessage, dependencyArray, - {isInitialMount, highlightedIndex, items, environment, ...rest}, + {highlightedIndex, items, environment, ...rest}, ) { + const isInitialMount = useIsInitialMount() // Sets a11y status message on changes in state. useEffect(() => { if (isInitialMount || isReactNative || !environment?.document) { @@ -558,9 +560,10 @@ function useScrollIntoView({ let useControlPropsValidator = noop /* istanbul ignore next */ if (process.env.NODE_ENV !== 'production') { - useControlPropsValidator = ({isInitialMount, props, state}) => { + useControlPropsValidator = ({props, state}) => { // used for checking when props are moving from controlled to uncontrolled. const prevPropsRef = useRef(props) + const isInitialMount = useIsInitialMount() useEffect(() => { if (isInitialMount) { @@ -615,6 +618,23 @@ function isDropdownsStateEqual(prevState, newState) { ) } +/** + * Tracks if it's the first render. + */ +function useIsInitialMount() { + const isInitialMountRef = React.useRef(true) + + React.useEffect(() => { + isInitialMountRef.current = false + + return () => { + isInitialMountRef.current = true + } + }, []) + + return isInitialMountRef.current +} + // Shared between all exports. const commonPropTypes = { environment: PropTypes.shape({ @@ -679,4 +699,5 @@ export { isDropdownsStateEqual, commonDropdownPropTypes, commonPropTypes, + useIsInitialMount, } diff --git a/src/utils.js b/src/utils.js index ad56af90..23b99223 100644 --- a/src/utils.js +++ b/src/utils.js @@ -1,4 +1,4 @@ -import { compute } from 'compute-scroll-into-view' +import {compute} from 'compute-scroll-into-view' import React from 'react' import {isPreact} from './is.macro' @@ -267,6 +267,10 @@ function pickState(state = {}) { * @returns {Object} The merged controlled state. */ function getState(state, props) { + if (!state || !props) { + return state + } + return Object.keys(state).reduce((prevState, key) => { prevState[key] = isControlledProp(props, key) ? props[key] : state[key] @@ -422,16 +426,19 @@ function targetWithinDownshift( environment, checkActiveElement = true, ) { - return environment && downshiftElements.some( - contextNode => - contextNode && - (isOrContainsNode(contextNode, target, environment) || - (checkActiveElement && - isOrContainsNode( - contextNode, - environment.document.activeElement, - environment, - ))), + return ( + environment && + downshiftElements.some( + contextNode => + contextNode && + (isOrContainsNode(contextNode, target, environment) || + (checkActiveElement && + isOrContainsNode( + contextNode, + environment.document.activeElement, + environment, + ))), + ) ) }