NoriginMedia / react-spatial-navigation

DEPRECATED. HOC-based Spatial Navigation. NEW Hooks version is available here: https://github.com/NoriginMedia/norigin-spatial-navigation
MIT License
226 stars 64 forks source link

Focusable items hidden are able to be accessed. #109

Closed AdrianBrown1 closed 2 years ago

AdrianBrown1 commented 2 years ago

Describe the bug We currently have a bug where I am able to touch focusable events that are covered by the current screen i'm on using React Navigation. We are using React Navigation to push new screens and when we do this we are running into issues with Focus events getting lost and focusing on items in the background of the current screen. Is there a solution to this?

Expected behavior I would expect to only be able to focus on items on the current screen rather than HOC focusable components.

Any guidance / help would be greatly appreciated.

asgvard commented 2 years ago

Hi!

There is a "blockNavigationOut" property specifically for this purpose. Hope it helps 👍

AdrianBrown1 commented 2 years ago

@asgvard Thank you! I will try this out asap and let you know if this worked.

AdrianBrown1 commented 2 years ago

@asgvard do you have an example of how to use blockNavigationOut ?

asgvard commented 2 years ago

It's quite straightforward, this property has to be set for the Screen or Popup component that will act as a "boundary" for the focus.

<Screen
  blockNavigationOut
/>

or

<Popup
  blockNavigationOut
/>

I can't think of any more detailed example :) We are using React Navigation ourselves and just adding this property to our screens or popups.

AdrianBrown1 commented 2 years ago

@asgvard I am attempting to add it to the wrapped Focusable component but I am getting the error below. We get to this screen by calling navigation.push('DetailScreen', {...params}).

Is there a way to apply this property without calling it directly like suggested above? <DetailScreen blockOutNavigation />

Screen Shot 2021-10-14 at 9 57 19 AM

asgvard commented 2 years ago

This is an error from the TS type checks. We do not (yet) officially provide TS support, so the types you are using are at your own risk. If these are the type definitions you are using: https://github.com/NoriginMedia/react-spatial-navigation/issues/41#issuecomment-564973916, they do not have blockOutNavigation in the withFocusableOpts, you can perhaps manually add it there. We are still planning a big refactoring of this library into hooks + TS support though. Stay tuned :)

AdrianBrown1 commented 2 years ago

@asgvard Should we only use Class Components in order to properly use properties like blockOutNavigation?

We currently are using Functional Components but without hooks support this might not be the best approach.

asgvard commented 2 years ago

Hi,

We are using functional components ourselves. Having class components wouldn't make any difference. Unfortunately it is hard to understand the problem just from the screenshot. We are actively using screen stacking with React Navigation, and blockOutNavigation should restrict focus going out from the focusable container (active screen).

AdrianBrown1 commented 2 years ago

Okay so we currently have the Main Screen of our app that is pushing a new screen like so...

this.props.navigate.push('DetailScreen', {...params})

Screen two is a functional Component that is apart of our Navigation Stack with 2 Focusable Buttons on the entire screen. We are wrapping the entire screen in a focusable like so >

export default withFocusable({ blockNavigationOut: true })(Detail);

However we are still able to Focus items that are on the HomeScreen.

@asgvard

asgvard commented 2 years ago

By "tapping" do you mean that you are able to press on the items on the touch screen device or click on them in the browser? If so, this library only handles the spatial navigation with the arrow keys (left-right-up-down) and changes the focus between them. It doesn't control any other interactions such as taps or clicks.

AdrianBrown1 commented 2 years ago

apologies I am using the up, down, left, right keys on the web simulator but we are getting the same behavior using the TV simulator as well. We are able to focus on elements from the first screen while we are on the 2nd screen and have that blockNavigationOut value set to true.

@asgvard

asgvard commented 2 years ago

Sounds like a bug then :) thanks for the information, we will get back to you. I will discuss it with our team, maybe someone encountered similar issue. If we don't have any immediate idea, we will probably ask you to create a demo project where this problem can be reproduced. E.g. barebone app with React navigation and 2 screens representing the issue. I understand that it probably would not be possible to share the real project.

AdrianBrown1 commented 2 years ago

Okay that sounds great! thanks for the quick feedback it is greatly appreciated! I look forward to hearing from you!

AdrianBrown1 commented 2 years ago

We also are using ReNative & using Tizen. We could also provide a sample project ASAP as this is a big blocking issue for us & we are on a tight deadline so any information or help you need from us we are happy to provide. @asgvard Thanks again!

guilleccc commented 2 years ago

Hi @AdrianBrown1,

We are using blockNavigationOut with React Navigation at the component level, not as a HOC prop. Consider you have a DetailScreen (or DetailContainer) then you should pass it like in the following example:

const DetailScreen = (props) => {
  ...

  return (<Detail
    {...otherProps}
    blockNavigationOut
  />);
};

Could you please try this out? If the error persists, please send us a demo example where we can reproduce it, and we will try to help you.

AdrianBrown1 commented 2 years ago

So we were able to get the issue fixed with our own work around.

We originally had our Stack Navigator set up like so ...

      <Stack.Navigator
        initialRouteName={HOME}
        screenOptions={{ headerShown: false }}
      >
        <Stack.Screen name={HOME} component={HomeScreen} />
        <Stack.Screen name={DETAIL} component={DetailScreen} />
      </Stack.Navigator>
    </NavigationContainer>

VS working implementation ...

     <NavigationContainer>
      <Stack.Navigator
        initialRouteName={HOME}
        screenOptions={{ headerShown: false }}
      >
        <Stack.Screen
          name={HOME}
          children={props => (
            <HomeScreen
              focusable={false}
              focusKey={HOME}
              {...props}
            />
          )}
        />
        <Stack.Screen
          name={DETAIL}
          children={props => (
            <DetailScreen
              focusable={false}
              focusKey={DETAIL}
              {...props}
            />
          )}
        />
      </Stack.Navigator>
    </NavigationContainer>

We are still not able to get blockNavigationOut working on its own without focusable={false} set. We will move forward with the current implementation as it is working as intended but please let us know if we are following the intended implementation of this API properly. If you have any other suggestions for us to try we are happy to give that a try!

I look forward to hearing back!

guilleccc commented 2 years ago

So we were able to get the issue fixed with our own work around.

We originally had our Stack Navigator set up like so ...

      <Stack.Navigator
        initialRouteName={HOME}
        screenOptions={{ headerShown: false }}
      >
        <Stack.Screen name={HOME} component={HomeScreen} />
        <Stack.Screen name={DETAIL} component={DetailScreen} />
      </Stack.Navigator>
    </NavigationContainer>

VS working implementation ...

    <NavigationContainer>
     <Stack.Navigator
       initialRouteName={HOME}
       screenOptions={{ headerShown: false }}
     >
       <Stack.Screen
         name={HOME}
         children={props => (
           <HomeScreen
             focusable={false}
             focusKey={HOME}
             {...props}
           />
         )}
       />
       <Stack.Screen
         name={DETAIL}
         children={props => (
           <DetailScreen
             focusable={false}
             focusKey={DETAIL}
             {...props}
           />
         )}
       />
     </Stack.Navigator>
   </NavigationContainer>

We are still not able to get blockNavigationOut working on its own without focusable={false} set. We will move forward with the current implementation as it is working as intended but please let us know if we are following the intended implementation of this API properly. If you have any other suggestions for us to try we are happy to give that a try!

I look forward to hearing back!

Would it be possible that you to share a demo project that we can clone and test ourselves? I would need to see how you are creating the withFocusable on HomeScreen and other components.

AdrianBrown1 commented 2 years ago

Hello @guilleccc

Apologies for the delay in response. Unfortunately we haven't had the time to create a demo project you can run but below is the pattern we are using for creating Focusable screens. Please let me know if this clears things up for you or if you need more information. We are using this pattern for all screens in our application that need to Focusable

const HomeScreen = (props) => {
return (
  <View>
   <TouchableOpacity onPress={() =>  props.navigation.push('details')}>
      <Text>Navigate to Details</Text>
   </TouchableOpacity>
  </View>
 );
}

export default withFocusable()(HomeScreen);
asgvard commented 2 years ago

Hi,

I will close it due to inactivity. If you still experience the same issue, please feel free to reopen, but we would need a demo project where the issue is easily reproducible. We are using "blockNavigationOut" and stack navigation on multiple projects and haven't experienced issues with the feature itself.