bamlab / react-tv-space-navigation

A React Native module to handle spatial navigation for a TV application in a 100% cross-platform way
https://bamlab.github.io/react-tv-space-navigation/
MIT License
178 stars 15 forks source link

Issue navigating side to side on more rows rendered #96

Closed dgocoder closed 3 months ago

dgocoder commented 3 months ago

Describe the bug I'm seeing a weird issue that at first load is not a problem but as I navigate the grid and come back the dpad is requiring multiple taps to move left to right. Code below and video sample is provided. Its mostly copy paste from the hoppix demo with just a different component rendering image.

To Reproduce

const NUMBER_OF_COLUMNS = 6;
const NUMBER_OF_ROWS_VISIBLE_ON_SCREEN = 3;
const NUMBER_OF_RENDERED_ROWS = NUMBER_OF_ROWS_VISIBLE_ON_SCREEN + 2;
const INFINITE_SCROLL_ROW_THRESHOLD = 2;
const NUMBER_OF_ITEMS_VISIBLE_ON_SCREEN = 7;
const WINDOW_SIZE = NUMBER_OF_ITEMS_VISIBLE_ON_SCREEN + 8;

const renderItem = useCallback(
    ({ item }: { item: ScheduleProgramWithProgramAndChannel }) => (
      <MovieNode movie={item} onSelect={() => onPress(item)} />
    ),
    [onPress],
  );

return (
    <Screen>
      <DefaultFocus>
        <SpatialNavigationScrollView
          offsetFromStart={140}
          style={{ backgroundColor: "black" }}
        >
          <View className="h-[1000px] bg-black pr-4 pt-[18.5px]">
            {moviesLoading ? (
              <Loader />
            ) : (
              <SpatialNavigationVirtualizedGrid
                data={schedulePrograms}
                style={{ backgroundColor: "#000000" }}
                renderItem={renderItem}
                itemHeight={207 * 1.075}
                // header={<Header />}
                // headerSize={185}
                numberOfColumns={NUMBER_OF_COLUMNS}
                numberOfRenderedRows={NUMBER_OF_RENDERED_ROWS}
                numberOfRowsVisibleOnScreen={NUMBER_OF_ROWS_VISIBLE_ON_SCREEN}
                onEndReachedThresholdRowsNumber={INFINITE_SCROLL_ROW_THRESHOLD}
                scrollInterval={150}
              />
            )}
          </View>
        </SpatialNavigationScrollView>
      </DefaultFocus>
    </Screen>
  );

Expected behavior I would expect left / right navigation to work as they did on load before navigating down and coming back up.

Screenshots

https://github.com/bamlab/react-tv-space-navigation/assets/13984190/46b01f39-c671-4ec7-a4a5-e159eb6cd76a

Version and OS

sClarkeDev commented 3 months ago

I am also experiencing this bug. You can also see this on the live example, specifically the virtualized grid example.

pierpo commented 3 months ago

Edit: wrong issue, sorry.

dgocoder commented 3 months ago

@pierpo Unfortunately we will have more than 100 grid items.

I want to call out that the last video does not have a header component at all set. Perhaps I misunderstood your comment, but I took out the header to isolate the issue so that it was separated out from the others.

pierpo commented 3 months ago

I am so sorry, I answered on the wrong issue! You should ignore my comment, and I will inspect this issue. I'll move it to the other thread.

pierpo commented 3 months ago

About this issue, that is very weird indeed!

Can I have a look at your MovieNode?

Also, you should try to remove the ScrollView, as the Grid already makes a scroll (it's actually a fake scroll for smoothness reasons, using a CSS translate, so it won't interoperate properly with a ScrollView!)

pierpo commented 3 months ago

I am also experiencing this bug. You can also see this on the live example, specifically the virtualized grid example.

Thank you for the precision. Do you see it on the web or only on Android?

sClarkeDev commented 3 months ago

I am also experiencing this bug. You can also see this on the live example, specifically the virtualized grid example.

Thank you for the precision. Do you see it on the web or only on Android?

Both web and android.

dgocoder commented 3 months ago

@pierpo here is the movie node. I copied what was in demo which did wrap grid in scrollview. Tried removing scrollview but did not solve issue.

image
type Props = {
  movie: ScheduleProgramWithProgramAndChannel;
  onSelect?: () => void;
  indexRange?: [number, number];
};

export const MovieNode = forwardRef<SpatialNavigationNodeRef, Props>(
  ({ movie, onSelect, indexRange }: Props, ref) => {
    return (
      <SpatialNavigationFocusableView
        onSelect={onSelect}
        indexRange={indexRange}
        viewProps={{ accessibilityLabel: movie.program.title ?? "no-name" }}
        ref={ref}
      >
        {({ isFocused }) => <Movie movie={movie} isFocused={isFocused} />}
      </SpatialNavigationFocusableView>
    );
  },
);
MovieNode.displayName = "MovieNode";
dgocoder commented 3 months ago

I'm not sure if this is related but I also see it glitch with certain nodes as well vertically.

https://github.com/bamlab/react-tv-space-navigation/assets/13984190/47dadb87-39f1-47f0-b137-bbbcf240ade9

pierpo commented 3 months ago

Indeed there is a scrollview in the example as well. That's a mistake, it's not supposed to be there 😁

@sClarkeDev you said that you reproduce on the web. You do reproduce it here, on a browser like Chrome? https://bamlab.github.io/react-tv-space-navigation/

If you do, can you share a video of that example? I can't reproduce myself 😭

sClarkeDev commented 3 months ago

Yes, here is the video of me reproducing it in Mozila FireFox v124.0.2.

I can also reproduce in Chrome using the same steps.

https://github.com/bamlab/react-tv-space-navigation/assets/143657751/88777495-c98d-4580-985c-83a16cab612e

pierpo commented 3 months ago

Thank you for the help, I do reproduce indeed. Quite an important bug 🤯 I'll try to have a look as soon as possible.

pierpo commented 3 months ago

Btw, I have a feeling that this might come from 3.0.0. I did a major change in the logic of how the elements are registered.

I just tried v2.1.1 and it looks like it works properly. If you need nothing from v3, you can give it a try. (no API breaking change, the major was only here to indicate that there was a huge change in the core)

pierpo commented 3 months ago

I think I have a fix. I'm still testing it on all platforms.

This will be in the hall of fame of my dumbest bugs 😂

Here is the PR. You can try the patch if it's not merged but I'll try to publish it later today. https://github.com/bamlab/react-tv-space-navigation/pull/103

pierpo commented 3 months ago

I just published the fix with 3.3.0. I'll close the issue as I'm quite confident that the fix will work 😊 Feel free to reopen if it's not the case.

I'd like to thank you both for the amazing issue illustrated with videos and clear explanations. That's extremely helpful, so... thanks 💞