fix: zIndex for exiting animations#9541
Conversation
| react_native_assert(index >= 0 && "index must be non-negative"); | ||
| int remainingNonExitingChildrenToCheck = index; | ||
| int exitingCount = 0; | ||
| for (std::size_t i = 0; i < children.size(); i++) { |
There was a problem hiding this comment.
Notice: this may affect the performance. RN flattens views so the children size could be large.
There was a problem hiding this comment.
I think we could store the number of animated children on the parent, this way we could short-circuit this method to return immediately if there are none.
We should also expect that the children indices in mutations are increasing within one transaction, this way we could reuse the calculation up to a certain point (if the assumption is wrong at any point we should just recalculate from scratch - this could happen if multiple transactions are combined into one on Android)
| childTags += "(exiting)"; | ||
| } | ||
| } | ||
| LOG(WARNING) << "Remove mutation index mismatch: expected tag " << mutation.oldChildShadowView.tag |
There was a problem hiding this comment.
Left the log on purpose. It helps with debugging if the assertion below fails. Logs are displayed only on iOS. On Android, RN uses some custom solution to log glog's sink.
There was a problem hiding this comment.
I think SYSLOG works on both iOS and Android. Maybe we could also extract this error logic to a helper as it takes up much space here
| if (it == lightNodes_.end()) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
With the assert, we don't really need this check. It could be useful for Release mode, but I think those defensive checks in here are not helping us in the long run
| auto lock = std::unique_lock<std::recursive_mutex>(mutex); | ||
| const PropsParserContext propsParserContext{surfaceId, *contextContainer_}; | ||
| ShadowViewMutationList filteredMutations; | ||
| ShadowViewMutationList outputMutations; |
There was a problem hiding this comment.
I think the renames here are mostly a good idea, though they make the review less convenient as many lines are affected for unrelated reasons.
| childTags += "(exiting)"; | ||
| } | ||
| } | ||
| LOG(WARNING) << "Remove mutation index mismatch: expected tag " << mutation.oldChildShadowView.tag |
There was a problem hiding this comment.
I think SYSLOG works on both iOS and Android. Maybe we could also extract this error logic to a helper as it takes up much space here
| react_native_assert(index >= 0 && "index must be non-negative"); | ||
| int remainingNonExitingChildrenToCheck = index; | ||
| int exitingCount = 0; | ||
| for (std::size_t i = 0; i < children.size(); i++) { |
There was a problem hiding this comment.
I think we could store the number of animated children on the parent, this way we could short-circuit this method to return immediately if there are none.
We should also expect that the children indices in mutations are increasing within one transaction, this way we could reuse the calculation up to a certain point (if the assumption is wrong at any point we should just recalculate from scratch - this could happen if multiple transactions are combined into one on Android)
2 participants
Summary
This MR changes the exiting animations' underlying logic. Currently, Reanimated re-inserts the exiting view component at the end of the children array, which breaks the z-order. The new approach doesn't send remove and delete mutations immediately and translates mutation indices instead.
Test plan
Attachments
Before iOS
BB.before.ios.mov
After iOS
BB.after.ios.mov
After Android
BB.after.android.mov
Unrelated Android Issue
(It exists on the main branch)
