Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(hooks): remove mouse and touch events on unmount #1581

Merged
merged 2 commits into from Mar 13, 2024
Merged

feat(hooks): remove mouse and touch events on unmount #1581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
adae371
feat(hooks): remove mouse and touch events on unmount
silviuaavram Mar 10, 2024
1c2da99
revert unrelated change
silviuaavram Mar 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/hooks/__tests__/utils.test.js
View file
Open in desktop
Expand Up @@ -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()
})

Expand All @@ -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)
Expand All @@ -123,6 +123,10 @@ describe('utils', () => {
)
expect(environment.removeEventListener).not.toHaveBeenCalled()

rerender()

expect(environment.removeEventListener).not.toHaveBeenCalled()

unmount()

expect(environment.addEventListener).toHaveBeenCalledTimes(5)
Expand All @@ -149,7 +153,9 @@ describe('utils', () => {
)

expect(result.current).toEqual({
current: {isMouseDown: false, isTouchMove: false, isTouchEnd: false},
isMouseDown: false,
isTouchMove: false,
isTouchEnd: false,
})
})
})
Expand Down
39 changes: 21 additions & 18 deletions src/hooks/useCombobox/index.js
View file
Open in desktop
Expand Up @@ -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',
Expand Down Expand Up @@ -309,7 +312,7 @@ function useCombobox(userProps = {}) {

const itemHandleMouseMove = () => {
if (
mouseAndTouchTrackersRef.current.isTouchEnd ||
mouseAndTouchTrackers.isTouchEnd ||
index === latestState.highlightedIndex
) {
return
Expand Down Expand Up @@ -353,7 +356,7 @@ function useCombobox(userProps = {}) {
}
},

[dispatch, elementIds, latest, mouseAndTouchTrackersRef, shouldScrollRef],
[dispatch, elementIds, latest, mouseAndTouchTrackers, shouldScrollRef],
)

const getToggleButtonProps = useCallback(
Expand Down Expand Up @@ -425,7 +428,7 @@ function useCombobox(userProps = {}) {
if (
environment?.document &&
latestState.isOpen &&
!mouseAndTouchTrackersRef.current.isMouseDown
!mouseAndTouchTrackers.isMouseDown
) {
const isBlurByTabChange =
event.relatedTarget === null &&
Expand Down Expand Up @@ -501,13 +504,13 @@ function useCombobox(userProps = {}) {
}
},
[
setGetterPropCallInfo,
latest,
elementIds,
inputKeyDownHandlers,
dispatch,
mouseAndTouchTrackersRef,
elementIds,
environment,
inputKeyDownHandlers,
latest,
mouseAndTouchTrackers,
setGetterPropCallInfo,
],
)

Expand Down
37 changes: 19 additions & 18 deletions src/hooks/useSelect/index.js
View file
Open in desktop
Expand Up @@ -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',
Expand Down Expand Up @@ -365,10 +369,7 @@ function useSelect(userProps = {}) {
})
}
const toggleButtonHandleBlur = () => {
if (
latestState.isOpen &&
!mouseAndTouchTrackersRef.current.isMouseDown
) {
if (latestState.isOpen && !mouseAndTouchTrackers.isMouseDown) {
dispatch({
type: stateChangeTypes.ToggleButtonBlur,
})
Expand Down Expand Up @@ -434,11 +435,11 @@ function useSelect(userProps = {}) {
return toggleProps
},
[
latest,
dispatch,
elementIds,
latest,
mouseAndTouchTrackers,
setGetterPropCallInfo,
dispatch,
mouseAndTouchTrackersRef,
toggleButtonKeyDownHandlers,
],
)
Expand Down Expand Up @@ -472,7 +473,7 @@ function useSelect(userProps = {}) {

const itemHandleMouseMove = () => {
if (
mouseAndTouchTrackersRef.current.isTouchEnd ||
mouseAndTouchTrackers.isTouchEnd ||
index === latestState.highlightedIndex
) {
return
Expand Down Expand Up @@ -525,7 +526,7 @@ function useSelect(userProps = {}) {

return itemProps
},
[latest, elementIds, mouseAndTouchTrackersRef, shouldScrollRef, dispatch],
[latest, elementIds, mouseAndTouchTrackers, shouldScrollRef, dispatch],
)

return {
Expand Down
52 changes: 20 additions & 32 deletions src/hooks/utils.js
View file
Open in desktop
Expand Up @@ -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.` : ''
}

/**
Expand Down Expand Up @@ -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<Object>} 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<HTMLElement>} 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({
Expand All @@ -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,
)
Expand All @@ -428,18 +417,17 @@ 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)
environment.removeEventListener('touchstart', onTouchStart)
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])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change broke the blur behaviour. Now using @testing-library/user-event, when we clears the input (and don't focus out) it triggers the blur and unselect the selected item:

await user.clear(input)

In my implementation, I am using the stateReducer to unselected the selected value of a combobox on blur if the input value is empty:

case useCombobox.stateChangeTypes.InputBlur:
        if (allowCustomValue) return changes

        if (state.inputValue === '') {
          setSelectedItem(null)

          return { ...changes, selectedItem: null }
        }

It's breaking the tests because I no longer have a way to clear the input without triggering the blur.


return mouseAndTouchTrackersRef
return mouseAndTouchTrackersRef.current
}

/* istanbul ignore next */
Expand Down