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

Scrollview not working properly in v4 formSheet #2424

Open
peterjskaltsis opened this issue Oct 21, 2024 · 29 comments · May be fixed by #2436
Open

Scrollview not working properly in v4 formSheet #2424

peterjskaltsis opened this issue Oct 21, 2024 · 29 comments · May be fixed by #2436
Assignees
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided

Comments

@peterjskaltsis
Copy link

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: react-navigation/react-navigation#12196 (comment)

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:

repro-recording.mov

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

@github-actions github-actions bot added Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided labels Oct 21, 2024
@kkafar
Copy link
Member

kkafar commented Oct 21, 2024

Hey!

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

Thank you!

@kkafar kkafar self-assigned this Oct 21, 2024
@kkafar
Copy link
Member

kkafar commented Oct 21, 2024

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
Copy link
Author

peterjskaltsis commented Oct 21, 2024

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
Copy link
Member

kkafar commented Oct 22, 2024

@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
Copy link
Author

Hmm interesting, yes I am using Paper at the moment

@kkafar
Copy link
Member

kkafar commented Oct 22, 2024

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
Copy link
Member

kkafar commented Oct 22, 2024

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

Screen.Recording.2024-10-22.at.13.06.34.mov

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/[email protected])

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
Copy link
Author

peterjskaltsis commented Oct 22, 2024

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)

Screen.Recording.2024-10-22.at.10.19.25.pm.mov

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
Copy link
Member

kkafar commented Oct 22, 2024

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!

@kkafar
Copy link
Member

kkafar commented Nov 4, 2024

I just wanted to update here, that the issue turned out to be more complex than I've thought initially & I've moved to doing other things, because most likely we'll need ability of synchronous React Native's layout on UI thread do resolve this issue, which is not available on Paper (and not yet sure how to do it on Fabric). Will be back to handling this after we release v4 (should be days from now).

@kot-ezhva
Copy link

I'm also have some trouble with v4

1.-.01.mp4

@a-eid
Copy link

a-eid commented Nov 14, 2024

I have a siliar issue with formsheet on ios where it addes additional padding to the bottom, notice the difference between android & ios.

form-sheet-RNSv4.mp4

however this is not scrollview related.

@kot-ezhva
Copy link

kot-ezhva commented Nov 14, 2024

however this is not scrollview related.

I tried to put inside just <View style={{flex:1, backgroundColor: 'red'}} /> I expected see full-height view, but seen nothing :)

@a-eid
Copy link

a-eid commented Nov 14, 2024

I see that this was filed as a paper issue, however I am using the new arch.

however this is not scrollview related.

I tried to put inside just <View style={{flex:1, backgroundColor: 'red'}} /> I expected see full-height view, but seen nothing :)

this also happens on the new arch.

@a-eid
Copy link

a-eid commented Nov 14, 2024

  return (
    <View style={{ flex: 1, backgroundColor: "red", marginBottom: 70 }}>
      {/*  */}
      {/*  */}
    </View>
  )
Simulator.Screen.Recording.-.iPhone.15.-.2024-11-14.at.23.55.00.mp4

FYI this is on the new arch.

@kot-ezhva
Copy link

  return (
    <View style={{ flex: 1, backgroundColor: "red", marginBottom: 70 }}>
      {/*  */}
      {/*  */}
    </View>
  )

Simulator.Screen.Recording.-.iPhone.15.-.2024-11-14.at.23.55.00.mp4

exactly strange behaviour

@a-eid
Copy link

a-eid commented Nov 15, 2024

somehow this is related to react-native-reanimated, I have this simple Component that when commented out the bug is no longer reproducible.

the component is on the parent screen.

Screenshot 2024-11-15 at 3 26 01 AM

I know it doesn't make sense ¯_(ツ)_/¯.

@kot-ezhva
Copy link

Problem not resolved. Any solution about worked ScrollView inside 'formSheet' screen?
Simple temporary solution for me it replace formSheet to modal. For iOS it looks similar (in my case it's ok)

screencast.2024-11-16.14-40-18.-.01.-.01.mp4

@rainerllera
Copy link

Any updates on this? I see your PR from October @kkafar, any luck with that? For context, in my case (Expo 52, Fabric) ScrollView inside modal screens (not formSheet) is behaving weirdly and I think it may be related with this issue.

The ScrollView's height seems to be miscalculated, and "jumps" on touch events. Sometimes it shows correctly when opening a modal, and some other times it's just wrong (same modal, same screen). Seems a bit random.

Because of the height variation, a component I have as a "footer" of these screens (sibling to the scrollview, with margin top auto) is sometimes pushed outside of the modal screen, without the possibility to scroll to it.

I've managed to make things work again by just placing my footer component inside the scrollview itself, with a fixed margin top, which makes it look kinda bad, but at least it is functional.

This started happening after I upgraded from Expo 51 to 52 (which included the react-navigation upgrade to v7, and screens to v4.1.0) and enabled new arch.

@fedpre
Copy link

fedpre commented Dec 6, 2024

Any update? I am using formSheet and if I nest a ScrollView inside the screen there is a scrolling conflict on Android. If I keep pressing the scroll I can go up and down, but if I scroll down, leave, and try to go back up on the list, it actually closes the formSheet instead of correctly scrolling to the top of the list AND THEN closing the formSheet.
This behaviour is correct on iOS.

Screen.Recording.2024-12-06.at.9.13.12.AM.mov

@danieljvdm
Copy link

Bumping on this again... as far as I can tell flex: 1 doesn't work on any components within a formSheet presentation. I'm trying to do something like Instagram's comment formSheet where there are two detents, one half, one full, and the content expands to fit the size of the sheet.

@mnelabs-kevin
Copy link

Bumping on this again... as far as I can tell flex: 1 doesn't work on any components within a formSheet presentation. I'm trying to do something like Instagram's comment formSheet where there are two detents, one half, one full, and the content expands to fit the size of the sheet.

Also seeing this (RN 0.76.5), commenting to follow along.

@chanphiromsok
Copy link

chanphiromsok commented Jan 16, 2025

flex style not apply on formSheet ,The flex style apply on the Screen with config options formSheet

options={{
      presentation: "formSheet",
      animation: "slide_from_bottom",
     gestureEnabled: true,
     gestureDirection: "vertical",
     headerShown: true,
    sheetGrabberVisible: true,
     sheetInitialDetentIndex: 0,
     sheetAllowedDetents: [0.9, 1.0],
    sheetCornerRadius: 100,
}}

Image

Updated

The release already include in caution that limit support flex 1 https://github.com/software-mansion/react-native-screens/releases/tag/4.0.0

@RohovDmytro
Copy link

RohovDmytro commented Jan 16, 2025

I just wanted to update here, that the issue turned out to be more complex than I've thought initially & I've moved to doing other things, because most likely we'll need ability of synchronous React Native's layout on UI thread do resolve this issue, which is not available on Paper (and not yet sure how to do it on Fabric). Will be back to handling this after we release v4 (should be days from now).

@kkafar Thanks for taking a look and digging in. Is there any updates on the matter?

@peterjskaltsis
Copy link
Author

I just wanted to update here, that the issue turned out to be more complex than I've thought initially & I've moved to doing other things, because most likely we'll need ability of synchronous React Native's layout on UI thread do resolve this issue, which is not available on Paper (and not yet sure how to do it on Fabric). Will be back to handling this after we release v4 (should be days from now).

Congrats on the recent big releases @kkafar! Unfortunately since I raised this issue I'm still blocked from trying out all the new versions of this package in our app. I just wanted to check if it was still on your radar after v4? 🚀

@kkafar
Copy link
Member

kkafar commented Feb 1, 2025

Hey, sorry this isn't fixed yet. There are plenty of tickets on the radar just haven't had opportunity to dig into this one yet. I'll try to handle this one in upcoming 1-2 weeks. Thanks for the remainder.

@a-eid
Copy link

a-eid commented Feb 5, 2025

@kkafar thank you for the great work overall including this feature,

I would like to note that, formsheet screens don't behave unexpectedly with just the scrollview. I've noticed some other issues with normal views as well.

  • if the screen is wrapped with a View, it doesn't behave properly with a flex: 1.
  • it's children also can't have a flex: 1.

they have to be given an explicit height, which I am not sure if that the desired behavior.

@a-eid
Copy link

a-eid commented Feb 5, 2025

there also seem to be an issue with insets on android. ( I have edge to edge active ).

Screen.Recording.2025-02-06.at.1.44.15.AM.mov

@a-eid
Copy link

a-eid commented Feb 6, 2025

there also seem to be an issue with insets on android. ( I have edge to edge active ).

Screen.Recording.2025-02-06.at.1.44.15.AM.mov

apparently this issue doesn't always happen. sometime I am able to repro but not always.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided
Projects
None yet
10 participants