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] [Animated] Fix Animated-driven transform state when deleting views from view hierarchy #22839

Closed
wants to merge 2 commits into from

Conversation

gavrix
Copy link

@gavrix gavrix commented Jan 2, 2019

This is an implementation of proposal #17475.
I haven't been using ReactNative since original evaluation about a year ago, upon revisiting I see this issue is still around.

Issue:

When deleting a view which UI state (like transform) has been connected to AnimatedProps, said state is reset to defaults before running deleting animation (fade to transparent using opacity, by default). This makes it impossible to implement nice swipe-to-delete experience (when swipe is driven by native driver)

Proposal

To iterate what I described in #17475: instead of disconnected animated component from native view when component is unmounted, keep that connection at native side around until native view is fully purged from the hierarchy.

Concerns:

I'm not 100% sure this is a correct approach. May there be legitimate use cases when AnimatedProps has to be disconnected from native view while latter has to remain in the hierarchy? If anyone has broader context around this mechanism I'd like to discuss.

Changelog:

[iOS] [Changed] - AnimatedProps isn't disconnected from native view (by calling __disconnectAnimatedView) when component is unmounted on js side. Instead, runtime on native side is keeping track of such connections and disconnects when native view is fully removed from hierarchy.

Test Plan:

As a proof-of-concept please see gif provided in #17475.
I wanted to provide a proper integration test but couldn't wrap my head around how to best do it. Any help or hint on direction would much appreciated.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 2, 2019
@matthargett
Copy link
Contributor

@hramos it looks like this is dependent on the iOS snapshot test fix PR by @grabbou #22861, if we want CI to be green before merge.

@gavrix does Android not have this problem as well?

@hramos hramos removed the 🔶APIs label Jan 24, 2019
@gavrix
Copy link
Author

gavrix commented Feb 8, 2019

I came to my attention that Reanimated implements the same functionality as Animated and somehow doesn't have this issue. I am going to close this issue and consider Reanimated preferred animation framework.

@gavrix gavrix closed this Feb 8, 2019
@matthargett
Copy link
Contributor

That solution works for you, but should this PR still be merged to fix the underlying issue in RN? It seems so to me :)

@gavrix
Copy link
Author

gavrix commented Feb 14, 2019

It was an experiment and only supported on iOS and I was mainly asking for feedback and I found that issue is not reproducible with Reanimated (which is basically an Animated replacement)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants