Skip to content

Fix layout animations when adding and removing views at the same time on Android #16091

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

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Sep 26, 2017

Actual fix for #7959 that was abandoned because of some issues with that implementation.

This fixes some view hierarchy inconsistencies caused by layout delete animations. The fix is similar to what was done on iOS recently. To be able to do so properly we introduce "React" views in ViewGroupManager to allow the native view hierarchy to diverge from what React knows about.

This also fixes another bug that is caused by animations being interrupted. To fix this we make sure to clearAnimations before starting a new one so that onAnimationEnd is called and we update the opacity to the final value there in case the animation didn't finish. Sadly Android seems to have really bad support for running multiple animations at the same time so just updating the value to the final value was the best fix for now.

Caveats
This won't work if the parent uses removeClippedSubviews. In that case it doesn't use ViewGroupManager methods because it implements its own way of having a different native and react view hierarchies. Since we're moving away from removeClippedSubviews (I think so haha), I didn't bother trying to make it work. This might be a bad idea since it will crash with some out of bounds errors and I'm not sure how we could detect that and show a useful warning. Another alternative would be to use the new React views api to reimplement subview clipping but I'd rather not mess with that code tbh :).

Perf
This should have a negligible impact since we're only adding a single map lookup to view operations for views that don't diverge from their native view hierarchy. It might be worth trying to add a check to get rid of the additional mapping if we detect that both native and react children becomes the same again after diverging.

Test plan
Used this gist https://gist.github.com/janicduplessis/211d78c5408ab17dee7ad2e381648a48 to reproduce the bugs and made sure there was no longer any inconsistencies when spamming a bunch of add and remove views. Also made sure no views were leaked because of the extra mapping we keep in mReactChildrenMap.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 26, 2017
@janicduplessis
Copy link
Contributor Author

cc @shergin This is similar to the fix you did on iOS

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@janicduplessis I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@jeanregisser
Copy link
Contributor

@janicduplessis @shergin any way to move this forward?

One app I'm working on is impacted by this bug. Would love to see this merged.

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Nov 7, 2017

@jeanregisser After trying this in a production app I found an issue where sometimes touch events are blocked (probably by a view with 0 opacity that doesn't get removed). I need to investigate this before we can move forward.

I would really like to also add tests to this since it is somewhat complex and easy to break.

@jeanregisser
Copy link
Contributor

Ok! Thanks for the update @janicduplessis! Let me know if I can help in any way.

@stale
Copy link

stale bot commented Jan 7, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 7, 2018
@stale stale bot closed this Jan 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants