Fix handling removal of transitioning views #47634
+29
−4
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,9 @@ | |
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
kkafar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /** | ||
| * Backing for a React View. Has support for borders, but since borders aren't common, lazy | ||
| * initializes most of the storage needed for them. | ||
|
|
@@ -137,6 +140,8 @@ public void shutdown() { | |
| private @Nullable ViewGroupDrawingOrderHelper mDrawingOrderHelper; | ||
| private float mBackfaceOpacity; | ||
| private String mBackfaceVisibility; | ||
| private boolean mIsTransitioning = false; | ||
| private @Nullable Set<Integer> mChildrenRemovedWhileTransitioning = null; | ||
|
There was a problem hiding this comment. I'm using There was a problem hiding this comment. Consider using androidx.collection.MutableIntSet instead to avoid Integer boxing. There was a problem hiding this comment. Thanks, I was not aware of this API! Hover it seems that it has been added few months ago in There was a problem hiding this comment. Looks like this would need to be a new dependency in https://github.com/facebook/react-native/blob/main/packages/react-native/gradle/libs.versions.toml#L45 - let's add a comment to change this in the future.
kkafar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Creates a new `ReactViewGroup` instance. | ||
|
|
@@ -405,6 +410,28 @@ public void updateClippingRect() { | |
| updateClippingToRect(mClippingRect); | ||
| } | ||
|
|
||
| @Override | ||
| public void startViewTransition(View view) { | ||
| super.startViewTransition(view); | ||
| mIsTransitioning = true; | ||
| } | ||
|
|
||
| @Override | ||
| public void endViewTransition(View view) { | ||
| super.endViewTransition(view); | ||
| mIsTransitioning = false; | ||
| if (mChildrenRemovedWhileTransitioning != null) { | ||
| mChildrenRemovedWhileTransitioning = null; | ||
| } | ||
kkafar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| public Set<Integer> ensureChildrenRemovedWhileTransitioning() { | ||
kkafar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (mChildrenRemovedWhileTransitioning == null) { | ||
| mChildrenRemovedWhileTransitioning = new HashSet<>(); | ||
| } | ||
| return mChildrenRemovedWhileTransitioning; | ||
| } | ||
|
|
||
| private void updateClippingToRect(Rect clippingRect) { | ||
| Assertions.assertNotNull(mAllChildren); | ||
| int clippedSoFar = 0; | ||
|
|
@@ -564,6 +591,11 @@ public void onViewRemoved(View child) { | |
| } else { | ||
| setChildrenDrawingOrderEnabled(false); | ||
| } | ||
|
|
||
| if (mIsTransitioning) { | ||
| ensureChildrenRemovedWhileTransitioning().add(child.getId()); | ||
| } | ||
|
|
||
| super.onViewRemoved(child); | ||
| } | ||
|
|
||
|
|
@@ -679,11 +711,12 @@ public void run() { | |
|
|
||
| /*package*/ void removeViewWithSubviewClippingEnabled(View view) { | ||
| UiThreadUtil.assertOnUiThread(); | ||
| Assertions.assertCondition(mRemoveClippedSubviews); | ||
| Assertions.assertNotNull(mClippingRect); | ||
| View[] childArray = Assertions.assertNotNull(mAllChildren); | ||
|
|
||
| view.removeOnLayoutChangeListener(mChildrenLayoutChangeListener); | ||
|
|
||
kkafar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| int index = indexOfChildInAllChildren(view); | ||
| if (!isViewClipped(childArray[index])) { | ||
| int clippedSoFar = 0; | ||
|
|
@@ -712,7 +745,8 @@ public void run() { | |
| */ | ||
| private boolean isViewClipped(View view) { | ||
| ViewParent parent = view.getParent(); | ||
|
|
||
| if (parent == null || (mChildrenRemovedWhileTransitioning != null && mChildrenRemovedWhileTransitioning.contains(view.getId()))) { | ||
| return true; | ||
| } else { | ||
| Assertions.assertCondition(parent == this); | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is really interesting, and may explain a crash we're seeing. I wonder why we were previously removing this subview twice - potentially messing up some state in ReactViewGroup.
What's your reasoning for removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javache thanks for review. My logic was as follows:
before 0b22b95 you relied on
handleRemoveViewbeing called to perform side effects regarding child drawing order. Implementation ofReactViewgroup.removeViewtook care of handling these sideeffects and detaching child view from parent.On the other hand
ReactViewGroup.removeViewWithSubviewClippingEnabledhad also two responsibilities:mAllChildrenarray, which also retained children already detached due to clipping.Notice, that it had not performed
handleRemoveView.My theory is that in clipping view manager you had call to
parent.removeView(child)to ensure sideeffects ofhandleRemoveViewwere performed and then you calledparent.removeViewWithSubviewClippingEnabled(child)to remove child frommAllChildrenarray, relying on the guard from (1), which prevented Android crash potentially caused by removing twice the same child view.After 0b22b95 side effects of
handleRemoveViewhave been moved toonViewRemovedwhich is called on any attempt of child removal by the Android framework. Therefore we can remove the call toparent.removeViewas all required side effects will be performed by the call toparent.removeViewWithSubviewClippingEnabled(child)- drawing order code, removal frommAllChildrenand child detachment. I've left the check for nullish child, as I couldn't determine what was the logic behind it.