Fix LayoutAnimation iOS delete bug when adding and removing views at the same time #7942
Conversation
|
@janicduplessis updated the pull request. |
|
@facebook-github-bot shipit |
|
Thanks for importing. If you are an FB employee go to Phabricator to review. |
|
I made a couple of small changes such as using an NSMutableSet instead of an array for the deleted views (which means calling I verified everything is fine for our use cases, but it would be very helpful if you could put up another PR to add/extend the example to include the scenario you encountered, since we don't have any tests for that. In particular, the code path for the scenario where views are added and removed in the same cycle seems like it could do with some test coverage. |
|
Hey Janic, I just had to unland the PR because it modified the snapshot tests and it looks like a break. You can see how the snapshot tests got changed: https://github.com/facebook/react-native/pull/8048/files Nick is not available to investigate this at this moment, would you be able to incorporate Nick's changes and investigate why the layout got broken? Thanks, |
|
@bestander Is it possible that this is related to one of the commits that landed right before this one? As far as I can tell there are 2 that seems related to text and by the snapshot test image it seems that is what changed. |
|
@nicklockwood I'll try to add some integration tests to layout animation soon as well as modify the UIExplorer example to have a more complex example that used to break without this fix. |
|
Hey @janicduplessis, you are right. |
…the same time Summary: Fix a bug that happens when views are added and removed in the same manageChildren block with a delete animation. What happens is that the inserted view is not inserted at the proper index if the deleted view index is smaller than the inserted one. This is because the view is not immediately removed from the subviews array so we need to offset the insert index for each view that is going to be deleted with an animation and is before the inserted view. To do this I separated `_removeChildren` into 2 different functions, one for animated delete and one for normal delete. The animated one returns an array of `RCTComponent` that are going to be animated. We can then use this array to offset the insert index. **Test plan (required)** Tested that this fixed the bug in an app where I noticed it, also tested the UIExplorer example to make sure LayoutAnimations still worked properly. Closes facebook#7942 Differential Revision: D3417194 Pulled By: nicklockwood fbshipit-source-id: 790f4ac15a8552323b359e6466cecfa80418c63c
Summary: Fix a bug that happens when views are added and removed in the same manageChildren block with a delete animation. What happens is that the inserted view is not inserted at the proper index if the deleted view index is smaller than the inserted one. This is because the view is not immediately removed from the subviews array so we need to offset the insert index for each view that is going to be deleted with an animation and is before the inserted view. To do this I separated `_removeChildren` into 2 different functions, one for animated delete and one for normal delete. The animated one returns an array of `RCTComponent` that are going to be animated. We can then use this array to offset the insert index. **Test plan (required)** Tested that this fixed the bug in an app where I noticed it, also tested the UIExplorer example to make sure LayoutAnimations still worked properly. Closes facebook#7942 Differential Revision: D3417194 Pulled By: bestander fbshipit-source-id: 9145a783e520c6718dd023a1e006646acb09cb7c
…the same time Summary: Fix a bug that happens when views are added and removed in the same manageChildren block with a delete animation. What happens is that the inserted view is not inserted at the proper index if the deleted view index is smaller than the inserted one. This is because the view is not immediately removed from the subviews array so we need to offset the insert index for each view that is going to be deleted with an animation and is before the inserted view. To do this I separated `_removeChildren` into 2 different functions, one for animated delete and one for normal delete. The animated one returns an array of `RCTComponent` that are going to be animated. We can then use this array to offset the insert index. **Test plan (required)** Tested that this fixed the bug in an app where I noticed it, also tested the UIExplorer example to make sure LayoutAnimations still worked properly. Closes facebook#7942 Differential Revision: D3417194 Pulled By: nicklockwood fbshipit-source-id: 790f4ac15a8552323b359e6466cecfa80418c63c
Summary: Fix a bug that happens when views are added and removed in the same manageChildren block with a delete animation. What happens is that the inserted view is not inserted at the proper index if the deleted view index is smaller than the inserted one. This is because the view is not immediately removed from the subviews array so we need to offset the insert index for each view that is going to be deleted with an animation and is before the inserted view. To do this I separated `_removeChildren` into 2 different functions, one for animated delete and one for normal delete. The animated one returns an array of `RCTComponent` that are going to be animated. We can then use this array to offset the insert index. **Test plan (required)** Tested that this fixed the bug in an app where I noticed it, also tested the UIExplorer example to make sure LayoutAnimations still worked properly. Closes facebook#7942 Differential Revision: D3417194 Pulled By: bestander fbshipit-source-id: 9145a783e520c6718dd023a1e006646acb09cb7c
|
I'm suffering this issue for quite a long time, why not reopen this as it has been reverted @janicduplessis |
|
It was shipped again but I think there are still other issues with this but I haven't had the time to look into it. |
Fix a bug that happens when views are added and removed in the same manageChildren block with a delete animation. What happens is that the inserted view is not inserted at the proper index if the deleted view index is smaller than the inserted one. This is because the view is not immediately removed from the subviews array so we need to offset the insert index for each view that is going to be deleted with an animation and is before the inserted view.
To do this I separated
_removeChildreninto 2 different functions, one for animated delete and one for normal delete. The animated one returns an array ofRCTComponentthat are going to be animated. We can then use this array to offset the insert index.Test plan (required)
Tested that this fixed the bug in an app where I noticed it, also tested the UIExplorer example to make sure LayoutAnimations still worked properly.