Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling removal of transitioning views #47634

Closed
wants to merge 14 commits into from
+29 −4
Closed

Fix handling removal of transitioning views #47634

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0c310cb
Prevent removal from parent when transitioning
kkafar Nov 15, 2024
ca9a22d
Track children removed while transitioning
kkafar Nov 23, 2024
00e0e52
Merge main && refactor the code to comply with recent changes
kkafar Nov 23, 2024
150c15e
Cleanup
kkafar Nov 24, 2024
48b90ad
Cleanup 2
kkafar Nov 24, 2024
6e15352
Cleanup 3
kkafar Nov 24, 2024
4d6b796
Prevent bug when setting removeClippedSubviews to `false` after `endV…
kkafar Nov 24, 2024
93b0a35
Add missing import
kkafar Nov 24, 2024
c3da8d0
Allocate hashset only when necessary
kkafar Nov 26, 2024
09b7c62
Merge branch 'main' into @kkafar/support-for-view-transition
kkafar Nov 26, 2024
eb4e4fd
Get rid of redundant null check
kkafar Nov 27, 2024
c14e026
Review part one
kkafar Nov 27, 2024
a7c7af3
Just clear the hash set, do not release it
kkafar Nov 27, 2024
f54164d
Support cases where not all children are transitioning at once
kkafar Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions ...tive/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactClippingViewManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,6 @@ public abstract class ReactClippingViewManager<T : ReactViewGroup> : ViewGroupMa
if (removeClippedSubviews) {
val child = getChildAt(parent, index)
if (child != null) {
if (child.parent != null) {
parent.removeView(child)
}
Comment on lines -65 to -67
Copy link
Member

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?

Copy link
Contributor Author

@kkafar kkafar Nov 26, 2024 •
edited
Loading

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 handleRemoveView being called to perform side effects regarding child drawing order. Implementation of ReactViewgroup.removeView took care of handling these sideeffects and detaching child view from parent.

On the other hand ReactViewGroup.removeViewWithSubviewClippingEnabled had also two responsibilities:

  1. detach child from parent if it has not been detached yet (notice the guard here!)
  2. remove child from mAllChildren array, 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 of handleRemoveView were performed and then you called parent.removeViewWithSubviewClippingEnabled(child) to remove child from mAllChildren array, relying on the guard from (1), which prevented Android crash potentially caused by removing twice the same child view.

After 0b22b95 side effects of handleRemoveView have been moved to onViewRemoved which is called on any attempt of child removal by the Android framework. Therefore we can remove the call to parent.removeView as all required side effects will be performed by the call to parent.removeViewWithSubviewClippingEnabled(child) - drawing order code, removal from mAllChildren and child detachment. I've left the check for nullish child, as I couldn't determine what was the logic behind it.

parent.removeViewWithSubviewClippingEnabled(child)
}
} else {
Expand Down
36 changes: 34 additions & 2 deletions ...react-native/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using Set<Integer> (holding only the view tag) instead of Set<View> to avoid any retain issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using androidx.collection.MutableIntSet instead to avoid Integer boxing.

Copy link
Contributor Author

@kkafar kkafar Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I was not aware of this API! Hover it seems that it has been added few months ago in androidx.collection.MutableIntSet 1.4.0, while my development project (using recent react-native 0.76) uses version 1.1.0. I'm not sure where would I need to bump / add this dependency in RN to make use of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Expand Down Expand Up @@ -405,6 +410,26 @@ 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;
mChildrenRemovedWhileTransitioning = null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this account for multiple views/transitions? Seems like we'd need to remove the id(s) instead, and then:
mIsTransitioning = mChildrenRemovedWhileTransitioning.isEmpty();

Also probably worth keeping mChildrenRemovedWhileTransitioning once we create it, we just don't want to create on init, it in cases where transitions aren't used at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed nulling the hashset to clearing it here: a7c7af3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdn120 I'm not sure though, what you mean by this:

Does this account for multiple views/transitions? Seems like we'd need to remove the id(s) instead [...]

Could you elaborate here?

The ReactViewGroup instance is responsible only of tracking its transitioning state & children. I wrote it so to align it with Android parent.startViewTransition(child) behaviour - it modifies only the parent behaviour with respect to this single child. The child is unaware of any changes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just at a basic level, if you have a timeline of:

startViewTransition(viewA)
startViewTransition(viewB)
endViewTransition(viewA)

At this point you would clear mIsTransitioning and mChildrenRemovedWhileTransitioning, so wouldn't viewB be out of sync until its transition ends?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was OOO by the end of last week. Back here now 👋🏻

I think I still don't understand the issue here.

Android docs explicitly state that a call to startViewTransition should be accompanied by call to endViewTransition. It is left up to the programmer to ensure this.

If you - as a API user - fail to fulfil this contract you'll get inconsistent (possibly buggy) behaviour and I believe there is not much we can do on React Native end to prevent that.

Regarding the example:

this.startViewTransition(viewA)
this.startViewTransition(viewB)
this.endViewTransition(viewA)

...

Oh, it just got to me. Yeah, I think you're right and that is a very good catch. Thanks. I'll update the code in a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should keep track of which children are transitioning and clear the state only if there are no more such children.

Copy link
Contributor Author

@kkafar kkafar Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdn120 I am thinking about various approaches, trying to avoid allocating additional hash set as much as possible. I would need it to store all the children for which startViewTransition has been called and then remove them from the set in endViewTransition. The working approach would look somehow like that:

  public boolean isChildTransitioning(View child) {
    return mTransitioningChildren != null && mTransitioningChildren.contains(child.getid());
  }

  public void onViewRemoved(View child) {
    ...

    if (isChildTransitioning(child)) {
      // This call would allocate
      ensureChildrenRemovedWhileTransitioning.add(child.getId());
    }

    ...
  }

  public void startViewTransition(View view) {
    super.startViewTransition(view);
    ensureTransitioningChildren().add(view.getId());
  }

  public void endViewTransition(View view) {
    super.endViewTransition(view);
    ensureTransitioningChildren().remove(view.getId());
    if (mChildrenRemovedWhileTransitioning != null) {
      mChildrenRemovedWhileTransitioning.remove(view.getId());
    }
  }

However, this requires 2 hash sets...

Only way to improve I see here is to rely in onViewRemoved on the fact, that if children is transitioning and it has been removed then its parent won't be nulled out.
Hence we could check it like this:

public void onViewRemoved(View child) {
    ...
    
    if (child.getParent() != null) {
        ensureChildrenRemovedWhileTransitioning.add(child.getId());
    }

    ...
}

Looking at Android code (link) this would be reliable, but we would be relaying on implementation detail. I feel like this behaviour is implicitly suggested in startViewTransition & endViewTransition docs, by stating that these remove child from parent state but noting about removing parent from child state, however this is not explicit.

Even if I'm overlooking some case this still would solve the initial issue, so we should be fine on this front.

Another possibility is to store the information on whether the view is transitioning or not directly on the child view itself, by using a tag as you suggested.

Lemme know which behavior you would agree for.

}

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;
Expand Down Expand Up @@ -564,6 +589,11 @@ public void onViewRemoved(View child) {
} else {
setChildrenDrawingOrderEnabled(false);
}

if (mIsTransitioning) {
ensureChildrenRemovedWhileTransitioning().add(child.getId());
}

super.onViewRemoved(child);
}

Expand Down Expand Up @@ -679,11 +709,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;
Expand Down Expand Up @@ -712,7 +743,8 @@ public void run() {
*/
private boolean isViewClipped(View view) {
ViewParent parent = view.getParent();
if (parent == null) {

if (parent == null || (mChildrenRemovedWhileTransitioning != null && mChildrenRemovedWhileTransitioning.contains(view.getId()))) {
return true;
} else {
Assertions.assertCondition(parent == this);
Expand Down
Loading