From 118212b3786fd509ed1e4b542e21011abe672b93 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Thu, 2 Jan 2025 11:19:56 +0200 Subject: [PATCH 1/3] improve ActiveHoverable CPU performance --- src/components/Hoverable/ActiveHoverable.tsx | 172 ++++++++----------- 1 file changed, 76 insertions(+), 96 deletions(-) diff --git a/src/components/Hoverable/ActiveHoverable.tsx b/src/components/Hoverable/ActiveHoverable.tsx index eda42a703d65..5015f6e55beb 100644 --- a/src/components/Hoverable/ActiveHoverable.tsx +++ b/src/components/Hoverable/ActiveHoverable.tsx @@ -15,142 +15,122 @@ type OnMouseEvents = Record void>; function ActiveHoverable({onHoverIn, onHoverOut, shouldHandleScroll, shouldFreezeCapture, children}: ActiveHoverableProps, outerRef: Ref) { const [isHovered, setIsHovered] = useState(false); - const elementRef = useRef(null); const isScrollingRef = useRef(false); - const isHoveredRef = useRef(false); - const isVisibiltyHidden = useRef(false); - - const updateIsHovered = useCallback( - (hovered: boolean) => { - isHoveredRef.current = hovered; - // Nullish coalescing operator (`??`) wouldn't be appropriate here because - // it's not a matter of providing a default when encountering `null` or `undefined` - // but rather making a decision based on the truthy nature of the complete expressions. - // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - if ((shouldHandleScroll && isScrollingRef.current) || shouldFreezeCapture) { + const lastKnownHoverState = useRef(false); + + const updateHoverState = useCallback( + (value: boolean) => { + if (lastKnownHoverState.current === value) { return; } - setIsHovered(hovered); + lastKnownHoverState.current = value; + setIsHovered(value); + if (value) { + onHoverIn?.(); + } else { + onHoverOut?.(); + } }, - [shouldHandleScroll, shouldFreezeCapture], + [onHoverIn, onHoverOut], ); - useEffect(() => { - if (isHovered) { - onHoverIn?.(); - } else { - onHoverOut?.(); - } - }, [isHovered, onHoverIn, onHoverOut]); - useEffect(() => { if (!shouldHandleScroll) { return; } const scrollingListener = DeviceEventEmitter.addListener(CONST.EVENTS.SCROLLING, (scrolling: boolean) => { + const wasScrolling = isScrollingRef.current; isScrollingRef.current = scrolling; - if (!isScrollingRef.current) { - setIsHovered(isHoveredRef.current); + + if (scrolling) { + // Store current hover state before clearing it + if (isHovered) { + lastKnownHoverState.current = true; + } + updateHoverState(false); + } else if (wasScrolling) { + // Only check hover state when scroll actually ends + const isMouseOver = elementRef.current?.matches(':hover'); + if (isMouseOver && !shouldFreezeCapture && lastKnownHoverState.current) { + updateHoverState(true); + } } }); return () => scrollingListener.remove(); - }, [shouldHandleScroll]); + }, [shouldHandleScroll, shouldFreezeCapture, updateHoverState, isHovered]); + // Handle visibility changes useEffect(() => { - // Do not mount a listener if the component is not hovered - if (!isHovered) { - return; - } - - /** - * Checks the hover state of a component and updates it based on the event target. - * This is necessary to handle cases where the hover state might get stuck due to an unreliable mouseleave trigger, - * such as when an element is removed before the mouseleave event is triggered. - * @param event The hover event object. - */ - const unsetHoveredIfOutside = (event: MouseEvent) => { - // We're also returning early if shouldFreezeCapture is true in order - // to not update the hover state but keep it frozen. - if (!elementRef.current || elementRef.current.contains(event.target as Node) || shouldFreezeCapture) { - return; + const handleVisibilityChange = () => { + if (document.visibilityState === 'hidden') { + if (isHovered) { + lastKnownHoverState.current = true; + } + updateHoverState(false); + } else if (lastKnownHoverState.current) { + // When becoming visible, check if we should restore hover + const isMouseOver = elementRef.current?.matches(':hover'); + if (isMouseOver && !shouldFreezeCapture) { + requestAnimationFrame(() => { + updateHoverState(true); + }); + } } - - setIsHovered(false); }; - document.addEventListener('mouseover', unsetHoveredIfOutside, true); - - return () => document.removeEventListener('mouseover', unsetHoveredIfOutside); - }, [isHovered, elementRef, shouldFreezeCapture]); + document.addEventListener('visibilitychange', handleVisibilityChange); + return () => document.removeEventListener('visibilitychange', handleVisibilityChange); + }, [updateHoverState, isHovered, shouldFreezeCapture]); - useEffect(() => { - const unsetHoveredWhenDocumentIsHidden = () => { - if (document.visibilityState !== 'hidden') { + const handleMouseEvents = useCallback( + (type: 'enter' | 'leave' | 'blur' | 'move') => (e: MouseEvent) => { + if (shouldFreezeCapture ?? (shouldHandleScroll && isScrollingRef.current)) { return; } - isVisibiltyHidden.current = true; - setIsHovered(false); - }; + if (type === 'blur') { + if (!elementRef.current?.contains(e.relatedTarget as Node)) { + updateHoverState(false); + } + return; + } - document.addEventListener('visibilitychange', unsetHoveredWhenDocumentIsHidden); + // Handle quick mouse movements with move event + if (type === 'move' && !isHovered && lastKnownHoverState.current) { + updateHoverState(true); + return; + } - return () => document.removeEventListener('visibilitychange', unsetHoveredWhenDocumentIsHidden); - }, []); + updateHoverState(type === 'enter'); + }, + [shouldFreezeCapture, shouldHandleScroll, updateHoverState, isHovered], + ); const child = useMemo(() => getReturnValue(children, !isScrollingRef.current && isHovered), [children, isHovered]); const {onMouseEnter, onMouseLeave, onMouseMove, onBlur} = child.props as OnMouseEvents; - const hoverAndForwardOnMouseEnter = useCallback( - (e: MouseEvent) => { - isVisibiltyHidden.current = false; - updateIsHovered(true); + return cloneElement(child, { + ref: mergeRefs(elementRef, outerRef, child.ref), + onMouseEnter: (e: MouseEvent) => { + handleMouseEvents('enter')(e); onMouseEnter?.(e); }, - [updateIsHovered, onMouseEnter], - ); - - const unhoverAndForwardOnMouseLeave = useCallback( - (e: MouseEvent) => { - updateIsHovered(false); + onMouseLeave: (e: MouseEvent) => { + handleMouseEvents('leave')(e); onMouseLeave?.(e); }, - [updateIsHovered, onMouseLeave], - ); - - const unhoverAndForwardOnBlur = useCallback( - (event: MouseEvent) => { - // Check if the blur event occurred due to clicking outside the element - // and the wrapperView contains the element that caused the blur and reset isHovered - if (!elementRef.current?.contains(event.target as Node) && !elementRef.current?.contains(event.relatedTarget as Node) && !shouldFreezeCapture) { - setIsHovered(false); - } - - onBlur?.(event); - }, - [onBlur, shouldFreezeCapture], - ); - - const handleAndForwardOnMouseMove = useCallback( - (e: MouseEvent) => { - isVisibiltyHidden.current = false; - updateIsHovered(true); + onMouseMove: (e: MouseEvent) => { + handleMouseEvents('move')(e); onMouseMove?.(e); }, - [updateIsHovered, onMouseMove], - ); - - return cloneElement(child, { - ref: mergeRefs(elementRef, outerRef, child.ref), - onMouseEnter: hoverAndForwardOnMouseEnter, - onMouseLeave: unhoverAndForwardOnMouseLeave, - onBlur: unhoverAndForwardOnBlur, - ...(isVisibiltyHidden.current ? {onMouseMove: handleAndForwardOnMouseMove} : {}), + onBlur: (e: MouseEvent) => { + handleMouseEvents('blur')(e); + onBlur?.(e); + }, }); } - export default forwardRef(ActiveHoverable); From a8a191c3aa061bc529324e94797bb7a53e19c6c4 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Fri, 10 Jan 2025 17:53:48 +0200 Subject: [PATCH 2/3] fix hover close --- src/components/Hoverable/ActiveHoverable.tsx | 98 ++++++++------------ 1 file changed, 39 insertions(+), 59 deletions(-) diff --git a/src/components/Hoverable/ActiveHoverable.tsx b/src/components/Hoverable/ActiveHoverable.tsx index 5015f6e55beb..72e890745763 100644 --- a/src/components/Hoverable/ActiveHoverable.tsx +++ b/src/components/Hoverable/ActiveHoverable.tsx @@ -17,22 +17,31 @@ function ActiveHoverable({onHoverIn, onHoverOut, shouldHandleScroll, shouldFreez const [isHovered, setIsHovered] = useState(false); const elementRef = useRef(null); const isScrollingRef = useRef(false); - const lastKnownHoverState = useRef(false); + const isHoveredRef = useRef(false); + const isVisibilityHidden = useRef(false); - const updateHoverState = useCallback( - (value: boolean) => { - if (lastKnownHoverState.current === value) { + const updateIsHovered = useCallback( + (hovered: boolean) => { + if (shouldFreezeCapture) { return; } - lastKnownHoverState.current = value; - setIsHovered(value); - if (value) { + + isHoveredRef.current = hovered; + isVisibilityHidden.current = false; + + if (shouldHandleScroll && isScrollingRef.current) { + return; + } + + setIsHovered(hovered); + + if (hovered) { onHoverIn?.(); } else { onHoverOut?.(); } }, - [onHoverIn, onHoverOut], + [shouldHandleScroll, shouldFreezeCapture, onHoverIn, onHoverOut], ); useEffect(() => { @@ -41,77 +50,51 @@ function ActiveHoverable({onHoverIn, onHoverOut, shouldHandleScroll, shouldFreez } const scrollingListener = DeviceEventEmitter.addListener(CONST.EVENTS.SCROLLING, (scrolling: boolean) => { - const wasScrolling = isScrollingRef.current; isScrollingRef.current = scrolling; - - if (scrolling) { - // Store current hover state before clearing it - if (isHovered) { - lastKnownHoverState.current = true; - } - updateHoverState(false); - } else if (wasScrolling) { - // Only check hover state when scroll actually ends - const isMouseOver = elementRef.current?.matches(':hover'); - if (isMouseOver && !shouldFreezeCapture && lastKnownHoverState.current) { - updateHoverState(true); - } + if (scrolling && isHovered) { + setIsHovered(false); + onHoverOut?.(); + } else if (!scrolling && elementRef.current?.matches(':hover')) { + setIsHovered(true); + onHoverIn?.(); } }); return () => scrollingListener.remove(); - }, [shouldHandleScroll, shouldFreezeCapture, updateHoverState, isHovered]); + }, [shouldHandleScroll, isHovered, onHoverIn, onHoverOut]); - // Handle visibility changes useEffect(() => { const handleVisibilityChange = () => { if (document.visibilityState === 'hidden') { - if (isHovered) { - lastKnownHoverState.current = true; - } - updateHoverState(false); - } else if (lastKnownHoverState.current) { - // When becoming visible, check if we should restore hover - const isMouseOver = elementRef.current?.matches(':hover'); - if (isMouseOver && !shouldFreezeCapture) { - requestAnimationFrame(() => { - updateHoverState(true); - }); - } + isVisibilityHidden.current = true; + setIsHovered(false); + } else { + isVisibilityHidden.current = false; } }; document.addEventListener('visibilitychange', handleVisibilityChange); return () => document.removeEventListener('visibilitychange', handleVisibilityChange); - }, [updateHoverState, isHovered, shouldFreezeCapture]); + }, []); const handleMouseEvents = useCallback( - (type: 'enter' | 'leave' | 'blur' | 'move') => (e: MouseEvent) => { - if (shouldFreezeCapture ?? (shouldHandleScroll && isScrollingRef.current)) { - return; - } - - if (type === 'blur') { - if (!elementRef.current?.contains(e.relatedTarget as Node)) { - updateHoverState(false); - } + (type: 'enter' | 'leave' | 'blur') => (e: MouseEvent) => { + if (shouldFreezeCapture) { return; } - // Handle quick mouse movements with move event - if (type === 'move' && !isHovered && lastKnownHoverState.current) { - updateHoverState(true); - return; - } + const newHoverState = type === 'enter'; + isHoveredRef.current = newHoverState; + isVisibilityHidden.current = false; - updateHoverState(type === 'enter'); + updateIsHovered(newHoverState); }, - [shouldFreezeCapture, shouldHandleScroll, updateHoverState, isHovered], + [shouldFreezeCapture, updateIsHovered], ); - const child = useMemo(() => getReturnValue(children, !isScrollingRef.current && isHovered), [children, isHovered]); + const child = useMemo(() => getReturnValue(children, isHovered), [children, isHovered]); - const {onMouseEnter, onMouseLeave, onMouseMove, onBlur} = child.props as OnMouseEvents; + const {onMouseEnter, onMouseLeave, onBlur} = child.props as OnMouseEvents; return cloneElement(child, { ref: mergeRefs(elementRef, outerRef, child.ref), @@ -123,14 +106,11 @@ function ActiveHoverable({onHoverIn, onHoverOut, shouldHandleScroll, shouldFreez handleMouseEvents('leave')(e); onMouseLeave?.(e); }, - onMouseMove: (e: MouseEvent) => { - handleMouseEvents('move')(e); - onMouseMove?.(e); - }, onBlur: (e: MouseEvent) => { handleMouseEvents('blur')(e); onBlur?.(e); }, }); } + export default forwardRef(ActiveHoverable); From a1e453337a81fd2568fe63354171777fa6849027 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Fri, 10 Jan 2025 21:30:48 +0200 Subject: [PATCH 3/3] fix eslint error --- src/components/Hoverable/ActiveHoverable.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/Hoverable/ActiveHoverable.tsx b/src/components/Hoverable/ActiveHoverable.tsx index 72e890745763..3ec7bb8f25d4 100644 --- a/src/components/Hoverable/ActiveHoverable.tsx +++ b/src/components/Hoverable/ActiveHoverable.tsx @@ -78,7 +78,7 @@ function ActiveHoverable({onHoverIn, onHoverOut, shouldHandleScroll, shouldFreez }, []); const handleMouseEvents = useCallback( - (type: 'enter' | 'leave' | 'blur') => (e: MouseEvent) => { + (type: 'enter' | 'leave' | 'blur') => () => { if (shouldFreezeCapture) { return; } @@ -99,15 +99,15 @@ function ActiveHoverable({onHoverIn, onHoverOut, shouldHandleScroll, shouldFreez return cloneElement(child, { ref: mergeRefs(elementRef, outerRef, child.ref), onMouseEnter: (e: MouseEvent) => { - handleMouseEvents('enter')(e); + handleMouseEvents('enter')(); onMouseEnter?.(e); }, onMouseLeave: (e: MouseEvent) => { - handleMouseEvents('leave')(e); + handleMouseEvents('leave')(); onMouseLeave?.(e); }, onBlur: (e: MouseEvent) => { - handleMouseEvents('blur')(e); + handleMouseEvents('blur')(); onBlur?.(e); }, });