Fixes removeClipSubviews check for offscreen rendering of ListViews #15669
Conversation
This issue has been open for a really long time, but I'm pretty sure this is the line that needed to change: facebook#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.
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
@MattFoley Do you have a test case for this? |
|
@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? |
|
Starting just from a demo app which did not work properly before would be a good start. |
|
@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 Does that work? |
|
Here the thing, both |
|
@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 |
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. |
|
@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 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. |
|
related: #13316 |
|
I am experiencing the same issue with FlatList. Is it in any roadmap to when we can expect fix for it ? |
|
@MattFoley I am affraid |
|
@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. |
|
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 |
|
@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. |
|
I understand that |
|
@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. |
|
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. |
|
@sahrens I had assumed it was defaulted to 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
added
GH Review: accepted
Import Started
|
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Okay, let's do it! |
|
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
facebook-github-bot
added
Import Failed
and removed
Import Started
|
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
added
the
Import Started
|
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
If it's a performance problem caused by listview |
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
CGRectIsEmptyreturns 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.