Skip to content

Commit

Permalink
Fixed removeClippedSubviews
Browse files Browse the repository at this point in the history
Summary:
The `removeClippedSubviews` feature works by umounting views from the hierarchy if they move outside the bounds of their parent.

This was previously restricted to clipping views which had `overflow: hidden`, since we cannot efficiently check whether the subviews of a view go outside its bounds, and so clipping a view that has potentially overflowing children becomes an expensive recursive operation.

The problem with this is that `overflow: visible` is the default, and it's not well documented nor easy to tell that `removeClippedSubviews` has been set up correctly (i.e. with all children having `overflow: hidden`).

When I checked, I found that `removeClippedSubviews` was not working on any of the examples in UIExplorer, nor in several of our internal apps, because the views inside the ListView has `overflow: visible`. This was probably caused by an infra change at some point, but I'm not sure how long it's been broken.

It's vanishingly unlikely that anyone would ever deliberately want subviews to overflow their bounds in this scenario, so I've updated the logic to simply ignore the `overflow` property and assume that views should be clipped if you are using the `removeClippedSubviews` property on the parent.

Cons / Breaking changes: in some rare circumstances, a view might get clipped prematurely if its parent is outside the scrollview bounds, but it itself is inside. This doesn't occur in practice in any of our products, and could be worked around with additional wrapper views if it did.

Pros: removeClippedSubviews is now much easier to use, and much more likely to work as intended, so most list-based apps should see a performance improvement.

Reviewed By: javache

Differential Revision: D3385316

fbshipit-source-id: 1c0064a4c21340a971ba80d794062a356ae6cfb3
  • Loading branch information
nicklockwood authored and Facebook Github Bot 6 committed Jun 6, 2016
1 parent 8731494 commit 1048e5d
Showing 1 changed file with 14 additions and 35 deletions.
49 changes: 14 additions & 35 deletions React/Views/RCTView.m
Expand Up @@ -312,43 +312,24 @@ - (void)remountSubview:(UIView *)view

- (void)mountOrUnmountSubview:(UIView *)view withClipRect:(CGRect)clipRect relativeToView:(UIView *)clipView
{
if (view.clipsToBounds) {

// View has cliping enabled, so we can easily test if it is partially
// or completely within the clipRect, and mount or unmount it accordingly

if (!CGRectIsEmpty(CGRectIntersection(clipRect, view.frame))) {

// View is at least partially visible, so remount it if unmounted
if (view.superview == nil) {
[self remountSubview:view];
}

// Then test its subviews
if (CGRectContainsRect(clipRect, view.frame)) {
[view react_remountAllSubviews];
} else {
[view react_updateClippedSubviewsWithClipRect:clipRect relativeToView:clipView];
}

} else if (view.superview) {

// View is completely outside the clipRect, so unmount it
[view removeFromSuperview];
}

} else {

// View has clipping disabled, so there's no way to tell if it has
// any visible subviews without an expensive recursive test, so we'll
// just add it.
if (!CGRectIsEmpty(CGRectIntersection(clipRect, view.frame))) {

// View is at least partially visible, so remount it if unmounted
if (view.superview == nil) {
[self remountSubview:view];
}

// Check if subviews need to be mounted/unmounted
[view react_updateClippedSubviewsWithClipRect:clipRect relativeToView:clipView];
// Then test its subviews
if (CGRectContainsRect(clipRect, view.frame)) {
[view react_remountAllSubviews];
} else {
[view react_updateClippedSubviewsWithClipRect:clipRect relativeToView:clipView];
}

} else if (view.superview) {

// View is completely outside the clipRect, so unmount it
[view removeFromSuperview];
}
}

Expand All @@ -375,10 +356,8 @@ - (void)react_updateClippedSubviewsWithClipRect:(CGRect)clipRect relativeToView:

// Convert clipping rect to local coordinates
clipRect = [clipView convertRect:clipRect toView:self];

This comment has been minimized.

Copy link
@zhangwx8973

zhangwx8973 Jul 2, 2019

when i set removeClippedSubviews to true,sometimes the children cannot show.i found the reason is the clipRect's size is bigger than self,that lead clipRect's size to zero,then _reactSubviews can not draw normaly.

clipRect = CGRectIntersection(clipRect, self.bounds);
clipView = self;
if (self.clipsToBounds) {
clipRect = CGRectIntersection(clipRect, self.bounds);
}

// Mount / unmount views
for (UIView *view in _reactSubviews) {
Expand Down

7 comments on commit 1048e5d

@olofd
Copy link

@olofd olofd commented on 1048e5d Jul 1, 2016

Choose a reason for hiding this comment

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

This caused some issues in our app, where we had some library-code that had removeClippedSubviews=true. I guess this had never worked before. But after 0.29.0-rc.0 subviews started getting removed. And it worked ok. But for som reason the subviews would not show up again until they where touched/swiped over, even if they were in viewport. We switched off the functionality and everything now works as before. Just wanted to let you know.

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

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

@olofd Do you have a demo? Would be great if you could open a new issue demonstrating this.

@ranjitpandit
Copy link

Choose a reason for hiding this comment

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

I'm having the same issue. The first view gets clipped until it's touched. Turning off removeClippedSubviews makes it work again.

@nihgwu
Copy link
Contributor

@nihgwu nihgwu commented on 1048e5d Jul 7, 2016

Choose a reason for hiding this comment

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

I'm not quite sure if we are facing the some issue #8607

@nihgwu
Copy link
Contributor

@nihgwu nihgwu commented on 1048e5d Jul 7, 2016

Choose a reason for hiding this comment

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

I'm using the removeClippedSubviews={true} in ViewPager too, I'll see if it would be solved after turning it off 😃

@nihgwu
Copy link
Contributor

@nihgwu nihgwu commented on 1048e5d Jul 7, 2016

Choose a reason for hiding this comment

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

Turning off removeClippedSubviews doesn't solve my issue #8607 , only solve the problem described in the comment.
I've reverted this commit and the issue #8607 went away

@ssssssssssss
Copy link

Choose a reason for hiding this comment

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

What about the scrollview with pageEnable=true? Does this prop supposed to support removing the offscreen page?

Please sign in to comment.