Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

measureLayout doesn't work properly for ScrollView container #1957

Closed
nandorojo opened this issue Mar 22, 2021 · 11 comments
Closed

measureLayout doesn't work properly for ScrollView container #1957

nandorojo opened this issue Mar 22, 2021 · 11 comments
Milestone

Comments

@nandorojo
Copy link
Contributor

nandorojo commented Mar 22, 2021

The problem

I want to scroll to an element which is inside of a ScrollView. In order to get the element's scroll position, I'm trying to use measureLayout.

However, if I pass a ScrollView as the "container" in measureLayout, the callback never fires.

How to reproduce

Simplified test case: https://snack.expo.io/@nandorojo/measurelayout-example

Steps to reproduce:

  1. Add a ScrollView, pass it a ref: scrollRef
  2. Add a text inside of the ScrollView, give it a ref: textRef
  3. Call textRef.current.measureLayout(scrollRef.current, callback)

Expected behavior

The third argument in measureLayout should fire the callback. However, nothing happens.

If I change the "relative to" ref from scrollRef to a View's ref, everything works as expected.

When I run the code on React Native (iOS, Android) it works fine. You'll see this on the Expo Snack URL.

Environment (include versions). Did this work in previous versions?

  • React Native for Web (version): ^0.14.10 (Snack uses an older one, 0.13.x)
  • React (version): 16.13.1
  • Browser: Chrome

I can't get it to work on older versions.

Additional context

I posted about this on the #web channel on the React Native discord.

@Simek reproduced the issue, and mentioned this:

it might be a react-native-web issue, because it look like the implementation only covers the deprecated method signature, which uses node, instead of ref: https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/exports/UIManager/index.js#L23

@necolas
Copy link
Owner

necolas commented Mar 22, 2021

It looks like this is because a ScrollView ref is a React class instance rather than a host node.

it look like the implementation only covers the deprecated method signature, which uses node, instead of ref

I don't know what this means.

@Simek
Copy link
Contributor

Simek commented Mar 22, 2021

@necolas As you can see in the React Native docs the measureLayout has two signatures:

The new variant of this method seems to fail in react-native-web. The example code in Snack below works on core platforms but has no effect on the Web:

@necolas
Copy link
Owner

necolas commented Mar 22, 2021

Didn't realise this had changed in 0.64. The plan is to eventually deprecate all the instance methods on RN refs, so I'm not sure why we've changing that API

@Simek
Copy link
Contributor

Simek commented Mar 22, 2021

Didn't realise this had changed in 0.64. The plan is to eventually deprecate all the instance methods on RN refs, so I'm not sure why we've changing that API

The Expo SDK v40 is based on RN 0.63 and if I remember correctly the change has been already a part of one of 0.63 releases, but I wasn't able to find specific changelog entry or commit.

@necolas
Copy link
Owner

necolas commented Mar 22, 2021

Turns out the RN docs are confusing. You pass ref.current not ref. So there's no issue with the measure function.

The issue is the scrollview ref is not a host component but a component instance. If you wrap the scrollview ref in a findNodeHandle then it works: https://snack.expo.io/ap6i4KVhK

@Simek
Copy link
Contributor

Simek commented Mar 22, 2021

Feel free to update the docs, since you seems to have better understanding what's actually happening in there and additionally you have some internal knowledge too.

The issue is the scrollview ref is not a host component but a component instance. If you wrap the scrollview ref in a findNodeHandle then it works: https://snack.expo.io/ap6i4KVhK

Hmm, but findNodeHandle was an alternative for previous solution and afaik should not be required since method signature refactor. 🤷‍♂️

@necolas
Copy link
Owner

necolas commented Mar 22, 2021

I'm not saying it should be required. I'm saying the reason you gave for this issue is incorrect and it is what I've already explained twice.

@necolas necolas reopened this Mar 24, 2021
@elicwhite
Copy link

elicwhite commented Mar 24, 2021

I believe you can call scrollviewref.getNativeScrollRef().measureLayout(...) to use the non-deprecated API. Using findNodeHandle here is deprecated.

https://github.com/facebook/react-native/blob/master/Libraries/Components/ScrollView/ScrollView.js#L892

@necolas necolas added this to the 0.15.* milestone Mar 25, 2021
@necolas
Copy link
Owner

necolas commented Mar 31, 2021

This should be fixed in 0.15.5

@nandorojo
Copy link
Contributor Author

nandorojo commented Aug 21, 2021

@necolas This should be re-opened. I found a situation where the measurement is incorrect.

measureLayout relative to a ScrollView works properly if and only if the current scroll position is 0. Once you've scrolled down, it returns the incorrect measurement.

Here's a reproduction: https://codesandbox.io/s/lucid-sea-rq0ny?file=/src/App.js:0-1339

Code here
import React from "react";
import { ScrollView, StyleSheet, Text, View } from "react-native";

const data = new Array(50).fill("").map((_, i) => i);

const HEIGHT = 100;

function App() {
  const viewRefs = React.useRef({});
  const scrollRef = React.useRef();

  const scrollTo = (index) => () => {
    viewRefs.current[index].measureLayout(scrollRef.current, (x, y) => {
      console.log("[scroll-to]", { y, expected: index * HEIGHT });
      scrollRef.current.scrollTo({
        y,
        animated: true
      });
    });
  };

  return (
    <View style={styles.app}>
      <ScrollView ref={scrollRef}>
        {data.map((item, i) => (
          <View
            ref={(ref) => {
              viewRefs.current[i] = ref;
            }}
            style={[styles.item, i % 2 ? styles.oddItem : styles.evenItem]}
            key={item}
          >
            <Text onPress={scrollTo(i)} style={styles.itemText}>
              {item}
            </Text>
          </View>
        ))}
      </ScrollView>
    </View>
  );
}

const styles = StyleSheet.create({
  app: {
    flex: 1
  },
  item: {
    justifyContent: "center",
    alignItems: "center",
    height: HEIGHT
  },
  oddItem: {
    backgroundColor: "#7928CA"
  },
  evenItem: {
    backgroundColor: "black"
  },
  itemText: {
    color: "white"
  }
});

export default App;
  1. Click on a number. This should scroll to it.
  2. Look in the console. It outputs the location it should scroll to vs the result of measureLayout.

It seems that measureLayout relative to a ScrollView works sometimes.

By comparison, it works properly on iOS. Here is the same code on Expo Snack: https://snack.expo.dev/@beatgig/dedc14 (don't use this to test RNW, since they use 0.13 on Snack.)

The problem

The issue is, react-native-web is subtracting the scroll position from the child item's layout, whereas React Native always measures from the top of the scroll content container.

For instance, if the scroll position is currently 100, and you measure they layout for an item that is at y: 100, measureLayout from RNW returns y: 0, whereas react-native returns y: 100 (which is correct).

To summarize: measureLayout relative to a ScrollView works properly if and only if the current scroll position is 0.

Let me know if you need help testing a fix. Thanks!

@necolas
Copy link
Owner

necolas commented Aug 21, 2021

That's a different issue. Please open a new issue

Repository owner locked as resolved and limited conversation to collaborators Aug 21, 2021
rnike pushed a commit to VeryBuy/react-native-web that referenced this issue Sep 13, 2022
Make sure the 'ref' for a ScrollView is a host node with React Native's
proprietary and legacy instance methods attached.

Fix necolas#1957
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants