From 7997f762b6d737e9256be28f20d861e3bb469253 Mon Sep 17 00:00:00 2001 From: Silviu Alexandru Avram Date: Wed, 13 Mar 2024 10:14:24 +0200 Subject: [PATCH] feat(hooks): remove mouse and touch events on unmount (#1581) * feat(hooks): remove mouse and touch events on unmount * revert unrelated change --- src/hooks/__tests__/utils.test.js | 18 +++++++---- src/hooks/useCombobox/index.js | 39 ++++++++++++----------- src/hooks/useSelect/index.js | 37 +++++++++++----------- src/hooks/utils.js | 52 ++++++++++++------------------- 4 files changed, 72 insertions(+), 74 deletions(-) diff --git a/src/hooks/__tests__/utils.test.js b/src/hooks/__tests__/utils.test.js index 7fc62bbe..1cd40c40 100644 --- a/src/hooks/__tests__/utils.test.js +++ b/src/hooks/__tests__/utils.test.js @@ -84,9 +84,7 @@ describe('utils', () => { describe('useMouseAndTouchTracker', () => { test('renders without error', () => { expect(() => { - renderHook(() => - useMouseAndTouchTracker(false, [], undefined, jest.fn()), - ) + renderHook(() => useMouseAndTouchTracker(undefined, [], jest.fn())) }).not.toThrowError() }) @@ -95,9 +93,11 @@ describe('utils', () => { addEventListener: jest.fn(), removeEventListener: jest.fn(), } + const refs = [] + const handleBlur = jest.fn() - const {unmount, result} = renderHook(() => - useMouseAndTouchTracker(false, [], environment, jest.fn()), + const {unmount, rerender, result} = renderHook(() => + useMouseAndTouchTracker(environment, refs, handleBlur), ) expect(environment.addEventListener).toHaveBeenCalledTimes(5) @@ -123,6 +123,10 @@ describe('utils', () => { ) expect(environment.removeEventListener).not.toHaveBeenCalled() + rerender() + + expect(environment.removeEventListener).not.toHaveBeenCalled() + unmount() expect(environment.addEventListener).toHaveBeenCalledTimes(5) @@ -149,7 +153,9 @@ describe('utils', () => { ) expect(result.current).toEqual({ - current: {isMouseDown: false, isTouchMove: false, isTouchEnd: false}, + isMouseDown: false, + isTouchMove: false, + isTouchEnd: false, }) }) }) diff --git a/src/hooks/useCombobox/index.js b/src/hooks/useCombobox/index.js index b932d9e7..bd256ccd 100644 --- a/src/hooks/useCombobox/index.js +++ b/src/hooks/useCombobox/index.js @@ -116,17 +116,20 @@ function useCombobox(userProps = {}) { previousResultCountRef.current = items.length } }) - // Add mouse/touch events to document. - const mouseAndTouchTrackersRef = useMouseAndTouchTracker( - isOpen, - [inputRef, menuRef, toggleButtonRef], + const mouseAndTouchTrackers = useMouseAndTouchTracker( environment, - () => { - dispatch({ - type: stateChangeTypes.InputBlur, - selectItem: false, - }) - }, + [toggleButtonRef, menuRef, inputRef], + useCallback( + function handleBlur() { + if (latest.current.state.isOpen) { + dispatch({ + type: stateChangeTypes.InputBlur, + selectItem: false, + }) + } + }, + [dispatch, latest], + ), ) const setGetterPropCallInfo = useGetterPropsCalledChecker( 'getInputProps', @@ -309,7 +312,7 @@ function useCombobox(userProps = {}) { const itemHandleMouseMove = () => { if ( - mouseAndTouchTrackersRef.current.isTouchEnd || + mouseAndTouchTrackers.isTouchEnd || index === latestState.highlightedIndex ) { return @@ -353,7 +356,7 @@ function useCombobox(userProps = {}) { } }, - [dispatch, elementIds, latest, mouseAndTouchTrackersRef, shouldScrollRef], + [dispatch, elementIds, latest, mouseAndTouchTrackers, shouldScrollRef], ) const getToggleButtonProps = useCallback( @@ -425,7 +428,7 @@ function useCombobox(userProps = {}) { if ( environment?.document && latestState.isOpen && - !mouseAndTouchTrackersRef.current.isMouseDown + !mouseAndTouchTrackers.isMouseDown ) { const isBlurByTabChange = event.relatedTarget === null && @@ -501,13 +504,13 @@ function useCombobox(userProps = {}) { } }, [ - setGetterPropCallInfo, - latest, - elementIds, - inputKeyDownHandlers, dispatch, - mouseAndTouchTrackersRef, + elementIds, environment, + inputKeyDownHandlers, + latest, + mouseAndTouchTrackers, + setGetterPropCallInfo, ], ) diff --git a/src/hooks/useSelect/index.js b/src/hooks/useSelect/index.js index 35be7cd8..bead1dfe 100644 --- a/src/hooks/useSelect/index.js +++ b/src/hooks/useSelect/index.js @@ -150,16 +150,20 @@ function useSelect(userProps = {}) { } // eslint-disable-next-line react-hooks/exhaustive-deps }, []) - // Add mouse/touch events to document. - const mouseAndTouchTrackersRef = useMouseAndTouchTracker( - isOpen, - [menuRef, toggleButtonRef], + + const mouseAndTouchTrackers = useMouseAndTouchTracker( environment, - () => { - dispatch({ - type: stateChangeTypes.ToggleButtonBlur, - }) - }, + [toggleButtonRef, menuRef], + useCallback( + function handleBlur() { + if (latest.current.state.isOpen) { + dispatch({ + type: stateChangeTypes.ToggleButtonBlur, + }) + } + }, + [dispatch, latest], + ), ) const setGetterPropCallInfo = useGetterPropsCalledChecker( 'getMenuProps', @@ -365,10 +369,7 @@ function useSelect(userProps = {}) { }) } const toggleButtonHandleBlur = () => { - if ( - latestState.isOpen && - !mouseAndTouchTrackersRef.current.isMouseDown - ) { + if (latestState.isOpen && !mouseAndTouchTrackers.isMouseDown) { dispatch({ type: stateChangeTypes.ToggleButtonBlur, }) @@ -434,11 +435,11 @@ function useSelect(userProps = {}) { return toggleProps }, [ - latest, + dispatch, elementIds, + latest, + mouseAndTouchTrackers, setGetterPropCallInfo, - dispatch, - mouseAndTouchTrackersRef, toggleButtonKeyDownHandlers, ], ) @@ -472,7 +473,7 @@ function useSelect(userProps = {}) { const itemHandleMouseMove = () => { if ( - mouseAndTouchTrackersRef.current.isTouchEnd || + mouseAndTouchTrackers.isTouchEnd || index === latestState.highlightedIndex ) { return @@ -525,7 +526,7 @@ function useSelect(userProps = {}) { return itemProps }, - [latest, elementIds, mouseAndTouchTrackersRef, shouldScrollRef, dispatch], + [latest, elementIds, mouseAndTouchTrackers, shouldScrollRef, dispatch], ) return { diff --git a/src/hooks/utils.js b/src/hooks/utils.js index 5e4fd198..04cb6378 100644 --- a/src/hooks/utils.js +++ b/src/hooks/utils.js @@ -74,9 +74,7 @@ function stateReducer(s, a) { function getA11ySelectionMessage(selectionParameters) { const {selectedItem, itemToString} = selectionParameters - return selectedItem - ? `${itemToString(selectedItem)} has been selected.` - : '' + return selectedItem ? `${itemToString(selectedItem)} has been selected.` : '' } /** @@ -351,20 +349,17 @@ function getHighlightedIndexOnOpen(props, state, offset) { } return offset < 0 ? items.length - 1 : 0 } - /** - * Reuse the movement tracking of mouse and touch events. + * Tracks mouse and touch events, such as mouseDown, touchMove and touchEnd. * - * @param {boolean} isOpen Whether the dropdown is open or not. - * @param {Array} downshiftElementRefs Downshift element refs to track movement (toggleButton, menu etc.) - * @param {Object} environment Environment where component/hook exists. - * @param {Function} handleBlur Handler on blur from mouse or touch. - * @returns {Object} Ref containing whether mouseDown or touchMove event is happening + * @param {Object} environment The environment to add the event listeners to, for instance window. + * @param {Array} downshiftElementRefs The refs for the element that should not trigger a blur action from mouseDown or touchEnd. + * @param {Function} handleBlur The function that is called if mouseDown or touchEnd occured outside the downshiftElements. + * @returns {Object} The mouse and touch events information, if any of are happening. */ function useMouseAndTouchTracker( - isOpen, - downshiftElementRefs, environment, + downshiftElementRefs, handleBlur, ) { const mouseAndTouchTrackersRef = useRef({ @@ -375,45 +370,39 @@ function useMouseAndTouchTracker( useEffect(() => { if (isReactNative || !environment) { - return + return noop } - // The same strategy for checking if a click occurred inside or outside downshift - // as in downshift.js. - const onMouseDown = () => { + const downshiftElements = downshiftElementRefs.map(ref => ref.current) + + function onMouseDown() { mouseAndTouchTrackersRef.current.isTouchEnd = false // reset this one. mouseAndTouchTrackersRef.current.isMouseDown = true } - const onMouseUp = event => { + function onMouseUp(event) { mouseAndTouchTrackersRef.current.isMouseDown = false if ( - isOpen && - !targetWithinDownshift( - event.target, - downshiftElementRefs.map(ref => ref.current), - environment, - ) + !targetWithinDownshift(event.target, downshiftElements, environment) ) { handleBlur() } } - const onTouchStart = () => { + function onTouchStart() { mouseAndTouchTrackersRef.current.isTouchEnd = false mouseAndTouchTrackersRef.current.isTouchMove = false } - const onTouchMove = () => { + function onTouchMove() { mouseAndTouchTrackersRef.current.isTouchMove = true } - const onTouchEnd = event => { + function onTouchEnd(event) { mouseAndTouchTrackersRef.current.isTouchEnd = true if ( - isOpen && !mouseAndTouchTrackersRef.current.isTouchMove && !targetWithinDownshift( event.target, - downshiftElementRefs.map(ref => ref.current), + downshiftElements, environment, false, ) @@ -428,7 +417,6 @@ function useMouseAndTouchTracker( environment.addEventListener('touchmove', onTouchMove) environment.addEventListener('touchend', onTouchEnd) - // eslint-disable-next-line consistent-return return function cleanup() { environment.removeEventListener('mousedown', onMouseDown) environment.removeEventListener('mouseup', onMouseUp) @@ -436,10 +424,10 @@ function useMouseAndTouchTracker( environment.removeEventListener('touchmove', onTouchMove) environment.removeEventListener('touchend', onTouchEnd) } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [isOpen, environment]) + // eslint-disable-next-line react-hooks/exhaustive-deps -- refs don't change + }, [environment, handleBlur]) - return mouseAndTouchTrackersRef + return mouseAndTouchTrackersRef.current } /* istanbul ignore next */