Uh oh!
There was an error while loading. Please reload this page.
Fix issue in removing items with LayoutAnimation on Android. When LayoutAnimation is enabled, indicesToRemove are incorrect, which makes adjacent views disappear. - #11962
Conversation
…n is enabled, indicesToRemove are incorrect, which makes adjacent views disappear.
facebook-github-bot
commented
Jan 18, 2017
By analyzing the blame information on this pull request, we identified @ASCE1885 and @janicduplessis to be potential reviewers. |
vinceyuan
commented
Jan 18, 2017
@janicduplessis Please take a look. Thanks. |
vinceyuan
commented
Jan 24, 2017
Ping @janicduplessis |
janicduplessis
commented
Jan 24, 2017
Thanks for the PR! I'll have a look this week ;) |
janicduplessis
commented
Feb 2, 2017
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
commented
Feb 3, 2017
@janicduplessis 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 Probably this fix is not perfect. But with this fix, everything works well with/without LayoutAnimation. 😸 |
freakinruben
commented
Feb 4, 2017
Does this solve #9549 as well? |
vinceyuan
commented
Feb 4, 2017
@freakinruben Probably not. This PR fixes an issue in LayoutAnimation on Android. #9549 doesn't seem to be animation related. |
vinceyuan
commented
Feb 11, 2017
@janicduplessis Any comment about this PR? Is it good? |
There was a problem hiding this comment.
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.
| mShouldAnimateLayout = false; | ||
| } | ||
| public boolean shouldAnimateLayoutChildrenInView(View parentView) { |
There was a problem hiding this comment.
Can you use shouldAnimateLayout instead of adding this new method?
…utAnimationController.
vinceyuan
commented
Feb 15, 2017
@janicduplessis Right. shouldAnimateLayoutChildrenInView is not necessary. I have removed it. Thanks! Let me think about the ideal solution you mentioned. |
f0rr0
commented
Mar 7, 2017
@janicduplessis@vinceyuan Is this good to go? Currently, removing items with Layout Animation throws on Android :/ |
vinceyuan
commented
Mar 9, 2017
@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
commented
May 19, 2017
Hi |
facebook-github-bot
commented
Jul 30, 2017
@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
commented
Jul 31, 2017
@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
commented
Mar 6, 2018
@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
commented
Mar 6, 2018
@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. |
Fixes issue react#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 react#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]: react#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><b>Current master (iOS vs Android)</b></summary> <p></p> <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><p></p> <details> <summary><b>With this patch (iOS vs Android, fixed)</b></summary> <p></p> <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><p></p> 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 react#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 react#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]: react#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><b>Current master (iOS vs Android)</b></summary> <p></p> <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><p></p> <details> <summary><b>With this patch (iOS vs Android, fixed)</b></summary> <p></p> <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><p></p> Previously addressed in react#11962, which wasn’t merged. Tangentially related to my other LayoutAnimation PR react#18651. No documentation changes needed. [ANDROID] [BUGFIX] [LayoutAnimation] - Removal LayoutAnimations no longer remove adjacent views as well in certain cases. Closesreact#18830 Reviewed By: achen1 Differential Revision: D7612904 Pulled By: mdvacca fbshipit-source-id: a04cf47ab80e0e813fa043125b1f907e212b1ad4
Fix issue #11828 Removing items with LayoutAnimation causes adjacent views/texts to disappear on Android
I explained my solution in #11828 (comment)