react-tv-space-navigation icon indicating copy to clipboard operation
react-tv-space-navigation copied to clipboard

`SpatialNavigationNode` isFocusable change is not working.

Open lifeisegg123 opened this issue 1 year ago • 2 comments

Describe the bug The SpatialNavigationNode component does not function correctly when the isFocusable prop is changed dynamically.

To Reproduce

const [focusable, setFocusable] = useState(false);

return (
  <SpatialNavigationNode isFocusable={focusable}>
    {() => <SomeView />}
  </SpatialNavigationNode>
);

Expected behavior The component should be able to receive focus when isFocusable is set to true and should not receive focus when it is false.

Screenshots If applicable, add screenshots to help explain your problem.

Version and OS

  • Library version:4.0.1
  • React Native version: react-native-web 0.19.13
  • OS: web

Additional context I found that adding isFocused as a dependency at this line resolves the issue. However, I'm uncertain if this is an appropriate solution.

lifeisegg123 avatar Nov 22 '24 07:11 lifeisegg123

Hi,

Good catch, I hadn't thought of this. It's very unexpected indeed. The patch is dangerous and will break things if you have nested nodes. It works if you don't 😁 But I would strongly not recommend it. Why? Because it unregisters / re-registers the node when isFocusable will change. Un-registering the node will also un-register the children, but we have no way to know this on the children side, so we will just kill all the children nodes.

This piece of code is about adapting the LRUD lib - the core underneath the spatial navigation - to React, and unfortunately this part is not very flexible (hence the non standard useEffect where we ignored some dependencies).

The cleanest way would be to use setNodeFocusable https://github.com/bbc/lrud/blob/master/docs/usage.md#modifying-node-focusability in a useEffect.

This needs a bit of manual testing but that should be OK 😁

// in Node.tsx
useEffect(() => {
  spatialNavigator.setIsFocusable(id, isFocusable);
}, [spatialNavigator, isFocusable])

// in SpatialNavigator.ts

public setIsFocusable(nodeId: string, isFocusable: boolean) {
  this.lrud.setIsFocusable(nodeId, isFocusable);
}

pierpo avatar Nov 23 '24 02:11 pierpo

@pierpo thanks. it worked for my case 👍

lifeisegg123 avatar Dec 17 '24 13:12 lifeisegg123