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.

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

Fix - Web - Blue selection border on plan option after a refresh #54682

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import useSingleExecution from '@hooks/useSingleExecution';
import useStyledSafeAreaInsets from '@hooks/useStyledSafeAreaInsets';
import useThemeStyles from '@hooks/useThemeStyles';
import getSectionsWithIndexOffset from '@libs/getSectionsWithIndexOffset';
import {addKeyDownPressListener, removeKeyDownPressListener} from '@libs/KeyboardShortcut/KeyDownPressListener';
import Log from '@libs/Log';
import variables from '@styles/variables';
import CONST from '@src/CONST';
Expand Down Expand Up @@ -129,6 +130,7 @@ function BaseSelectionList<TItem extends ListItem>(
const {translate} = useLocalize();
const listRef = useRef<RNSectionList<TItem, SectionWithIndexOffset<TItem>>>(null);
const innerTextInputRef = useRef<RNTextInput | null>(null);
const hasKeyBeenPressed = useRef(false);
const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null);
const shouldShowSelectAll = !!onSelectAll;
const activeElementRole = useActiveElementRole();
Expand Down Expand Up @@ -318,6 +320,16 @@ function BaseSelectionList<TItem extends ListItem>(

const debouncedScrollToIndex = useMemo(() => lodashDebounce(scrollToIndex, CONST.TIMING.LIST_SCROLLING_DEBOUNCE_TIME, {leading: true, trailing: true}), [scrollToIndex]);

const setHasKeyBeenPressed = useCallback(() => {
if (hasKeyBeenPressed.current) {
return;
}

// We need to track whether a key has been pressed to enable focus syncing only if a key has been pressed.
// This is to avoid the default behavior of web showing blue border on click of items after a page refresh.
hasKeyBeenPressed.current = true;
}, []);

// If `initiallyFocusedOptionKey` is not passed, we fall back to `-1`, to avoid showing the highlight on the first member
const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({
initialFocusedIndex: flattenedSections.allOptions.findIndex((option) => option.keyForList === initiallyFocusedOptionKey),
Expand All @@ -333,9 +345,16 @@ function BaseSelectionList<TItem extends ListItem>(
(shouldDebounceScrolling ? debouncedScrollToIndex : scrollToIndex)(index, true);
}
},
...(!hasKeyBeenPressed.current && {setHasKeyBeenPressed}),
isFocused,
});

useEffect(() => {
addKeyDownPressListener(setHasKeyBeenPressed);

return () => removeKeyDownPressListener(setHasKeyBeenPressed);
}, [setHasKeyBeenPressed]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Same, some comments might help for whoever comes here in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cristipaval I added a comment here

// We need to track whether a key has been pressed to enable focus syncing only if a key has been pressed.
// This is to avoid the default behavior of web showing blue border on click of items after a page refresh.
hasKeyBeenPressed.current = true;

Isn't that enough? Because here the name of the ref already gives a hint why it is being set on key down event. WDYT

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I think that should be enough to know the reason for this code. Thanks

const selectedItemIndex = useMemo(
() => (initiallyFocusedOptionKey ? flattenedSections.allOptions.findIndex((option) => option.isSelected) : -1),
[flattenedSections.allOptions, initiallyFocusedOptionKey],
Expand Down Expand Up @@ -549,7 +568,7 @@ function BaseSelectionList<TItem extends ListItem>(
shouldIgnoreFocus={shouldIgnoreFocus}
setFocusedIndex={setFocusedIndex}
normalizedIndex={normalizedIndex}
shouldSyncFocus={!isTextInputFocusedRef.current}
shouldSyncFocus={!isTextInputFocusedRef.current && hasKeyBeenPressed.current}
wrapperStyle={listItemWrapperStyle}
titleStyles={listItemTitleStyles}
shouldHighlightSelectedItem={shouldHighlightSelectedItem}
Expand Down
9 changes: 6 additions & 3 deletions src/hooks/useArrowKeyFocusManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type Config = {
allowHorizontalArrowKeys?: boolean;
allowNegativeIndexes?: boolean;
isFocused?: boolean;
setHasKeyBeenPressed?: () => void;
};

type UseArrowKeyFocusManager = [number, (index: number) => void];
Expand Down Expand Up @@ -50,6 +51,7 @@ export default function useArrowKeyFocusManager({
allowHorizontalArrowKeys = false,
allowNegativeIndexes = false,
isFocused = true,
setHasKeyBeenPressed,
}: Config): UseArrowKeyFocusManager {
const [focusedIndex, setFocusedIndex] = useState(initialFocusedIndex);
const prevIsFocusedIndex = usePrevious(focusedIndex);
Expand Down Expand Up @@ -82,7 +84,7 @@ export default function useArrowKeyFocusManager({
return;
}
const nextIndex = disableCyclicTraversal ? -1 : maxIndex;

setHasKeyBeenPressed?.();
setFocusedIndex((actualIndex) => {
const currentFocusedIndex = actualIndex > 0 ? actualIndex - (itemsPerRow ?? 1) : nextIndex;
let newFocusedIndex = currentFocusedIndex;
Expand All @@ -105,14 +107,15 @@ export default function useArrowKeyFocusManager({
}
return newFocusedIndex;
});
}, [maxIndex, isFocused, disableCyclicTraversal, itemsPerRow, disabledIndexes, allowNegativeIndexes]);
}, [maxIndex, isFocused, disableCyclicTraversal, itemsPerRow, disabledIndexes, allowNegativeIndexes, setHasKeyBeenPressed]);

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_UP, arrowUpCallback, arrowConfig);

const arrowDownCallback = useCallback(() => {
if (maxIndex < 0 || !isFocused) {
return;
}
setHasKeyBeenPressed?.();

const nextIndex = disableCyclicTraversal ? maxIndex : 0;

Expand Down Expand Up @@ -150,7 +153,7 @@ export default function useArrowKeyFocusManager({
}
return newFocusedIndex;
});
}, [disableCyclicTraversal, disabledIndexes, isFocused, itemsPerRow, maxIndex]);
}, [disableCyclicTraversal, disabledIndexes, isFocused, itemsPerRow, maxIndex, setHasKeyBeenPressed]);

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN, arrowDownCallback, arrowConfig);

Expand Down
29 changes: 2 additions & 27 deletions src/hooks/useSyncFocus/index.ts
Original file line number Diff line number Diff line change
@@ -1,28 +1,3 @@
import {useContext, useLayoutEffect} from 'react';
import type {RefObject} from 'react';
import type {View} from 'react-native';
import {ScreenWrapperStatusContext} from '@components/ScreenWrapper';
import useSyncFocusImplementation from './useSyncFocusImplementation';

/**
* Custom React hook created to handle sync of focus on an element when the user navigates through the app with keyboard.
* When the user navigates through the app using the arrows and then the tab button, the focus on the element and the native focus of the browser differs.
* To maintain consistency when an element is focused in the app, the focus() method is additionally called on the focused element to eliminate the difference between native browser focus and application focus.
*/
const useSyncFocus = (ref: RefObject<View | HTMLElement>, isFocused: boolean, shouldSyncFocus = true) => {
// this hook can be used outside ScreenWrapperStatusContext (eg. in Popovers). So we to check if the context is present.
// If we are outside context we don't have to look at transition status
const contextValue = useContext(ScreenWrapperStatusContext);

const didScreenTransitionEnd = contextValue ? contextValue.didScreenTransitionEnd : true;

useLayoutEffect(() => {
if (!isFocused || !shouldSyncFocus || !didScreenTransitionEnd) {
return;
}

ref.current?.focus({preventScroll: true});
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [didScreenTransitionEnd, isFocused, ref]);
};

export default useSyncFocus;
export default useSyncFocusImplementation;
28 changes: 28 additions & 0 deletions src/hooks/useSyncFocus/useSyncFocusImplementation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import {useContext, useLayoutEffect} from 'react';
import type {RefObject} from 'react';
import type {View} from 'react-native';
import {ScreenWrapperStatusContext} from '@components/ScreenWrapper';

/**
* Custom React hook created to handle sync of focus on an element when the user navigates through the app with keyboard.
* When the user navigates through the app using the arrows and then the tab button, the focus on the element and the native focus of the browser differs.
* To maintain consistency when an element is focused in the app, the focus() method is additionally called on the focused element to eliminate the difference between native browser focus and application focus.
*/
const useSyncFocusImplementation = (ref: RefObject<View | HTMLElement>, isFocused: boolean, shouldSyncFocus = true) => {
// this hook can be used outside ScreenWrapperStatusContext (eg. in Popovers). So we to check if the context is present.
// If we are outside context we don't have to look at transition status
const contextValue = useContext(ScreenWrapperStatusContext);

const didScreenTransitionEnd = contextValue ? contextValue.didScreenTransitionEnd : true;

useLayoutEffect(() => {
if (!isFocused || !shouldSyncFocus || !didScreenTransitionEnd) {
return;
}

ref.current?.focus({preventScroll: true});
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [didScreenTransitionEnd, isFocused, ref]);
};

export default useSyncFocusImplementation;
39 changes: 39 additions & 0 deletions tests/unit/useSyncFocusTest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import {renderHook} from '@testing-library/react-native';
import type {RefObject} from 'react';
import type {View} from 'react-native';
import useSyncFocus from '@hooks/useSyncFocus/useSyncFocusImplementation';

describe('useSyncFocus', () => {
it('useSyncFocus should only focus if shouldSyncFocus is true', () => {
const refMock = {current: {focus: jest.fn()}};

// When useSyncFocus is rendered initially while shouldSyncFocus is false.
const {rerender} = renderHook(
({
ref = refMock,
isFocused = true,
shouldSyncFocus = false,
}: {
isFocused?: boolean;
shouldSyncFocus?: boolean;
ref?: {
current: {
focus: jest.Mock;
};
};
}) => useSyncFocus(ref as unknown as RefObject<View>, isFocused, shouldSyncFocus),
{initialProps: {}},
);
// Then the ref focus will not be called.
expect(refMock.current.focus).not.toHaveBeenCalled();

rerender({isFocused: false});
expect(refMock.current.focus).not.toHaveBeenCalled();

// When shouldSyncFocus and isFocused are true
rerender({isFocused: true, shouldSyncFocus: true});

// Then the ref focus will be called.
expect(refMock.current.focus).toHaveBeenCalled();
});
});