fix(lists): fix imperative focus not working properly on android (#149)

* fix(lists): fix imperative focus for lists not working on android

* refactor(lists): rename deferred focus method

* test(lists): add test for imperative focus on long lists

Author: Pierre Poupin

packages/lib/src/spatial-navigation/SpatialNavigator.ts

@@ -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 };
@@ -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.
@@ -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;
+      }
+    }
   };
 
   /**
@@ -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);
@@ -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) => {

packages/lib/src/spatial-navigation/components/virtualizedList/SpatialNavigationVirtualizedList.test.tsx

@@ -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';
 
@@ -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,
@@ -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">
@@ -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();
@@ -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');
+  });
 });

packages/lib/src/spatial-navigation/components/virtualizedList/SpatialNavigationVirtualizedListWithScroll.tsx

@@ -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);
             }
           },
         }),

packages/lib/src/spatial-navigation/helpers/isError.ts

@@ -0,0 +1 @@
+export const isError = (e: unknown): e is Error => e instanceof Error;