software-mansion / react-native-screens

Native navigation primitives for your React Native app.
https://docs.swmansion.com/react-native-screens/
MIT License
3.04k stars 515 forks source link

Scrollview not working properly in v4 formSheet #2424

Open peterjskaltsis opened 1 day ago

peterjskaltsis commented 1 day ago

Description

Not sure if I should be posting this here or in react-navigation, nor if its too early since things are still in beta!

But I was requested to post this via: https://github.com/react-navigation/react-navigation/pull/12196#issuecomment-2425929004

Scrolling inside a scrollview inside a formSheet modal does not seem to work on new rc/beta react-navigation & react-native-screens.

See video demo here:

https://github.com/user-attachments/assets/ee93e076-71f1-4c13-b3c6-c1d8a0deeb8e

Steps to reproduce

  1. Open a modal type formSheet on native stack with ScrollView in it
  2. Try to scroll, bounces back to top
  3. If you pull up on the modal towards status bar & then try to scroll, it works

Snack or a link to a repository

https://github.com/peterjskaltsis/rn-screens-repro

Screens version

4.0.0-beta.11

React Native version

0.74.5

Platforms

iOS

JavaScript runtime

Hermes

Workflow

Expo managed workflow

Architecture

Paper (Old Architecture)

Build type

Debug mode

Device

iOS simulator

Device model

iPhone 16 Pro

Acknowledgements

Yes

kkafar commented 1 day ago

Hey!

  1. Love your font
  2. Every feedback is great
  3. I'll be looking into it in couple of minutes

Thank you!

kkafar commented 1 day ago

I have the guilty one - view flattening and this is bad.

Despite the fact we define hierarchy in form of Screen -> ContentWrapper -> View -> ScrollView the resulting one is Screen -> ContentWrapper -> View, ScrollView. See:

image

And this is bad, because we look for the scrollview in the direct line (full DFS on whole subtree is rather no-no for performance reasons). If the view flattening is applied at any level higher in the HostTree this algorithm won't work...

@peterjskaltsis Right now the best solution is to just disable the view flattening by setting collapsable={false} on the view wrapping the scroll view:

  return (
    <View style={{backgroundColor: 'crimson' }} collapsable={false} >
      <ScrollView>
        {Array.from({ length: 90 }).map((_, index) => (<Text key={index}>I'm a long scrollview</Text>))}
        <View style={{ width: '100%', height: 50, backgroundColor: 'seagreen' }} />
      </ScrollView>
    </View>
  )
peterjskaltsis commented 1 day ago

Thank you for the quickly reply & explanation @kkafar. That makes sense 👍 would this be a blocker for the v4 full release?

In terms of solution, I am unable to get it working by adding collapsable={false} the parent view. Once I do this, even dragging the header up before scrolling (as seen in my original recording) is no longer working.

Were you able to get it working with that change locally?

kkafar commented 10 hours ago

@peterjskaltsis Yes, setting collapsable={false} did make it work for me.

I want to clarify one aspect: in the issue report you specified that you're using old architecture (Paper), is that accurate? Could you confirm it?

peterjskaltsis commented 10 hours ago

Hmm interesting, yes I am using Paper at the moment

kkafar commented 9 hours ago

Yeah... I was testing on Fabric. On paper I do not have the problem at all, as I believe view flattening does not exist on Paper & iOS combination .

kkafar commented 9 hours ago

@peterjskaltsis I'm using exactly the code you provided now on Paper & it seems that everything is in order.

https://github.com/user-attachments/assets/94308daa-cda5-4618-9fd1-5f4444b7c674

One thing that differs in our approaches is usage of Expo Go, but that not really in our scope to fix.

[!important] Please note, that I'm using latest beta of react-native-screens 4.0.0-beta.13 and current main of react-navigation (you can use @react-navigation/native-stack@7.0.0-rc.28)

I do not know what version of libraries are shipped with Expo Go. It might be reasonable to wait for beta release of sdk 52, which contains up-to-date pacakage versions.

peterjskaltsis commented 9 hours ago

Thank you very much for digging into this one!

Confirmed it works on Paper in my repro repo if I adde the sheetAllowedDetents.

I think it has something to do with the top of the screen.

Notice when I change it to default sheetDetents it does not scroll properly. But if I drag the modal towards the status bar and then try to scroll, it works? (video attached)

https://github.com/user-attachments/assets/d7ce2d7c-933f-4bb8-94f1-b803b394294f

BTW I confirmed this does not occur on Fabric 👍 Also I am not using Expo Go, I am using managed expo via running expo prebuild && expo run:ios

kkafar commented 9 hours ago

Okay! I confirm the issue. If sheetAllowedDetents is not set or if the sheetAllowedDetents is set to a single value - the bug occurs.

Thanks!

I'll look into this later today!