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(lists): fix imperative focus not working properly on android #149

Merged
merged 3 commits into from
Sep 12, 2024
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
36 changes: 33 additions & 3 deletions packages/lib/src/spatial-navigation/SpatialNavigator.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Direction, Lrud } from '@bam.tech/lrud';
import { isError } from './helpers/isError';

export type OnDirectionHandledWithoutMovement = (direction: Direction) => void;
type OnDirectionHandledWithoutMovementRef = { current: OnDirectionHandledWithoutMovement };
Expand Down Expand Up @@ -85,6 +86,15 @@ export default class SpatialNavigator {
*/
private focusQueue: string | null = null;

/**
* In the case of virtualized lists, we have some race condition issues when trying
* to imperatively assign focus.
* Indeed, we need the list to scroll to the element and then focus it. But the element
* needs to exist to be focused, so we need first to scroll then wait for the element to render
* then focus it.
*/
private virtualNodeFocusQueue: string | null = null;

/**
* To handle the default focus, we want to queue the element to be focused.
* We queue it because it might not be registered yet when it asks for focus.
Expand All @@ -110,13 +120,19 @@ export default class SpatialNavigator {
*
* Still, I want to queue it, because the element might not be registered yet (example: in the case of virtualized lists)
*/
public deferredFocus = (id: string) => {
setTimeout(() => {
public grabFocusDeferred = (id: string) => {
try {
if (this.lrud.getNode(id)) {
this.lrud.assignFocus(id);
return;
}
}, 0);
} catch (error) {
// If the element exists but is not focusable, it is very likely that it will
// have a focusable child soon. This is the case for imperative focus on virtualized lists.
if (isError(error) && error.message === 'trying to assign focus to a non focusable node') {
this.virtualNodeFocusQueue = id;
}
}
};

/**
Expand All @@ -127,6 +143,7 @@ export default class SpatialNavigator {
* when the element is finally registered.
*/
private handleQueuedFocus = () => {
// Handle focus queue
if (this.focusQueue && this.lrud.getNode(this.focusQueue)) {
try {
this.lrud.assignFocus(this.focusQueue);
Expand All @@ -135,6 +152,19 @@ export default class SpatialNavigator {
// pass
}
}

// Handle virtual nodes (for virtualized lists) focus queue
if (
this.virtualNodeFocusQueue &&
this.lrud.getNode(this.virtualNodeFocusQueue)?.children?.length !== 0
) {
try {
this.lrud.assignFocus(this.virtualNodeFocusQueue);
this.virtualNodeFocusQueue = null;
} catch (e) {
// pass
}
}
};

public grabFocus = (id: string) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { SpatialNavigationVirtualizedList } from './SpatialNavigationVirtualized
import { DefaultFocus } from '../../context/DefaultFocusContext';
import testRemoteControlManager from '../tests/helpers/testRemoteControlManager';
import { setComponentLayoutSize } from '../../../testing/setComponentLayoutSize';
import { useRef } from 'react';
import { useMemo, useRef } from 'react';
import { SpatialNavigationVirtualizedListRef } from '../../types/SpatialNavigationVirtualizedListRef';
import { SpatialNavigationNode } from '../Node';

Expand All @@ -26,10 +26,13 @@ describe('SpatialNavigationVirtualizedList', () => {
<TestButton title={`button ${index + 1}`} onSelect={item.onSelect} />
);

const data = Array.from({ length: 10 }, (_, index) => ({
onSelect: () => undefined,
currentItemSize: index % 2 === 0 ? 100 : 200,
}));
const generateData = (size = 10) =>
Array.from({ length: size }, (_, index) => ({
onSelect: () => undefined,
currentItemSize: index % 2 === 0 ? 100 : 200,
}));

const data = generateData();

const dataWithVariableSizes = Array.from({ length: 10 }, (_, index) => ({
onSelect: () => undefined,
Expand All @@ -56,8 +59,15 @@ describe('SpatialNavigationVirtualizedList', () => {
</SpatialNavigationRoot>,
);

const VirtualizedListWithNavigationButtons = () => {
const listRef = useRef<SpatialNavigationVirtualizedListRef>(null);
// This is bad practice but easiest way to access the ref from outside the component for testing
let currentListRef: React.RefObject<SpatialNavigationVirtualizedListRef>;
const VirtualizedListWithNavigationButtons = ({ listSize = 10 }) => {
currentListRef = useRef<SpatialNavigationVirtualizedListRef>(null);

const data = useMemo(() => {
return generateData(listSize);
}, [listSize]);

return (
<SpatialNavigationRoot>
<SpatialNavigationNode orientation="vertical">
Expand All @@ -70,19 +80,19 @@ describe('SpatialNavigationVirtualizedList', () => {
itemSize={100}
numberOfRenderedItems={5}
numberOfItemsVisibleOnScreen={3}
ref={listRef}
ref={currentListRef}
/>
</DefaultFocus>
<TestButton title="Go to first" onSelect={() => listRef.current?.focus(0)} />
<TestButton title="Go to last" onSelect={() => listRef.current?.focus(9)} />
<TestButton title="Go to first" onSelect={() => currentListRef.current?.focus(0)} />
<TestButton title="Go to last" onSelect={() => currentListRef.current?.focus(9)} />
</>
</SpatialNavigationNode>
</SpatialNavigationRoot>
);
};

const renderVirtualizedListWithNavigationButtons = () =>
render(<VirtualizedListWithNavigationButtons />);
const renderVirtualizedListWithNavigationButtons = (size = 10) =>
render(<VirtualizedListWithNavigationButtons listSize={size} />);

it('renders the correct number of item', async () => {
const component = renderList();
Expand Down Expand Up @@ -626,4 +636,26 @@ describe('SpatialNavigationVirtualizedList', () => {
expectButtonToHaveFocus(component, 'button 10');
expectListToHaveScroll(listElement, -700);
});

it('jumps to first and last element for a huge list, even while focus is still on the list', async () => {
const component = renderVirtualizedListWithNavigationButtons(1000);
act(() => jest.runAllTimers());

setComponentLayoutSize(listTestId, component, { width: 300, height: 300 });
const listElement = await component.findByTestId(listTestId);

testRemoteControlManager.handleRight();
testRemoteControlManager.handleRight();
expectButtonToHaveFocus(component, 'button 3');
expectListToHaveScroll(listElement, -200);

act(() => {
currentListRef?.current?.focus(999);
});
expectButtonToHaveFocus(component, 'button 1000');
act(() => {
currentListRef?.current?.focus(0);
});
expectButtonToHaveFocus(component, 'button 1');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ export const SpatialNavigationVirtualizedListWithScroll = typedMemo(
setCurrentlyFocusedItemIndex(index);
if (idRef.current) {
const newId = idRef.current.getNthVirtualNodeID(index);
spatialNavigator.deferredFocus(newId);
spatialNavigator.grabFocusDeferred(newId);
}
},
}),
Expand Down
1 change: 1 addition & 0 deletions packages/lib/src/spatial-navigation/helpers/isError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const isError = (e: unknown): e is Error => e instanceof Error;
Loading