Handle layout updates during LayoutAnimation animations on Android #18651
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lnikkila
changed the title
facebook-github-bot
added
the
CLA Signed
|
Android test failures on CircleCI seem to be coming from SoLoader, I don’t think this affects it. |
lnikkila
force-pushed
the
lnikkila/android-layoutanimation-update
branch
from
ebb0e80
to
7f854a2
Compare
On Android, LayoutAnimation directly updates the layout since a generic scaling animation is more difficult to implement. This causes a problem if the layout is updated during an animation, as the previous layout is stored with the animation and is not updated. As a result the view gets the old layout instead once the animation completes. This commit fixes this issue by storing the layout handling animations while those animations are active, and updating the animations on the fly if one of the views receives a new layout. The resulting behaviour mirrors what iOS currently does.
The animated view should become square once the animation finishes.
lnikkila
force-pushed
the
lnikkila/android-layoutanimation-update
branch
from
7f854a2
to
629e496
Compare
|
Hi! I noticed you requested a review some time ago, would you still be interested in reviewing this? |
janicduplessis
approved these changes
Apr 12, 2018
|
@facebook-github-bot shipit |
facebook-github-bot
added
the
Import Started
facebook-github-bot
approved these changes
Apr 12, 2018
There was a problem hiding this comment.
@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 16, 2018
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
bunnyc1986
pushed a commit
to bunnyc1986/react-native
that referenced
this pull request
May 11, 2018
Summary: On Android, LayoutAnimation directly updates the layout since a generic scaling animation is more difficult to implement. This causes a problem if the layout is updated during an animation, as the previous layout is stored with the animation and is not updated. As a result the view gets the old layout instead once the animation completes. This commit fixes this issue by storing the layout handling animations while those animations are active, and updating the animations on the fly if one of the views receives a new layout. The resulting behaviour mirrors what iOS currently does. This bug has real world consequences, for example if a LayoutAnimation happens right after a VirtualizedList has mounted, it’s possible that some list rows are mounted while the animation is active, making the list content view bigger. If the content view is being animated, the new size will not take effect and it becomes impossible to scroll to the end of the list. I wrote a minimal test case to verify the bug, which I’ve also added to RNTester. You can find the standalone app here: <https://gist.github.com/lnikkila/18096c15b2fb99b232795ef59f8fb0cd> The app creates a 100x300 view that gets animated to 200x300 using LayoutAnimation. In the middle of that animation, the view’s dimensions are updated to 300x300. The expected result (which is currently exhibited by iOS) is that the view’s dimensions after the animation would be 300x300. On Android the view keeps the 200x300 dimensions since the animation overrides the layout update. The test app could probably be turned into an integration test by measuring the view through UIManager after the animation, however I don’t have time to do that right now... 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/38191325-f1aeb3d4-3670-11e8-8aca-14e7b24e2946.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38191337-f643fd8c-3670-11e8-9aac-531a32cc0a67.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/38191325-f1aeb3d4-3670-11e8-8aca-14e7b24e2946.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38191355-07f6e972-3671-11e8-8ad2-130d06d0d64d.gif" height="400" /> </details><p></p> No documentation changes needed. [ANDROID] [BUGFIX] [LayoutAnimation] - View layout is updated correctly during an ongoing LayoutAnimation, mirroring iOS behaviour. Closes facebook#18651 Differential Revision: D7604698 Pulled By: hramos fbshipit-source-id: 4d114682fd540419b7447e999910e05726f42b39
bunnyc1986
pushed a commit
to bunnyc1986/react-native
that referenced
this pull request
May 11, 2018
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><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 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
lnikkila
added a commit
to Boulevard/react-native
that referenced
this pull request
May 29, 2018
Summary: On Android, LayoutAnimation directly updates the layout since a generic scaling animation is more difficult to implement. This causes a problem if the layout is updated during an animation, as the previous layout is stored with the animation and is not updated. As a result the view gets the old layout instead once the animation completes. This commit fixes this issue by storing the layout handling animations while those animations are active, and updating the animations on the fly if one of the views receives a new layout. The resulting behaviour mirrors what iOS currently does. This bug has real world consequences, for example if a LayoutAnimation happens right after a VirtualizedList has mounted, it’s possible that some list rows are mounted while the animation is active, making the list content view bigger. If the content view is being animated, the new size will not take effect and it becomes impossible to scroll to the end of the list. I wrote a minimal test case to verify the bug, which I’ve also added to RNTester. You can find the standalone app here: <https://gist.github.com/lnikkila/18096c15b2fb99b232795ef59f8fb0cd> The app creates a 100x300 view that gets animated to 200x300 using LayoutAnimation. In the middle of that animation, the view’s dimensions are updated to 300x300. The expected result (which is currently exhibited by iOS) is that the view’s dimensions after the animation would be 300x300. On Android the view keeps the 200x300 dimensions since the animation overrides the layout update. The test app could probably be turned into an integration test by measuring the view through UIManager after the animation, however I don’t have time to do that right now... 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/38191325-f1aeb3d4-3670-11e8-8aca-14e7b24e2946.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38191337-f643fd8c-3670-11e8-9aac-531a32cc0a67.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/38191325-f1aeb3d4-3670-11e8-8aca-14e7b24e2946.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38191355-07f6e972-3671-11e8-8ad2-130d06d0d64d.gif" height="400" /> </details><p></p> No documentation changes needed. [ANDROID] [BUGFIX] [LayoutAnimation] - View layout is updated correctly during an ongoing LayoutAnimation, mirroring iOS behaviour. Closes facebook#18651 Differential Revision: D7604698 Pulled By: hramos fbshipit-source-id: 4d114682fd540419b7447e999910e05726f42b39
lnikkila
added a commit
to Boulevard/react-native
that referenced
this pull request
May 29, 2018
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><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 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
macdoum1
pushed a commit
to macdoum1/react-native
that referenced
this pull request
Jun 28, 2018
Summary: On Android, LayoutAnimation directly updates the layout since a generic scaling animation is more difficult to implement. This causes a problem if the layout is updated during an animation, as the previous layout is stored with the animation and is not updated. As a result the view gets the old layout instead once the animation completes. This commit fixes this issue by storing the layout handling animations while those animations are active, and updating the animations on the fly if one of the views receives a new layout. The resulting behaviour mirrors what iOS currently does. This bug has real world consequences, for example if a LayoutAnimation happens right after a VirtualizedList has mounted, it’s possible that some list rows are mounted while the animation is active, making the list content view bigger. If the content view is being animated, the new size will not take effect and it becomes impossible to scroll to the end of the list. I wrote a minimal test case to verify the bug, which I’ve also added to RNTester. You can find the standalone app here: <https://gist.github.com/lnikkila/18096c15b2fb99b232795ef59f8fb0cd> The app creates a 100x300 view that gets animated to 200x300 using LayoutAnimation. In the middle of that animation, the view’s dimensions are updated to 300x300. The expected result (which is currently exhibited by iOS) is that the view’s dimensions after the animation would be 300x300. On Android the view keeps the 200x300 dimensions since the animation overrides the layout update. The test app could probably be turned into an integration test by measuring the view through UIManager after the animation, however I don’t have time to do that right now... 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/38191325-f1aeb3d4-3670-11e8-8aca-14e7b24e2946.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38191337-f643fd8c-3670-11e8-9aac-531a32cc0a67.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/38191325-f1aeb3d4-3670-11e8-8aca-14e7b24e2946.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38191355-07f6e972-3671-11e8-8ad2-130d06d0d64d.gif" height="400" /> </details><p></p> No documentation changes needed. [ANDROID] [BUGFIX] [LayoutAnimation] - View layout is updated correctly during an ongoing LayoutAnimation, mirroring iOS behaviour. Closes facebook#18651 Differential Revision: D7604698 Pulled By: hramos fbshipit-source-id: 4d114682fd540419b7447e999910e05726f42b39
macdoum1
pushed a commit
to macdoum1/react-native
that referenced
this pull request
Jun 28, 2018
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><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 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
rozele
pushed a commit
to microsoft/react-native-windows
that referenced
this pull request
Aug 17, 2018
Summary: On Android, LayoutAnimation directly updates the layout since a generic scaling animation is more difficult to implement. This causes a problem if the layout is updated during an animation, as the previous layout is stored with the animation and is not updated. As a result the view gets the old layout instead once the animation completes. This commit fixes this issue by storing the layout handling animations while those animations are active, and updating the animations on the fly if one of the views receives a new layout. The resulting behaviour mirrors what iOS currently does. This bug has real world consequences, for example if a LayoutAnimation happens right after a VirtualizedList has mounted, it’s possible that some list rows are mounted while the animation is active, making the list content view bigger. If the content view is being animated, the new size will not take effect and it becomes impossible to scroll to the end of the list. I wrote a minimal test case to verify the bug, which I’ve also added to RNTester. You can find the standalone app here: <https://gist.github.com/lnikkila/18096c15b2fb99b232795ef59f8fb0cd> The app creates a 100x300 view that gets animated to 200x300 using LayoutAnimation. In the middle of that animation, the view’s dimensions are updated to 300x300. The expected result (which is currently exhibited by iOS) is that the view’s dimensions after the animation would be 300x300. On Android the view keeps the 200x300 dimensions since the animation overrides the layout update. The test app could probably be turned into an integration test by measuring the view through UIManager after the animation, however I don’t have time to do that right now... 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/38191325-f1aeb3d4-3670-11e8-8aca-14e7b24e2946.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38191337-f643fd8c-3670-11e8-9aac-531a32cc0a67.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/38191325-f1aeb3d4-3670-11e8-8aca-14e7b24e2946.gif" height="400" /><img src="https://user-images.githubusercontent.com/1291143/38191355-07f6e972-3671-11e8-8ad2-130d06d0d64d.gif" height="400" /> </details><p></p> No documentation changes needed. [ANDROID] [BUGFIX] [LayoutAnimation] - View layout is updated correctly during an ongoing LayoutAnimation, mirroring iOS behaviour. Closes facebook/react-native#18651 Differential Revision: D7604698 Pulled By: hramos fbshipit-source-id: 4d114682fd540419b7447e999910e05726f42b39
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Android, LayoutAnimation directly updates the layout since a generic
scaling animation is more difficult to implement. This causes a problem
if the layout is updated during an animation, as the previous layout is
stored with the animation and is not updated. As a result the view gets
the old layout instead once the animation completes.
This commit fixes this issue by storing the layout handling animations
while those animations are active, and updating the animations on the
fly if one of the views receives a new layout. The resulting behaviour
mirrors what iOS currently does.
This bug has real world consequences, for example if a LayoutAnimation
happens right after a VirtualizedList has mounted, it’s possible that
some list rows are mounted while the animation is active, making the
list content view bigger. If the content view is being animated, the
new size will not take effect and it becomes impossible to scroll to
the end of the list.
Test Plan
I wrote a minimal test case to verify the bug, which I’ve also added to
RNTester. You can find the standalone app here:
https://gist.github.com/lnikkila/18096c15b2fb99b232795ef59f8fb0cd
The app creates a 100x300 view that gets animated to 200x300 using
LayoutAnimation. In the middle of that animation, the view’s dimensions
are updated to 300x300.
The expected result (which is currently exhibited by iOS) is that the
view’s dimensions after the animation would be 300x300. On Android the
view keeps the 200x300 dimensions since the animation overrides the
layout update.
The test app could probably be turned into an integration test by
measuring the view through UIManager after the animation, however I
don’t have time to do that right now...
Here are some GIFs to compare, click to expand:
Current master (iOS vs Android)
With this patch (iOS vs Android, fixed)
Related PRs
No documentation changes needed.
Release Notes
[ANDROID] [BUGFIX] [LayoutAnimation] - View layout is updated correctly during an ongoing LayoutAnimation, mirroring iOS behaviour.