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.

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

Fixes removeClipSubviews check for offscreen rendering of ListViews #15669

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
@MattFoley
Contributor

MattFoley commented Aug 27, 2017

This issue has been open for a really long time, but I'm pretty sure this is the line that needed to change:
#1831

What was happening here is that CGRectIsEmpty returns true when either height or width is zero. With the current logic, one of those would always be zero when the parent was rendered off screen. This ensures that there the intersection be of CGSizeZero for the view to actually be clipped.

That being said, there seems to be something more complex going on here that I'm not understanding? I would think that you'd simply want to check if the child view's frame is within the bounds of the parent at all. If it was, then don't clip it. If I'm in the wrong, could someone explain this a bit more? If so, I'll fix this issue.

Test Plan

Using this repository, this one line change fixes the issue and still clips cells as they are scrolled off screen.

Fixes removeClipSubviews check for offscreen rendering of ListViews
This issue has been open for a really long time, but I'm pretty sure this is the line that needed to change:
#1831

What was happening here is that `CGRectIsEmpty` returns true when either height or width is zero. With the current logic, one of those would always be zero when the parent was rendered off screen. This ensures that there the intersection have a CGSizeZero for the view to actually be clipped. 

Using this [repository](https://github.com/jcharlet/react_native_listview_bug), this one line change fixes the issue and still clips cells as they are scrolled off screen. 

That being said, there seems to be something more complex going on here that I'm not understanding? I would think that you'd simply want to check if the child view's frame is within the bounds of the parent at all. If it was, then don't clip it. If I'm in the wrong, could someone explain this a bit more? If so, I'll fix this issue.
@pull-bot

This comment has been minimized.

Show comment
Hide comment
@pull-bot

pull-bot Aug 27, 2017

Attention: @shergin

Generated by 🚫 dangerJS

pull-bot commented Aug 27, 2017

Attention: @shergin

Generated by 🚫 dangerJS

@shergin

This comment has been minimized.

Show comment
Hide comment
@shergin

shergin Aug 27, 2017

Contributor

@MattFoley Do you have a test case for this?

Contributor

shergin commented Aug 27, 2017

@MattFoley Do you have a test case for this?

@MattFoley

This comment has been minimized.

Show comment
Hide comment
@MattFoley

MattFoley Aug 27, 2017

Contributor

@shergin I don't and I'm honestly not sure where to start with that. It looks like there aren't any tests around clipping yet. Do you think you could give me a hand? What would be expected?

Contributor

MattFoley commented Aug 27, 2017

@shergin I don't and I'm honestly not sure where to start with that. It looks like there aren't any tests around clipping yet. Do you think you could give me a hand? What would be expected?

@shergin

This comment has been minimized.

Show comment
Hide comment
@shergin

shergin Aug 27, 2017

Contributor

Starting just from a demo app which did not work properly before would be a good start.

Contributor

shergin commented Aug 27, 2017

Starting just from a demo app which did not work properly before would be a good start.

@MattFoley

This comment has been minimized.

Show comment
Hide comment
@MattFoley

MattFoley Aug 27, 2017

Contributor

@shergin That I do have. If you view this repository with no changes, you'll notice that after launch/refresh and tap to the second tab, the cells there do not display on iOS until you scroll them. Once you add my fix, that no longer happens, and it still removes the subviews properly when scrolled off screen.

The current workaround that I've been using (mentioned in this issue) is to render the ListView with removeClippedSubviews set to true, and then changing this to false after it moves into view. Apparently people have found similar workarounds for FlatList and other ScrollView based components, there's a ton of information in that thread. (Hard for me to follow all of it actually)

Does that work?

Contributor

MattFoley commented Aug 27, 2017

@shergin That I do have. If you view this repository with no changes, you'll notice that after launch/refresh and tap to the second tab, the cells there do not display on iOS until you scroll them. Once you add my fix, that no longer happens, and it still removes the subviews properly when scrolled off screen.

The current workaround that I've been using (mentioned in this issue) is to render the ListView with removeClippedSubviews set to true, and then changing this to false after it moves into view. Apparently people have found similar workarounds for FlatList and other ScrollView based components, there's a ton of information in that thread. (Hard for me to follow all of it actually)

Does that work?

@shergin

This comment has been minimized.

Show comment
Hide comment
@shergin

shergin Aug 29, 2017

Contributor

Here the thing, both ListView and removeClipSubviews are deprecated, and this mean that it is unlikely that someone from core team at Facebook will investigate this pretty magical problem. 😞
But if someone with good karma assures me that this is a right approach, I will merge this.
I am really sorry, but at this moment we are more affraid to break something stable/old than want to make other old and deprecated thing better.
Anyone? 😄
cc @janicduplessis

Contributor

shergin commented Aug 29, 2017

Here the thing, both ListView and removeClipSubviews are deprecated, and this mean that it is unlikely that someone from core team at Facebook will investigate this pretty magical problem. 😞
But if someone with good karma assures me that this is a right approach, I will merge this.
I am really sorry, but at this moment we are more affraid to break something stable/old than want to make other old and deprecated thing better.
Anyone? 😄
cc @janicduplessis

@MattFoley

This comment has been minimized.

Show comment
Hide comment
@MattFoley

MattFoley Aug 29, 2017

Contributor

@shergin Are you sure removeClippedSubviews is deprecated? I don't see this mentioned anywhere in the documentation or code.

Also, this isn't just a problem with ListView, that issue I linked has reports of people having the same problem with FlatList. I would guess this issue effects ScrollView, and other Views in some circumstances. (Although, I could totally be wrong)

Contributor

MattFoley commented Aug 29, 2017

@shergin Are you sure removeClippedSubviews is deprecated? I don't see this mentioned anywhere in the documentation or code.

Also, this isn't just a problem with ListView, that issue I linked has reports of people having the same problem with FlatList. I would guess this issue effects ScrollView, and other Views in some circumstances. (Although, I could totally be wrong)

@shergin

This comment has been minimized.

Show comment
Hide comment
@shergin

shergin Aug 30, 2017

Contributor

@MattFoley

Are you sure removeClippedSubviews is deprecated? I don't see this mentioned anywhere in the documentation or code.

Technically, it is not deprecated. Yet. But we are thinking about this. We see a lot of issues and negative side effects caused by this feature, and at the same time the positive possible benefits from this is very limited.
This feature will not be removed until it provides some significant benefits though.
We don't use it for <FlatList>, AFAIK. So, it should not be related. (Right?)

Contributor

shergin commented Aug 30, 2017

@MattFoley

Are you sure removeClippedSubviews is deprecated? I don't see this mentioned anywhere in the documentation or code.

Technically, it is not deprecated. Yet. But we are thinking about this. We see a lot of issues and negative side effects caused by this feature, and at the same time the positive possible benefits from this is very limited.
This feature will not be removed until it provides some significant benefits though.
We don't use it for <FlatList>, AFAIK. So, it should not be related. (Right?)

@MattFoley

This comment has been minimized.

Show comment
Hide comment
@MattFoley

MattFoley Sep 1, 2017

Contributor

@shergin FlatList is a subclass of VirtualizedList, which is a subclass of ScrollView, which is in turn implemented by RCTScrollView on the native ObjC side, which is then a subclass of RCTView. I'm running a slightly older version of RN, but I think that means that even FlatList would be impacted by issues with the removeClippedSubviews calculation.

Do you guys have a back up in place for removing removeClippedSubviews?

Any Table/Cell component needs the ability to pop offscreen views, and reuse them. It's a major performance bottleneck if not. RecyclerView and UITabletView both do this almost automatically for the developer, so it would be a shame if this was removed without a replacement.

Contributor

MattFoley commented Sep 1, 2017

@shergin FlatList is a subclass of VirtualizedList, which is a subclass of ScrollView, which is in turn implemented by RCTScrollView on the native ObjC side, which is then a subclass of RCTView. I'm running a slightly older version of RN, but I think that means that even FlatList would be impacted by issues with the removeClippedSubviews calculation.

Do you guys have a back up in place for removing removeClippedSubviews?

Any Table/Cell component needs the ability to pop offscreen views, and reuse them. It's a major performance bottleneck if not. RecyclerView and UITabletView both do this almost automatically for the developer, so it would be a shame if this was removed without a replacement.

@chirag04

This comment has been minimized.

Show comment
Hide comment
@chirag04

chirag04 Sep 1, 2017

Collaborator

related: #13316

Collaborator

chirag04 commented Sep 1, 2017

related: #13316

@kozillla

This comment has been minimized.

Show comment
Hide comment
@kozillla

kozillla Sep 8, 2017

I am experiencing the same issue with FlatList.
As https://facebook.github.io/react-native/docs/flatlist.html states "Note: may have bugs (missing content) in some circumstances - use at your own risk.This may improve scroll performance for large lists.".

Is it in any roadmap to when we can expect fix for it ?

kozillla commented Sep 8, 2017

I am experiencing the same issue with FlatList.
As https://facebook.github.io/react-native/docs/flatlist.html states "Note: may have bugs (missing content) in some circumstances - use at your own risk.This may improve scroll performance for large lists.".

Is it in any roadmap to when we can expect fix for it ?

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 8, 2017

Contributor

that's really an aged issue, #8607 #1831

Contributor

nihgwu commented Sep 8, 2017

that's really an aged issue, #8607 #1831

@shergin

This comment has been minimized.

Show comment
Hide comment
@shergin

shergin Sep 9, 2017

Contributor

@MattFoley I am affraid removingClippingSubviews feature cannot help with recycling (it cannot reduce memory footprint), it works differently.
Yes, as I mentioned earlier, this feature will not be removed until we have "back up plan".
AFAIK, enabling removingClippingSubviews for FlatList does not makes much sense since FlatList does recycle views (so there is no need to remove them from view hierarchy).

Contributor

shergin commented Sep 9, 2017

@MattFoley I am affraid removingClippingSubviews feature cannot help with recycling (it cannot reduce memory footprint), it works differently.
Yes, as I mentioned earlier, this feature will not be removed until we have "back up plan".
AFAIK, enabling removingClippingSubviews for FlatList does not makes much sense since FlatList does recycle views (so there is no need to remove them from view hierarchy).

@MattFoley

This comment has been minimized.

Show comment
Hide comment
@MattFoley

MattFoley Sep 11, 2017

Contributor

@shergin The issue I linked above still has people running into problems. I get a couple emails a week with replys to that issue. If my PR isn’t sufficient, can a fix for this be prioritised? It seems like a real problem that many people are having with both ListView and FlatList.

Contributor

MattFoley commented Sep 11, 2017

@shergin The issue I linked above still has people running into problems. I get a couple emails a week with replys to that issue. If my PR isn’t sufficient, can a fix for this be prioritised? It seems like a real problem that many people are having with both ListView and FlatList.

@lprhodes

This comment has been minimized.

Show comment
Hide comment
@lprhodes

lprhodes Sep 11, 2017

Contributor

It's a significant issue for every one of our apps. I'm unsure how the issue has gone under the radar for so long.

Using removeClipSubviews certainly has an impact on app performance, especially with older devices so I hope it won't be the case that it's removed.

Contributor

lprhodes commented Sep 11, 2017

It's a significant issue for every one of our apps. I'm unsure how the issue has gone under the radar for so long.

Using removeClipSubviews certainly has an impact on app performance, especially with older devices so I hope it won't be the case that it's removed.

@narychen

This comment has been minimized.

Show comment
Hide comment
@narychen

narychen Sep 11, 2017

Contributor

@MattFoley It indeed fixes my issue. I'm using the old version 0.27.2 of react-native and cannot update since I changed too much native code. But I have to turn the removeClipSubviews on because if turn it off the app will soon crash due to the memory issue as my app has too many GIF images.
But after I merged your code and get removeClipSubviews still be true. The ListView rows never will be blank again.
You are amazing. Thank you really very much.

Contributor

narychen commented Sep 11, 2017

@MattFoley It indeed fixes my issue. I'm using the old version 0.27.2 of react-native and cannot update since I changed too much native code. But I have to turn the removeClipSubviews on because if turn it off the app will soon crash due to the memory issue as my app has too many GIF images.
But after I merged your code and get removeClipSubviews still be true. The ListView rows never will be blank again.
You are amazing. Thank you really very much.

@kelset

This comment has been minimized.

Show comment
Hide comment
@kelset

kelset Sep 11, 2017

Collaborator

I understand that removingClippingSubviews will be removed/deprecated soon - but this fix has quite a small footprint; it's more like an hotfix to me, I'd love to see it merged & cherry-picked in 0.49 :3

Collaborator

kelset commented Sep 11, 2017

I understand that removingClippingSubviews will be removed/deprecated soon - but this fix has quite a small footprint; it's more like an hotfix to me, I'd love to see it merged & cherry-picked in 0.49 :3

@kozillla

This comment has been minimized.

Show comment
Hide comment
@kozillla

kozillla Sep 11, 2017

@MattFoley Thanks a lot for fixing this up. Im using the latest react-native ver 0.48.1 with your fix. It works fine in majority of cases, how ever Im still running into scenario whenFlatList becomes blank.

kozillla commented Sep 11, 2017

@MattFoley Thanks a lot for fixing this up. Im using the latest react-native ver 0.48.1 with your fix. It works fine in majority of cases, how ever Im still running into scenario whenFlatList becomes blank.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Sep 11, 2017

Contributor

Sounds like we should merge this.

Note FlatList doesn't set removeClippedSubviews by default, but developers may set it manually. More importantly, other libraries like navigation my use removeClippedSubviews which causes the proble.

Contributor

sahrens commented Sep 11, 2017

Sounds like we should merge this.

Note FlatList doesn't set removeClippedSubviews by default, but developers may set it manually. More importantly, other libraries like navigation my use removeClippedSubviews which causes the proble.

@MattFoley

This comment has been minimized.

Show comment
Hide comment
@MattFoley

MattFoley Sep 11, 2017

Contributor

@sahrens I had assumed it was defaulted to true everywhere. I just dug around and found that actually only ListView and WindowedListView set it to true by default. Perhaps FlatList has a different issue (or maybe they're just setting it to true for the performance?).

I think I've started understanding the more complex logic in this function. I think it's using the extra intersection logic to compensate for if a parent subview is only partially on screen, but the subview it's checking is still off screen. That must be why it's not just checking if it's within the bounds of it's parent. I'm not sure if that's a worthwhile optimization or not.

What do you think? Would it be better to simply check if a view is within the bounds of it's parent and clip it if it's not?

Contributor

MattFoley commented Sep 11, 2017

@sahrens I had assumed it was defaulted to true everywhere. I just dug around and found that actually only ListView and WindowedListView set it to true by default. Perhaps FlatList has a different issue (or maybe they're just setting it to true for the performance?).

I think I've started understanding the more complex logic in this function. I think it's using the extra intersection logic to compensate for if a parent subview is only partially on screen, but the subview it's checking is still off screen. That must be why it's not just checking if it's within the bounds of it's parent. I'm not sure if that's a worthwhile optimization or not.

What do you think? Would it be better to simply check if a view is within the bounds of it's parent and clip it if it's not?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Sep 12, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented Sep 12, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@shergin

This comment has been minimized.

Show comment
Hide comment
@shergin

shergin Sep 12, 2017

Contributor

Okay, let's do it!
If someone has troubles because of this change, please let us know.

Contributor

shergin commented Sep 12, 2017

Okay, let's do it!
If someone has troubles because of this change, please let us know.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Sep 12, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented Sep 12, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Sep 13, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented Sep 13, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Sep 13, 2017

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

facebook-github-bot commented Sep 13, 2017

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Sep 19, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented Sep 19, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Sep 26, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented Sep 26, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment