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

[ios][bug] Huge issue with LayoutAnimation - Single Animated.View with layout prop is enough to break un-mounting and de-allocation of any View which has been popped/swiped-back/closed/destroyed via navigation #3124

Closed
1 of 3 tasks
hirbod opened this issue Apr 1, 2022 · 69 comments
Assignees
Labels
Important This seem to be a serious issue and we will need to take a deeper look into it some time soon Needs review Issue is ready to be reviewed by a maintainer Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snippet of code, snack or repo is provided

Comments

@hirbod
Copy link
Contributor

hirbod commented Apr 1, 2022

Description

@piaskowyk @kmagiera

I have found a serious bug with LayoutAnimation. I have been trying to investigate the problem for several days, which is why videos that were in a screen or modal that I closed (goBack/pop/swipe to dismiss) still played even though everything was un-mounted and destroyed. The same is true for cameras (Green Light Indicator is no longer out).

It is sufficient to have a single Reanimated.View / createAnimatedComponent ANYWHERE in the app which uses layout, entering or exiting prop from Reanimated. Once a single View with that prop has been mounted (no matter where), no element that is in a navigation stack will be unmounted anymore, even if the layout hierarchy and lifecycle methods reports that it has been unmounted (eg cleanup function of useEffect). When you remove something conditionally on the screen (eg. with useState), un-mounting works fine. But it looks like a single use of LayoutAnimation is enough to break element un-mounting / de-allocation when closing Navigators, because (this is my guess) LayoutAnimation is still "watching all of them (or snapshotting?)" in order to be able to animate the layout, even when those views are in different screens. Maybe RNS is also an issue here?

I also did some tests, such as setting breakpoints at the deallocate function of expo-av. The methods were never called when layout, entering or exiting were present and mounted anywhere in the app. But if no Reanimated.View with layout was used the breakpoints triggered.

I have also created a video as well as a repro which can be tested with Expo Go.

Expected behavior

Views should correctly un-mount and deallocate once a Navigation screen has been dismissed / popped etc, even when using LayoutAnimation.

Actual behavior & steps to reproduce

To test my findings, simply grab my repro, spin it up, open Tab One and tap on the info icon at the top (headerRight) - as many times as you like. The video will start and stop as soon as the modal has been closed. But if you now switch to Tab two, which mounts a Reanimated.View with layout={Layout}, (imported from REA) and you now simply return to Tab one, you will notice that the video will continue to run as soon as you open and close the modal and it will start multiple videos until the App crashes eventually. If you remove layout from Tab two, you can open and close the modal 1000x and it will work fine. Please note that you need to restart the app when you add or remove layout, fast-refresh is not enough.

I provided a repro and all informations in my issue + in my repro App as Text.
Here is also a Video showing the issue (watch with audio please)

Watch with audio

Bildschirmaufnahme.2022-04-01.um.02.10.50.mp4

Snack or minimal code example

https://github.com/hirbod/react-reanimatad-layoutanimation-bug

And this single layout prop is enough to trigger the issue (once mounted):
https://github.com/hirbod/react-reanimatad-layoutanimation-bug/blob/main/screens/TabTwoScreen.tsx#L7

Package versions

name version
react-native 0.64.3
react-native-reanimated 2.5.0 (but breaks with any version that has LayoutAnimation)
Xcode 13.3
expo SDK 44

Affected platforms

  • Android
  • iOS
  • Web

Here is a cross link to RNS, since I am not sure if RNS or REA is the root issue:
software-mansion/react-native-screens#1379

@hirbod hirbod added the Needs review Issue is ready to be reviewed by a maintainer label Apr 1, 2022
@hirbod hirbod changed the title [ios] Huge issue with LayoutAnimation - Single view with layout is enough to break un-mounting and de-allocation of views which has been popped/swiped-back/closed/destroyed via navigation [ios][bug] Huge issue with LayoutAnimation - Single view with layout is enough to break un-mounting and de-allocation of views which has been popped/swiped-back/closed/destroyed via navigation Apr 1, 2022
@github-actions github-actions bot added Repro provided A reproduction with a snippet of code, snack or repo is provided Platform: iOS This issue is specific to iOS labels Apr 1, 2022
@hirbod hirbod changed the title [ios][bug] Huge issue with LayoutAnimation - Single view with layout is enough to break un-mounting and de-allocation of views which has been popped/swiped-back/closed/destroyed via navigation [ios][bug] Huge issue with LayoutAnimation - Single Animated.View with layout prop is enough to break un-mounting and de-allocation of any View which has been popped/swiped-back/closed/destroyed via navigation Apr 1, 2022
@adgarcia
Copy link

adgarcia commented Apr 1, 2022

Nice timing; we ran into this bug about 2 hours ago. Like you said any exit/enter/layout prop anywhere breaks anything in a navigation stack everywhere.

@LeviWilliams
Copy link

LeviWilliams commented Apr 1, 2022

Thank you so much for posting this, we have been racking our brains at why all kinds of random things in our app stopped working when we implemented reanimated v2 layout anims (images, svgs, other animations, etc). As soon as we remove the entering/exiting props across the application everything works as normal.

Weirdly enough in our case iOS is not affected but Android is. This is a massive blocker for us, anyway we could get some eyes on this when you guys have some time? @j-piasecki

@RMG0
Copy link

RMG0 commented Apr 1, 2022

I am facing the same issue like when we use entering/exiting/layout prop from reanimated V2 some of the ui views goes invisible. Thanks for investigating this and bringing this up as this is hard to debug and find the root cause. When I commented all entering/exiting/layout props, it works perfectly. A huge blocking for our case as we added many animations and now this adds visibility issue for other UIs.
Also, this is for android only in my case
In my case somehow when I use any of entering/exiting/layout props, zindex/layering is done wrongly on newly mounted screen , so my invisible ui were behind some fullscreen ui. (I temporarily fixed by reordering component sequence but ideally reanimated should not touch zindex/layering)
It would be great if someone can look into this.

@hirbod
Copy link
Contributor Author

hirbod commented Apr 1, 2022

Glad that my findings helped you. I searched 15 hours without sleep until I found the root cause (I was THIS close to switch RNS to RNN because I was out of ideas. It was a lucky find at the end and never have I ever thought that reanimated could be the cause 😅 )

I did not encounter bugs on Android though, at least in my specific scenarios, everything was working fine. I only did use LayoutAnimation on two FlatLists and a couple of buttons. What hurts the most is my beautiful TextInput animation. I hope I don't have to remove LayoutAnimation and they will be able to fix the issue soon.

I think it would be helpful if you guys could also provide a failing repro on Android, this might help to get both issues resolved.

@LeviWilliams
Copy link

@hirbod I'll try to get a minimum repro on Android out asap. We're using exiting/entering in a much larger capacity than just on some lists so we would have to rewrite lots of this with normal react native animations. Would be unfortunate with all the benefits reanimated brings. Also surprised this has not been reported sooner, seems like a severe issue to me.

@hirbod
Copy link
Contributor Author

hirbod commented Apr 1, 2022

I mean, there are alternatives, like https://moti.fyi which use reanimated under the hood without LayoutAnimation (but be careful, they also added support for LayoutAnimation from REA as well if you would like to use it).
Its pretty straightforward to animate (also entering/exiting) with AnimatePresence, but there is no comparison to LayoutAnimation (specially how easy it is to use).

The issue is severe imho, I think the feature is just "too new" and not adopted widely enough. Thats why nobody had complaints so far. And I think its pretty hard to find the root cause when things start acting weird (like elements not un-mounting). I would've been curious if I had a LayoutAnimation on a Video, but for me it was a Button inside an invisible (but mounted) tab.

@LeviWilliams feel free to copy my repro to create your case with android.

@pairohit305
Copy link

pairohit305 commented Apr 1, 2022

I am too facing the same issue on android
It crashes everytime on exiting the app and only happens if I use layout animation feature
image

@hirbod
Copy link
Contributor Author

hirbod commented Apr 1, 2022

I think thats something different @pairohit305. We should try not to mix up different issues.

@RMG0
Copy link

RMG0 commented Apr 1, 2022

@pairohit305 , this is a different(crash) issue so lets not mix this with thread
Thank you

@piaskowyk
Copy link
Member

@hirbod Hey, I have just started to investigate it. This is a priority issue, I will notify you about progress.

@piaskowyk piaskowyk self-assigned this Apr 5, 2022
@piaskowyk piaskowyk added the Important This seem to be a serious issue and we will need to take a deeper look into it some time soon label Apr 5, 2022
@hirbod
Copy link
Contributor Author

hirbod commented Apr 5, 2022

@piaskowyk thank you very much! Appreciated.

@mateioprea
Copy link

mateioprea commented Apr 6, 2022

I'm also affected by this. I've created a view with a hardcoded height and tried to layout animate only that view using

if ( [targetValues[@"height"] isEqualToNumber:@329] ) {}

in - onViewCreate:() in REAAnimationsManager.

and it works. the unmounted views are deallocated.

In my case, the bug reported by @hirbod is visible when using a combination of AVPlayer and Navigation.reset().

Resetting the stack won't deallocate the avplayer instance, leading to hitting the AVPlayer instances limit which is 15. Due to this no videos will be displayed in the app anymore.

@LeviWilliams
Copy link

LeviWilliams commented Apr 6, 2022

In our case, we were able to resolve some of our issues by reordering the hierarchy. We realized that lots of views using zIndex were affected and what was happening was the views were being rendered in the wrong order causing them to be fully under other root views (????). Still, we observed odd mounting/unmounting behaviour while fixing this and reordering things, certain orders worked and certain didn't.

To be clear these affected views had no relation to any form of layout animations and were even in different navigation stacks. Assuming reanimated was affecting both lifecycle methods and internally modifying zIndex's or something.

@hirbod
Copy link
Contributor Author

hirbod commented Apr 6, 2022

We need to clearly distinguish between all the other errors reported here:

  1. My error is related to iOS - un-mounting and de-allocation of views, most likely there
    is something in REA and RNS causing this
  2. Similar/related issues reported in this thread for Android (still missing a reproduction for this)
  3. Another report for iOS, with reproduction, most likely related:
    Layout animations create duplicates nodes in react native SVG  #3093
    Duplication of Nodes, especially when react-native-svg is used.

All of this might be related or be isolated issues.

@LeviWilliams
Copy link

LeviWilliams commented Apr 6, 2022

Following up here as we have reproduced what @hirbod initially reported. Our iOS app is affected by an RTMP stream never releasing even when unmounted, similar problem to what is displayed in the reproduction. Commenting out layout props resolves the issue as well.

We can still try to reproduce the zIndex hierarchy issue in a repo if necessary. We've spent a whole pile of time debugging all these issues so reproduction has been a little delayed so far

@piaskowyk
Copy link
Member

Good news, I found the cause of the issue today and I know how to resolve it.

@hirbod
Copy link
Contributor Author

hirbod commented Apr 7, 2022

This one specific or all of them? :)
Anyway, these are great news! Thanks for your work.

@RMG0
Copy link

RMG0 commented Apr 7, 2022

I hope root issue fix would all case :)
Thank you for all efforts

@davidskaarup
Copy link

davidskaarup commented Oct 6, 2022

It seems like there is no activity in the rewrite branch since early July. We basically cannot use layout animation in its current state and I suspect many are in the same situation.
Is there any workaround we can do to be able to use layoutanimation until a rewrite/fix is out?

@hirbod
Copy link
Contributor Author

hirbod commented Nov 18, 2022

FYI: Reanimated V3 RC6 includes the all new LayoutAnimation rewrite. I think its time to close this issue, right @piaskowyk ? :)

In case you guys asking: when your codebase does not make use of any Rea1 functions, its safe to upgrade to Rea3 (its Rea2 with Fabric support but works also with the old architecture). If you can't upgrade to REA3 because of Rea1 legacy libraries, you'll have to wait. The rewrite will be backported to v2 soon.

This issue was long lasting but pretty complex. But you're not only getting a stable LayoutAnimation, you're getting SHARED ELEMENTS with react-native-screens - NATIVE. Thanks a ton to everybody involved in this.

@paullinator
Copy link

@hirbod LayoutAnimations with V3 RC6 seem even more broken. I get screen corruption and frozen UI as soon as layout animations are enabled. I haven't filed an issue yet, but wondering if anyone else is seeing this with RC6

@hirbod
Copy link
Contributor Author

hirbod commented Nov 28, 2022

@paullinator I am currently on vacation and did not test the RC, I was just spreading the news. Filing issues with repos and videos are helpful though, as SWM asked for testers and reports!

@caolong0204
Copy link

waiting for  resolve :((

@TomasSestak
Copy link

Cant use the functionality at all until this is fixed

@zhelezkov
Copy link

zhelezkov commented Jan 27, 2023

I guess #3865 is supposed to fix this one?
Faced same issue in our project, will try to validate if fix in PR helps

@iway1
Copy link

iway1 commented Feb 1, 2023

probably should mark the API as "not safe for use" or something until the rewrite is finished considering it can just brick an app with no error message - I've been using the API quite a lot and had no idea it had this problem until it happened to me.

Any recommendation from @software-mansion? Don't use the API at all? Only use it in certain circumstances? Anything?

@ziyoshams
Copy link

probably should mark the API as "not safe for use" or something until the rewrite is finished considering it can just brick an app with no error message - I've been using the API quite a lot and had no idea it had this problem until it happened to me.

Have you tried the latest RC? It has the rewrite.

@iway1
Copy link

iway1 commented Feb 1, 2023

@ziyoshams no but I'm not super comfortable migrating my apps to an RC

@ziyoshams
Copy link

@iway1 don't migrate. See if the new layout rewrite fixes your issues.

@iway1
Copy link

iway1 commented Feb 3, 2023

@ziyoshams turns out mine had nothing to do with this issue

@Bowlerr
Copy link

Bowlerr commented Feb 15, 2023

Any news on this one?

@nateshmbhat
Copy link

any update on this issue ?

@hirbod
Copy link
Contributor Author

hirbod commented Mar 5, 2023

Reanimated v3 was released three days ago. All of these issues should be fixed.

@kirbi96
Copy link

kirbi96 commented Mar 14, 2023

resolve for me after update to 3.2.0 version

@Osebrian
Copy link

Good news, I found the cause of the issue today and I know how to resolve it.

@piaskowyk Please let us know if this issue has been resolved

@BalogunofAfrica
Copy link

@Osebrian The rewrite shipped with one of the minor versions of Reanimated 3, so you're all good now.

@hirbod
Copy link
Contributor Author

hirbod commented May 18, 2023

The time has come. After testing REA 3 very intensively, I take a bow and say thanks to everybody involved!

@hirbod hirbod closed this as completed May 18, 2023
@piaskowyk
Copy link
Member

@hirbod glad to read it 😊

@dFelinger
Copy link

Using @react-navigation/native-stack with react-native-vision-camera when I navigate back from stack screen with camera there is green dot on iOS still active. What am I doing wrong? react-native-reanimated is in the project with version 3.5.4

@vargajacint
Copy link

@dFelinger I think you missed this:

function App() {
  const isFocused = useIsFocused()
  const appState = useAppState()
  const isActive = isFocused && appState === "active"

  return <Camera {...props} isActive={isActive} />
}

@dFelinger
Copy link

In @react-navigation/native-stack page is not rendering on out even if I use useIsFocused - isFocused is never in false state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Important This seem to be a serious issue and we will need to take a deeper look into it some time soon Needs review Issue is ready to be reviewed by a maintainer Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snippet of code, snack or repo is provided
Projects
None yet
Development

Successfully merging a pull request may close this issue.