Fix issue in removing items with LayoutAnimation on Android. When LayoutAnimation is enabled, indicesToRemove are incorrect, which makes adjacent views disappear. #11962

vinceyuan · 2017-01-18T10:04:07Z

Fix issue #11828 Removing items with LayoutAnimation causes adjacent views/texts to disappear on Android

I explained my solution in #11828 (comment)

…n is enabled, indicesToRemove are incorrect, which makes adjacent views disappear.

facebook-github-bot · 2017-01-18T10:04:41Z

By analyzing the blame information on this pull request, we identified @ASCE1885 and @janicduplessis to be potential reviewers.

vinceyuan · 2017-01-18T10:06:03Z

@janicduplessis Please take a look. Thanks.

vinceyuan · 2017-01-24T14:39:16Z

Ping @janicduplessis

janicduplessis · 2017-01-24T21:04:41Z

Thanks for the PR! I'll have a look this week ;)

janicduplessis · 2017-02-02T22:55:40Z

Could you provide a test plan that describe how you tested this?

Also are you sure that indicesToRemove and tagsToDelete are always the same? I tried fixing the indices a while back but ended up finding some edge cases where it didn't work. #7959

vinceyuan · 2017-02-03T03:44:07Z

@janicduplessis
I explained my solution in #11828 (comment)
Test plan:
Tested that this fixed the bug in https://github.com/vinceyuan/ReactNativeOpacityBugDemo. See screenshot with this fix:

I am not sure that indicesToRemove and tagsToDelete are always the same. Probably they are not same when LayoutAnimation is not enabled. If we totally ignore indicesToRemove, there will be problems in removing items without animation. You can see some views are not removed after I tap Remove without animation button in the following screenshot:

Probably this fix is not perfect. But with this fix, everything works well with/without LayoutAnimation. 😸

freakinruben · 2017-02-04T16:01:05Z

Does this solve #9549 as well?

vinceyuan · 2017-02-04T16:47:29Z

@freakinruben Probably not. This PR fixes an issue in LayoutAnimation on Android. #9549 doesn't seem to be animation related.

vinceyuan · 2017-02-11T13:21:56Z

@janicduplessis Any comment about this PR? Is it good?

janicduplessis

I'm not so confident landing this as is since there are cases where it will obviously be broken. I know the current implementation also has issues but I just want to make sure we don't introduce other bugs.

I think the ideal solution would be to implement something in ViewManager that offsets index for views that are being delete-animated. ViewManager could keep a map of these views and offset indices in it's methods that operate with an index.

I realize this will be a bigger change but I'd rather have a solution that we are confident works in all cases.

janicduplessis · 2017-02-14T21:00:59Z

...id/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutAnimationController.java

@@ -65,6 +65,10 @@ public void reset() {
    mShouldAnimateLayout = false;
  }

+  public boolean shouldAnimateLayoutChildrenInView(View parentView) {


Can you use shouldAnimateLayout instead of adding this new method?

…utAnimationController.

vinceyuan · 2017-02-15T02:57:54Z

@janicduplessis Right. shouldAnimateLayoutChildrenInView is not necessary. I have removed it. Thanks!

Let me think about the ideal solution you mentioned.

f0rr0 · 2017-03-07T13:10:48Z

@janicduplessis @vinceyuan Is this good to go? Currently, removing items with Layout Animation throws on Android :/

vinceyuan · 2017-03-09T07:56:19Z

@f0rr0 My PR solved the issue. But janicduplessis doesn't think it is good enough. I am trying to find time to work on it.

Antoine-C · 2017-05-19T06:31:34Z

Hi
Any update on this ?
Thank's

facebook-github-bot · 2017-07-30T17:02:23Z

@vinceyuan 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.

hramos · 2017-07-31T19:15:05Z

@vinceyuan thanks for your contribution! After reviewing the comments here, it looks like this PR is not going to be accepted in its current state. I'll set it back to closed.

jpshelley · 2018-03-06T05:10:20Z

@vinceyuan @janicduplessis @hramos what is keeping this PR from being merged? Still a bit confused by that. Would love to help in anyway I can. This issue has existed on Android for a long time now, and I think it is also in react-native-windows, so learning how to fix it here will help there.

hramos · 2018-03-06T17:05:46Z

@jpshelley the PR has been closed for a while, which is a huge blocker for getting it merged in. If you're interested in making this change, I suggest going back through the thread and understanding why the reviewers pushed back on the change. If I recall correctly, I closed this PR for two reasons: reviewers rejected the changes and the PR laid dormant without activity for months afterwards.

@janicduplessis

Fixes issue facebook#11828 that causes layout animations for removed views to remove some adjacent views as well. This happens because the animated views are still present in the ViewGroup, which throws off subsequent operations that rely on view indices having updated. This issue was addressed in facebook#11962, which was closed in favour of a more reliable solution that addresses the issue globally since it’s difficult to account for animated views everywhere. @janicduplessis recommended[0] handling the issue through ViewManager. Since API 11, Android provides `ViewGroup#startViewTransition(View)` that can be used to keep child views visible even if they have been removed from the group. ViewGroup keeps an array of these views, which is only used for drawing. Methods such as `ViewGroup#getChildCount()` and `ViewGroup#getChildAt(int)` will ignore them. I believe relying on these framework methods within ViewManager is the most reliable way to solve this issue because it also works if callers ignore ViewManager and reach into the native view indices and counts directly. [0]: facebook#11962 (review)

Summary: Fixes issue #11828 that causes layout animations for removed views to remove some adjacent views as well. This happens because the animated views are still present in the ViewGroup, which throws off subsequent operations that rely on view indices having updated. This issue was addressed in #11962, which was closed in favour of a more reliable solution that addresses the issue globally since it’s difficult to account for animated views everywhere. janicduplessis [recommended][0] handling the issue through ViewManager. Since API 11, Android provides `ViewGroup#startViewTransition(View)` that can be used to keep child views visible even if they have been removed from the group. ViewGroup keeps an array of these views, which is only used for drawing. Methods such as `ViewGroup#getChildCount()` and `ViewGroup#getChildAt(int)` will ignore them. I believe relying on these framework methods within ViewManager is the most reliable way to solve this issue because it also works if callers ignore ViewManager and reach into the native view indices and counts directly. [0]: #11962 (review) I wrote a minimal test app that you can find here: <https://gist.github.com/lnikkila/87f3825442a5773f17ead433a810d53f> The expected result is that the red and green squares disappear, a blue one appears, and the black one stays in place. iOS has this behaviour, but Android removes the black square as well. We can see the bug with some breakpoint logging. Without LayoutAnimation: ``` NativeViewHierarchyOptimizer: Removing node from parent with tag 2 at index 0 NativeViewHierarchyOptimizer: Removing node from parent with tag 4 at index 1 NativeViewHierarchyManager: Removing indices [0] with tags [2] RootViewManager: Removing child view at index 0 with tag 2 NativeViewHierarchyManager: Removing indices [1] with tags [4] RootViewManager: Removing child view at index 1 with tag 4 ``` With LayoutAnimation tag 3 gets removed when it shouldn’t be: ``` NativeViewHierarchyOptimizer: Removing node from parent with tag 2 at index 0 NativeViewHierarchyOptimizer: Removing node from parent with tag 4 at index 1 NativeViewHierarchyManager: Removing indices [0] with tags [2] NativeViewHierarchyManager: Removing indices [1] with tags [4] -> RootViewManager: Removing child view at index 1 with tag 3 RootViewManager: Removing child view at index 2 with tag 4 (Animation listener kicks in here) RootViewManager: Removing child view at index 1 with tag 2 ``` Here are some GIFs to compare, click to expand: <details> <summary>Current master (iOS vs Android)</summary> <img src="https://user-images.githubusercontent.com/1291143/38695083-fbc29cd4-3e93-11e8-9150-9b8ea75b87aa.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38695108-06eb73a6-3e94-11e8-867a-b95d7f926ccd.gif" height="400" /> </details> <details> <summary>With this patch (iOS vs Android, fixed)</summary> <img src="https://user-images.githubusercontent.com/1291143/38695083-fbc29cd4-3e93-11e8-9150-9b8ea75b87aa.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38695137-1090f782-3e94-11e8-94c8-ce33a5d7ebdb.gif" height="400" /> </details> Previously addressed in #11962, which wasn’t merged. Tangentially related to my other LayoutAnimation PR #18651. No documentation changes needed. [ANDROID] [BUGFIX] [LayoutAnimation] - Removal LayoutAnimations no longer remove adjacent views as well in certain cases. Closes #18830 Reviewed By: achen1 Differential Revision: D7612904 Pulled By: mdvacca fbshipit-source-id: a04cf47ab80e0e813fa043125b1f907e212b1ad4

Summary: Fixes issue facebook#11828 that causes layout animations for removed views to remove some adjacent views as well. This happens because the animated views are still present in the ViewGroup, which throws off subsequent operations that rely on view indices having updated. This issue was addressed in facebook#11962, which was closed in favour of a more reliable solution that addresses the issue globally since it’s difficult to account for animated views everywhere. janicduplessis [recommended][0] handling the issue through ViewManager. Since API 11, Android provides `ViewGroup#startViewTransition(View)` that can be used to keep child views visible even if they have been removed from the group. ViewGroup keeps an array of these views, which is only used for drawing. Methods such as `ViewGroup#getChildCount()` and `ViewGroup#getChildAt(int)` will ignore them. I believe relying on these framework methods within ViewManager is the most reliable way to solve this issue because it also works if callers ignore ViewManager and reach into the native view indices and counts directly. [0]: facebook#11962 (review) I wrote a minimal test app that you can find here: <https://gist.github.com/lnikkila/87f3825442a5773f17ead433a810d53f> The expected result is that the red and green squares disappear, a blue one appears, and the black one stays in place. iOS has this behaviour, but Android removes the black square as well. We can see the bug with some breakpoint logging. Without LayoutAnimation: ``` NativeViewHierarchyOptimizer: Removing node from parent with tag 2 at index 0 NativeViewHierarchyOptimizer: Removing node from parent with tag 4 at index 1 NativeViewHierarchyManager: Removing indices [0] with tags [2] RootViewManager: Removing child view at index 0 with tag 2 NativeViewHierarchyManager: Removing indices [1] with tags [4] RootViewManager: Removing child view at index 1 with tag 4 ``` With LayoutAnimation tag 3 gets removed when it shouldn’t be: ``` NativeViewHierarchyOptimizer: Removing node from parent with tag 2 at index 0 NativeViewHierarchyOptimizer: Removing node from parent with tag 4 at index 1 NativeViewHierarchyManager: Removing indices [0] with tags [2] NativeViewHierarchyManager: Removing indices [1] with tags [4] -> RootViewManager: Removing child view at index 1 with tag 3 RootViewManager: Removing child view at index 2 with tag 4 (Animation listener kicks in here) RootViewManager: Removing child view at index 1 with tag 2 ``` Here are some GIFs to compare, click to expand: <details> <summary>Current master (iOS vs Android)</summary> <img src="https://user-images.githubusercontent.com/1291143/38695083-fbc29cd4-3e93-11e8-9150-9b8ea75b87aa.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38695108-06eb73a6-3e94-11e8-867a-b95d7f926ccd.gif" height="400" /> </details> <details> <summary>With this patch (iOS vs Android, fixed)</summary> <img src="https://user-images.githubusercontent.com/1291143/38695083-fbc29cd4-3e93-11e8-9150-9b8ea75b87aa.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38695137-1090f782-3e94-11e8-94c8-ce33a5d7ebdb.gif" height="400" /> </details> Previously addressed in facebook#11962, which wasn’t merged. Tangentially related to my other LayoutAnimation PR facebook#18651. No documentation changes needed. [ANDROID] [BUGFIX] [LayoutAnimation] - Removal LayoutAnimations no longer remove adjacent views as well in certain cases. Closes facebook#18830 Reviewed By: achen1 Differential Revision: D7612904 Pulled By: mdvacca fbshipit-source-id: a04cf47ab80e0e813fa043125b1f907e212b1ad4

Fix issue in removing items with LayoutAnimation. When LayoutAnimatio…

6632bf6

…n is enabled, indicesToRemove are incorrect, which makes adjacent views disappear.

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jan 18, 2017

vinceyuan mentioned this pull request Jan 18, 2017

Removing items with LayoutAnimation causes adjacent views/texts to disappear on Android #11828

Closed

janicduplessis suggested changes Feb 14, 2017

View reviewed changes

Remove unnecessary method shouldAnimateLayoutChildrenInView from Layo…

49731e9

…utAnimationController.

at-tran mentioned this pull request Feb 18, 2017

Delete animation bug when using "Clear completed" at-tran/todo-mvc-re-natal#16

Open

vjeranc mentioned this pull request Mar 30, 2017

LayoutAnimation easeInEaseOut causes children of top view to disappear or duplicate #13207

Closed

st0ffern mentioned this pull request May 17, 2017

android crash #13984

Closed

shergin added the Android label May 25, 2017

hramos added the Icebox label Jul 20, 2017

hramos closed this Jul 20, 2017

hramos reopened this Jul 21, 2017

hramos removed the Icebox label Jul 21, 2017

hramos closed this Jul 31, 2017

lnikkila mentioned this pull request Apr 12, 2018

Fix view indices with Android LayoutAnimation #18830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue in removing items with LayoutAnimation on Android. When LayoutAnimation is enabled, indicesToRemove are incorrect, which makes adjacent views disappear. #11962

Fix issue in removing items with LayoutAnimation on Android. When LayoutAnimation is enabled, indicesToRemove are incorrect, which makes adjacent views disappear. #11962

vinceyuan commented Jan 18, 2017 •

edited

Loading

facebook-github-bot commented Jan 18, 2017

vinceyuan commented Jan 18, 2017

vinceyuan commented Jan 24, 2017

janicduplessis commented Jan 24, 2017

janicduplessis commented Feb 2, 2017

vinceyuan commented Feb 3, 2017

freakinruben commented Feb 4, 2017

vinceyuan commented Feb 4, 2017

vinceyuan commented Feb 11, 2017

janicduplessis left a comment •

edited

Loading

janicduplessis Feb 14, 2017

vinceyuan commented Feb 15, 2017

f0rr0 commented Mar 7, 2017

vinceyuan commented Mar 9, 2017

Antoine-C commented May 19, 2017

facebook-github-bot commented Jul 30, 2017

hramos commented Jul 31, 2017

jpshelley commented Mar 6, 2018

hramos commented Mar 6, 2018

Fix issue in removing items with LayoutAnimation on Android. When LayoutAnimation is enabled, indicesToRemove are incorrect, which makes adjacent views disappear. #11962

Fix issue in removing items with LayoutAnimation on Android. When LayoutAnimation is enabled, indicesToRemove are incorrect, which makes adjacent views disappear. #11962

Conversation

vinceyuan commented Jan 18, 2017 • edited Loading

facebook-github-bot commented Jan 18, 2017

vinceyuan commented Jan 18, 2017

vinceyuan commented Jan 24, 2017

janicduplessis commented Jan 24, 2017

janicduplessis commented Feb 2, 2017

vinceyuan commented Feb 3, 2017

freakinruben commented Feb 4, 2017

vinceyuan commented Feb 4, 2017

vinceyuan commented Feb 11, 2017

janicduplessis left a comment • edited Loading

Choose a reason for hiding this comment

janicduplessis Feb 14, 2017

Choose a reason for hiding this comment

vinceyuan commented Feb 15, 2017

f0rr0 commented Mar 7, 2017

vinceyuan commented Mar 9, 2017

Antoine-C commented May 19, 2017

facebook-github-bot commented Jul 30, 2017

hramos commented Jul 31, 2017

jpshelley commented Mar 6, 2018

hramos commented Mar 6, 2018

vinceyuan commented Jan 18, 2017 •

edited

Loading

janicduplessis left a comment •

edited

Loading