Wrong direction of focus navigation.

[fulminant]

Describe the bug I have a situation where I need to display the button only after some loading logic. And after I implemented a simple piece of code I observed weird behavior. The first button that I need to show appears in the correct place conditionally, but the focus direction is not correct. In the video, you can see how the first button is accessible through navigation by buttons on the remote.

Assumption I guess this happens when spatialNavigator.registerNode, gets children from DOM where there is no first button at that moment. After the button appears it registers it as a latest element which we could see on focus behavior. The button is accessible as the last right (in my case) element.

To Reproduce You need to conditionally show SpatialNavigationNode.

<SpatialNavigationView direction="horizontal">
  {!isLoading && (
    <SpatialNavigationFocusableView>
      {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >First button</Text>
      )}
    </SpatialNavigationFocusableView>
  )}

  <SpatialNavigationFocusableView>
    {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >Second button</Text>
      )}
  </SpatialNavigationFocusableView>

  <SpatialNavigationFocusableView>
    {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >Third button</Text>
      )}
  </SpatialNavigationFocusableView>
</SpatialNavigationView>

Expected behavior The focus direction works correctly, and the first button (in my case) will be accessible not as the last right but as the first left focus element.

Comment Saw a comment in the code: react-tv-space-navigation/src/spatial-navigation/components/Node.tsx:118

/*

• We don't re-register in LRUD on each render, because LRUD does not allow updating the nodes.
• Therefore, the SpatialNavigator Node callbacks are registered at 1st render but can change (ie. if props change) afterwards.
• Since we want the functions to always be up to date, we use a reference to them. */

As far as I understand, the LRUD library doesn't support updating, and to update navigation, we need to update the whole navigation with all children which could be an expensive operation.

In case this behavior is really a bug I'm ready to help and contribute to fixing it, but maybe I need some guidance.

Screenshots

[fulminant]

Found a workaround using the index property from the LRUD library.

LRUD /docs/usage.md#index

Need to add a new prop to SpatialNode called index in /src/spatial-navigation/components/Node.tsx and update spatialNavigator.registerNode to something like that:

spatialNavigator.registerNode(id, {
  parent: parentId,
  index,
  isFocusable,
  onBlur: () => {
    currentOnBlur.current?.();
    setIsFocused(false);
  },
  onFocus: () => {
    currentOnFocus.current?.();
    setIsFocused(true);
  },
  onSelect: () => currentOnSelect.current?.(),
  orientation,
  isIndexAlign: alignInGrid,
  indexRange,
  onActive: () => setIsActive(true),
  onInactive: () => setIsActive(false),
});

After this modifications we'll be able to pass index prop to SpatialNode like this:

<SpatialNavigationView direction="horizontal">
  {!isLoading && (
    <SpatialNavigationFocusableView index={0} alignInGrid={true}>
      {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >First button</Text>
      )}
    </SpatialNavigationFocusableView>
  )}

  <SpatialNavigationFocusableView index={1} alignInGrid={true}>
    {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >Second button</Text>
      )}
  </SpatialNavigationFocusableView>

  <SpatialNavigationFocusableView index={2} alignInGrid={true}>
    {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >Third button</Text>
      )}
  </SpatialNavigationFocusableView>
</SpatialNavigationView>

It works perfectly. But as we need isIndexAlign set to true, that this feature will work I can assume that will somehow conflict with alignInGrid logic.

UPD Looks like isIndexAlign only needed to work with indexRange and has no effect on index itself.

[pierpo]

Hey! Sorry that I didn't answer quicker 😁 This is actually a very classic issue with the lib. While the index will work, it's quite annoying and not very scalable.

The best solution (in my opinion), is to add SpatialNavigationNode that are not conditionally rendered. Your component inside can be conditionally rendered (and can have its own Nodes/FocusableViews).

This is all documented here 😊 https://github.com/bamlab/react-tv-space-navigation/blob/main/docs/pitfalls.md

Here's the fix for you:

<SpatialNavigationView direction="horizontal">
+ <SpatialNavigationNode>
  {!isLoading && (
    <SpatialNavigationFocusableView>
      {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >First button</Text>
      )}
    </SpatialNavigationFocusableView>
  )}
+ </SpatialNavigationNode>

  <SpatialNavigationFocusableView>
    {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >Second button</Text>
      )}
  </SpatialNavigationFocusableView>

  <SpatialNavigationFocusableView>
    {({ isFocused }) => (
        <Text
          style={{
            color: 'white',
            backgroundColor: isFocused ? 'tomato' : 'green',
          }}
        >Third button</Text>
      )}
  </SpatialNavigationFocusableView>
</SpatialNavigationView>

[pierpo]

This is one very faulty abstraction with the library but I don't know how to make it clearer (or solve it completely) 😭 I wish we could just do it the natural way without the trick of fake nodes.

[fulminant]

@pierpo, Thanks for your response. I missed the documented pitfalls)

Agree with you that is not a super scalable option. Maybe we can get a node index related to its parent from DOM and put it automatically, without the necessity to set prop?

[fulminant]

Also, the proposed workaround works perfectly, but assume that you not only want to show loading but to show node only on success response. In this case, we'll have an empty Node in case of a failure response.

[pierpo]

Also, the proposed workaround works perfectly, but assume that you not only want to show loading but to show node only on success response. In this case, we'll have an empty Node in case of a failure response.

Yes, which is totally fine... Because it is non focusable 😁 You can try out this error case (you can force your call to fail to see how it behaves), it works perfectly 😊 we've been using this pattern a lot without any problems.

[pierpo]

I added an example in the app to showcase the solution in a more visible way: https://github.com/bamlab/react-tv-space-navigation/pull/121

I think that will do it for now 😊