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

[Bug] onBecameFocused always returns 0 for all dimensions on native platforms #48

Closed shirakaba closed 4 years ago

shirakaba commented 4 years ago

Describe the bug

I'm trying to make my ScrollView scroll to the focused item (just like a Netflix-style carousel) whenever the focus updates (i.e. upon onBecameFocused). It works flawlessly on the web (via React Native Web) but not on Android/iOS, wherein the x value is always reported as 0, rather than the focused item's currently-measured offsetLeft.

To Reproduce

const scrollViewRef = React.createRef();
const SpatialItem = withFocusable()(({ item, ...props }) => <Text {...props}>{item.content}<Text/>);

const ScrollViewThatScrollsToFocusedItem = ({ items }) => (
  <ScrollView ref={scrollViewRef} horizontal>
    {items.map((item, index) => {
      const itemFocusKey = `focusable-item-${item._key}`;

      return (
        <SpatialItem
          item={item}
          focusKey={itemFocusKey}
          key={itemFocusKey}
          onBecameFocused={
            ({ x, y, top, left, width, height }) => {
              console.log(`[onBecameFocused] x: ${x}`);
              // BUG: Logs incorrect value on native.

              scrollViewRef.current.scrollTo({ x });
            }
          }
        />
      );
    })}
  </ScrollView>
);

You can render <ScrollViewThatScrollsToFocusedItem> as follows:

// Make several items in this format; I've included an array of one for brevity.
<ScrollViewThatScrollsToFocusedItem items={[{ _key: item1, content: "Item 1" }]} />

Expected behavior The ScrollView should scroll along to the focused <SpatialItem> child whenever the focused child updates.

Additional context

I believe the core of the problem is in measureLayout.js. In the measureLayout() function, the condition if (relativeNode) { } is never satisfied for a native app, because the node.parentNode API does not exist in React Native. Thus, the frame of the focusable is always reported as the default starting value (0 for all the values of the frame, e.g. x and y).

I've tried introducing the following else block for that condition to fix things, but it's not really working:

else {
  node.measure((fx, fy, width, height, px, py) => {
    var frameRect = {
      width: width,
      height: height,
      left: fx,
      top: fy,
    };
    var pageRect = {
      width: width,
      height: height,
      left: px,
      top: py,
    };
    console.log('[measure] Component rect to frame is: ', frameRect);
    console.log('[measure] Component rect to page is: ', pageRect);

    /* Note: I'm not sure if this maths is accurate, but React Native doesn't expose
     * any APIs closer to the DOM implementation, so I'll enter them as a proof-of-concept. */
    var x = pageRect.left - frameRect.left;
    var y = pageRect.top - frameRect.top;

    callback(x, y, frameRect.width, frameRect.height, frameRect.left, frameRect.top);
  });
}

This improves (but far from fixes) the behaviour as follows:

Disclaimer: it's a bit hard to characterise, so I may not be completely accurate in my above analysis of what the logic is behind the reported values for iOS and Android.

Basically, it's failing partially due to race conditions (due to both React Spatial Navigation's layout measurement process and React Native's asynchronous nature), partially due to Android/iOS differences, and largely because there are no equivalent measurement APIs to the DOM ones.

Could anyone more familiar with the layout measurement process please investigate this?

Related: #47 by @salvan13 (which improved the Web layout measurement behaviour, but had no impact on the native layout measurement behaviour).

asgvard commented 4 years ago

Hi,

We are aware of this issue, however the problem is that measuring the layout in native environments is quite heavy since it is making calls to a native thread and it is async. That is why it is intended that it doesn't return coordinates which is also documented in README. In order to solve this issue we have implemented this outside of this library (in the project itself). I can provide a snippet, I hope it helps to get an idea of how to handle this. But basically in this callback you also get a node reference (that is valid in native as well):

node && UIManager.measureLayout(
            findNodeHandle(node),
            findNodeHandle(scrollRef),
            noop,
            (measuredX, _y, measuredWidth, ...restCoordinates) => {
              // perform scrolling based on the values you get here
            }
          );

This UIManager.measureLayout is not very well documented, but basically what it is doing is that it can find relative coordinates based on the current and parent node you pass in.

asgvard commented 4 years ago

Please let me know if it works for you. I'm not sure we will add native layout measuring in this library since it is very specific to native environments and it is quite heavy :) But feel free to reopen if you still have any issues with this approach.