From 00e05d581e80689dc4d942cf35bb04d6529250e1 Mon Sep 17 00:00:00 2001 From: Rahim Rahman Date: Thu, 5 Sep 2024 11:28:00 -0600 Subject: [PATCH] fix: MM-58164 Search text is behind the textbox while searching (#8026) * fix: MM-58164 search text hides behind textbox * clean up unit test for useCollapsibleHeader * simplify math * unlock doesn't need to use useCallback as it doesn't have any deps * revert changes on unlock * using lockValue & headerOffset directly into children props * disable scrolling to prevent scrollValue from being updated * properly snapping flatlist to the proper spot in a few scenarios --------- Co-authored-by: Mattermost Build --- app/components/navigation_header/header.tsx | 6 +- app/components/navigation_header/index.tsx | 31 +++----- app/hooks/header.ts | 15 ++-- app/hooks/useCollapsibleHeader.test.tsx | 88 +++++++++++++++++++++ app/screens/home/search/search.tsx | 22 ++++-- 5 files changed, 126 insertions(+), 36 deletions(-) create mode 100644 app/hooks/useCollapsibleHeader.test.tsx diff --git a/app/components/navigation_header/header.tsx b/app/components/navigation_header/header.tsx index 66099285b..1c8d4b572 100644 --- a/app/components/navigation_header/header.tsx +++ b/app/components/navigation_header/header.tsx @@ -32,7 +32,6 @@ type Props = { onTitlePress?: () => void; rightButtons?: HeaderRightButton[]; scrollValue?: Animated.SharedValue; - lockValue?: Animated.SharedValue; showBackButton?: boolean; subtitle?: string; subtitleCompanion?: React.ReactElement; @@ -135,7 +134,6 @@ const Header = ({ onTitlePress, rightButtons, scrollValue, - lockValue, showBackButton = true, subtitle, subtitleCompanion, @@ -155,7 +153,7 @@ const Header = ({ } const barHeight = heightOffset - ViewConstants.LARGE_HEADER_TITLE_HEIGHT; - const val = (scrollValue?.value ?? 0); + const val = (scrollValue?.value || 0); const showDuration = 200; const hideDuration = 50; const duration = val >= barHeight ? showDuration : hideDuration; @@ -168,7 +166,7 @@ const Header = ({ const containerAnimatedStyle = useAnimatedStyle(() => ({ height: defaultHeight, paddingTop: insets.top, - }), [defaultHeight, lockValue]); + }), [defaultHeight]); const containerStyle = useMemo(() => ( [styles.container, containerAnimatedStyle]), [styles, containerAnimatedStyle]); diff --git a/app/components/navigation_header/index.tsx b/app/components/navigation_header/index.tsx index 7c474e1a1..dae9d1c28 100644 --- a/app/components/navigation_header/index.tsx +++ b/app/components/navigation_header/index.tsx @@ -24,7 +24,7 @@ type Props = SearchProps & { onTitlePress?: () => void; rightButtons?: HeaderRightButton[]; scrollValue?: Animated.SharedValue; - lockValue?: Animated.SharedValue; + lockValue?: number; hideHeader?: () => void; showBackButton?: boolean; subtitle?: string; @@ -61,32 +61,28 @@ const NavigationHeader = forwardRef(({ const styles = getStyleSheet(theme); const {largeHeight, defaultHeight, headerOffset} = useHeaderHeight(); + + const minScrollValue = useDerivedValue(() => scrollValue?.value || 0, [scrollValue]); + const containerHeight = useAnimatedStyle(() => { - const value = -(scrollValue?.value || 0); - const calculatedHeight = (isLargeTitle ? largeHeight : defaultHeight) + value; - const height = lockValue?.value ? lockValue.value : calculatedHeight; + const calculatedHeight = (isLargeTitle ? largeHeight : defaultHeight) - minScrollValue.value; + const height = lockValue || calculatedHeight; return { height: Math.max(height, defaultHeight), minHeight: defaultHeight, maxHeight: largeHeight + MAX_OVERSCROLL, }; - }); - - const minScrollValue = useDerivedValue(() => scrollValue?.value || 0, [scrollValue]); + }, [defaultHeight, largeHeight, lockValue, isLargeTitle]); const translateY = useDerivedValue(() => ( - lockValue?.value ? -lockValue.value : Math.min(-minScrollValue.value, headerOffset) - ), [lockValue, minScrollValue, headerOffset]); + lockValue ? -lockValue : Math.min(-minScrollValue.value, headerOffset) + ), [lockValue, headerOffset]); const searchTopStyle = useAnimatedStyle(() => { const margin = clamp(-minScrollValue.value, -headerOffset, headerOffset); - const marginTop = (lockValue?.value ? -lockValue?.value : margin) - SEARCH_INPUT_HEIGHT - SEARCH_INPUT_MARGIN; + const marginTop = (lockValue ? -lockValue : margin) - SEARCH_INPUT_HEIGHT - SEARCH_INPUT_MARGIN; return {marginTop}; - }, [lockValue, headerOffset, scrollValue]); - - const heightOffset = useDerivedValue(() => ( - lockValue?.value ? lockValue.value : headerOffset - ), [lockValue, headerOffset]); + }, [lockValue, headerOffset]); return ( @@ -94,12 +90,11 @@ const NavigationHeader = forwardRef(({ defaultHeight={defaultHeight} hasSearch={hasSearch} isLargeTitle={isLargeTitle} - heightOffset={heightOffset.value} + heightOffset={lockValue || headerOffset} leftComponent={leftComponent} onBackPress={onBackPress} onTitlePress={onTitlePress} rightButtons={rightButtons} - lockValue={lockValue} scrollValue={scrollValue} showBackButton={showBackButton} subtitle={subtitle} @@ -109,7 +104,7 @@ const NavigationHeader = forwardRef(({ /> {isLargeTitle && (isLargeTitle: boolean, onSnap?: (offset: const animatedRef = useAnimatedRef(); const {largeHeight, defaultHeight, headerOffset} = useHeaderHeight(); const scrollValue = useSharedValue(0); - const lockValue = useSharedValue(null); + const [lockValue, setLockValue] = useState(0); const autoScroll = useSharedValue(false); const snapping = useSharedValue(false); const scrollEnabled = useSharedValue(true); const headerHeight = useDerivedValue(() => { - const value = -(scrollValue?.value || 0); + const value = -(scrollValue.value); const heightWithScroll = (isLargeTitle ? largeHeight : defaultHeight) + value; let height = Math.max(heightWithScroll, defaultHeight); if (value > insets.top) { @@ -134,11 +134,13 @@ export const useCollapsibleHeader = (isLargeTitle: boolean, onSnap?: (offset: const hideHeader = useCallback((lock = false) => { if (lock) { - lockValue.value = defaultHeight; + // by disabling scroll, this prevent the scrollValue from being updated needlessly as observed in iOS simulators + scrollEnabled.value = false; + setLockValue(defaultHeight); } const offset = headerOffset; - if (animatedRef?.current && Math.abs((scrollValue?.value || 0)) <= insets.top) { + if (animatedRef?.current && Math.abs((scrollValue.value)) <= insets.top) { autoScroll.value = true; if ('scrollTo' in animatedRef.current) { animatedRef.current.scrollTo({y: offset, animated: true}); @@ -154,7 +156,8 @@ export const useCollapsibleHeader = (isLargeTitle: boolean, onSnap?: (offset: }, [largeHeight, defaultHeight]); const unlock = useCallback(() => { - lockValue.value = null; + scrollEnabled.value = true; + setLockValue(0); }, []); return { diff --git a/app/hooks/useCollapsibleHeader.test.tsx b/app/hooks/useCollapsibleHeader.test.tsx new file mode 100644 index 000000000..83a93c605 --- /dev/null +++ b/app/hooks/useCollapsibleHeader.test.tsx @@ -0,0 +1,88 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. +import {renderHook} from '@testing-library/react-hooks'; +import {act} from '@testing-library/react-native'; + +import ViewConstants from '@constants/view'; + +import * as DeviceFunctions from './device'; +import {useCollapsibleHeader} from './header'; + +const LARGE_HEADER_TITLE_HEIGHT = 128; +const HEADER_OFFSET = LARGE_HEADER_TITLE_HEIGHT - ViewConstants.DEFAULT_HEADER_HEIGHT; + +describe('useCollapsibleHeader', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + const commonHookResponse = { + largeHeight: LARGE_HEADER_TITLE_HEIGHT, + scrollRef: {current: null}, + scrollValue: {value: 0}, + onScroll: expect.any(Function), + hideHeader: expect.any(Function), + lockValue: 0, + unlock: expect.any(Function), + headerOffset: HEADER_OFFSET, + scrollEnabled: {value: true}, + setAutoScroll: expect.any(Function), + }; + + it('should return the correct values with isLargeTitle is true', () => { + const {result} = renderHook(() => useCollapsibleHeader(true)); + + expect(result.current).toEqual({ + defaultHeight: ViewConstants.DEFAULT_HEADER_HEIGHT, + scrollPaddingTop: LARGE_HEADER_TITLE_HEIGHT, + headerHeight: {value: LARGE_HEADER_TITLE_HEIGHT}, + ...commonHookResponse, + }); + }); + + it('should return the correct values with isLargeTitle is false', () => { + const {result} = renderHook(() => useCollapsibleHeader(false)); + + expect(result.current).toEqual({ + defaultHeight: ViewConstants.DEFAULT_HEADER_HEIGHT, + scrollPaddingTop: ViewConstants.DEFAULT_HEADER_HEIGHT, + headerHeight: {value: ViewConstants.DEFAULT_HEADER_HEIGHT}, + ...commonHookResponse, + }); + }); + + it('should return the correct values with isLargeTitle is true, and on a tablet', () => { + jest.spyOn(DeviceFunctions, 'useIsTablet').mockReturnValue(true); + + const {result} = renderHook(() => useCollapsibleHeader(true)); + + expect(result.current).toEqual({ + defaultHeight: ViewConstants.TABLET_HEADER_HEIGHT, + scrollPaddingTop: LARGE_HEADER_TITLE_HEIGHT, + headerHeight: {value: LARGE_HEADER_TITLE_HEIGHT}, + ...commonHookResponse, + }); + }); + + it('should change the lock value when hideHeader is called', () => { + const {result} = renderHook(() => useCollapsibleHeader(true)); + + expect(result.current.lockValue).toBe(0); + + act(() => result.current.hideHeader(true)); + + expect(result.current.lockValue).toBe(ViewConstants.DEFAULT_HEADER_HEIGHT); + }); + + it('should reset the lockValue when unlock is called', () => { + const {result} = renderHook(() => useCollapsibleHeader(true)); + + act(() => result.current.hideHeader(true)); + + expect(result.current.lockValue).toBe(ViewConstants.DEFAULT_HEADER_HEIGHT); + + act(() => result.current.unlock()); + + expect(result.current.lockValue).toBe(0); + }); +}); diff --git a/app/screens/home/search/search.tsx b/app/screens/home/search/search.tsx index 48c6463b3..45386ab6b 100644 --- a/app/screens/home/search/search.tsx +++ b/app/screens/home/search/search.tsx @@ -126,9 +126,13 @@ const SearchScreen = ({teamId, teams}: Props) => { scrollRef.current?.scrollToOffset({offset, animated}); }; + const onSnapWithTimeout = useCallback((offset: number, animated = true) => { + // wait until the keyboard is completely dismissed before scrolling to where the header should be + setTimeout(() => onSnap(offset, animated), 100); + }, [onSnap]); + const { headerHeight, - headerOffset, hideHeader, lockValue, onScroll, @@ -156,7 +160,6 @@ const SearchScreen = ({teamId, teams}: Props) => { const handleCancelSearch = useCallback(() => { cancelRef.current = true; resetToInitial(); - onSnap(0); }, [resetToInitial]); const handleTextChange = useCallback((newValue: string) => { @@ -204,7 +207,10 @@ const SearchScreen = ({teamId, teams}: Props) => { const onBlur = useCallback(() => { setSearchIsFocused(false); - }, []); + if (!cancelRef.current && !clearRef.current) { + onSnapWithTimeout(0); + } + }, [onSnapWithTimeout]); const onFocus = useCallback(() => { setSearchIsFocused(true); @@ -281,7 +287,7 @@ const SearchScreen = ({teamId, teams}: Props) => { }, [isFocused, stateIndex]); const headerTopStyle = useAnimatedStyle(() => ({ - top: lockValue.value ? lockValue.value : headerHeight.value, + top: lockValue || headerHeight.value, zIndex: lastSearchedValue ? 10 : 0, }), [headerHeight, lastSearchedValue, lockValue]); @@ -304,15 +310,15 @@ const SearchScreen = ({teamId, teams}: Props) => { const onFlatLayout = useCallback(() => { if (clearRef.current || cancelRef.current) { unlock(); + onSnapWithTimeout(0); } + if (clearRef.current) { - onSnap(headerOffset, false); clearRef.current = false; } else if (cancelRef.current) { - onSnap(0); cancelRef.current = false; } - }, [headerOffset, scrollRef]); + }, [unlock, onSnapWithTimeout]); useDidUpdate(() => { if (isFocused) { @@ -407,7 +413,7 @@ const SearchScreen = ({teamId, teams}: Props) => { posts={posts} matches={matches} fileInfos={fileInfos} - scrollPaddingTop={lockValue.value} + scrollPaddingTop={lockValue} fileChannelIds={fileChannelIds} /> }