[Mobile] - KeyboardAwareFlatList - Enable FlatList virtualization for iOS #59833
Conversation
…on for larger lists
geriux
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.75 MB ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Potentially helps with this: wordpress-mobile/WordPress-iOS#23023 |
|
@geriux is it fair to say that we should expect some animation hitches on large posts when scrolling quickly through content due to lazy loading? I think it's a fair trade-off, if so. Just confirming expectations. |
There was a problem hiding this comment.
@geriux Thank you for making these changes to improve virtualization and scrolling behavior for long posts.
Sharing my testing experience:
iOS
Tested with an iPhone SE 2020
✅ Drag and Drop
Drag and drop for blocks worked well. I did not note any issues when using the long post markup template provided in the PR description.
Drag and Drop
drag-and-drop.mov
✅ Paste HTML Content
I did not note any issues when pasting HTML content into a post using the long post markup template provided in the PR description.
Paste HTML Content
paste-html.mov
⚪ Scrolling on a long post using the content template provided in the PR description
Scrolling with long content was mostly smooth. When starting from the top of a long post, the initial scrolling behavior and JS/UI framerate seemed to be improved over trunk.
When nearing the end of a post, however, I noted that the scroll behavior seemed to "snap" the view vertically by a small amount. Scrolling back up from the bottom of a long post sometimes moved the vertical position of the Editor without any scrolling input on my part, even for 2 - 3 seconds after releasing my touch from the screen.
I wanted to check if this matched your experience, and could be considered part of the caveat included in the warning:
This example code has 195 blocks so it is expected for the performance to not be 100% smooth as blocks will mount and unmount while scrolling.
In the example video below, you can keep an eye on the position of the scrollbar indicator on the right hand side of the screen to note when it causes the screen to jump a bit. This may be expected behavior, however, as the blocks are mounting and unmounting while scrolling (and thus changing the height of the view).
Scrolling Long Content
scrolling-long-content.mov
Android
Tested with a Samsung Galaxy SE 20
On Android, the experience worked as expected. I was able to drag-and-drop, paste HTML content, and perform a scroll on long content. The scrolling behavior was not as smooth as on iOS.
Android Long Scrolling
android.mov
For both platforms, I was able to test the RichText clear selection when unmounting behavior successfully.
Let me know if you have any further thoughts on the iOS scrolling behavior. I did not experience any crashes during my testing, so if the performance improvement is even incremental, it may be worth moving forward with the change.
Yes if there's a lot of content, there might be some jumpiness when scrolling fast or blank spaces for example, due to blocks being attached to the list again. Hopefully, those could improve in the future with the new architecture. That behavior should be reproducible on Android currently as well due to the virtualization of the FlatList it already has. |
Correct, that's something that I experienced as well with having too many blocks of different kinds. I think that was kind of like a stress test to see if nothing breaks 😄
Thank you for sharing the video, I saw that too with the example content. I think this is a trade-off when using virtualization but in a way, it would prevent other issues related to low memory which would prevent some of the crashes we get.
I think on Android it is prioritizing loading the items and blocking the rendering part, so the scroll is a bit slow but the items are visible when you reach that position.
I agree that it would be worth moving with these changes to prevent other issues. In the future, we can work on continuing to optimize the FlatList and possibly improving the performance with the new architecture. Thank you for testing and sharing your feedback @derekblank ! |
There was a problem hiding this comment.
Correct, that's something that I experienced as well with having too many blocks of different kinds. I think that was kind of like a stress test to see if nothing breaks.
I agree that it would be worth moving with these changes to prevent other issues. In the future, we can work on continuing to optimize the FlatList and possibly improving the performance with the new architecture.
Thanks for clarifying that this behavior is expected, which is what I would expect as well. The code changes look good to me. I support moving this change forward as an overall improvement. Nice work! 🚀
| maxToRenderPerBatch: 15, | ||
| }; | ||
|
|
||
| export const OPTIMIZATION_ITEMS_THRESHOLD = 30; |
There was a problem hiding this comment.
Potentially, we may want to leave a note or comment with some further context on these values (as they deviate from the default values). Not a blocker for this PR, however.
There was a problem hiding this comment.
Good idea! Updated in a9dc932
| keyboardShouldPersistTaps="handled" | ||
| onContentSizeChange={ onContentSizeChange } | ||
| onScroll={ scrollHandler } | ||
| scrollEventThrottle={ 16 } |
There was a problem hiding this comment.
At some point, we may want to consider exporting the scrollEventThrottle value from a shared constant, as it's used in a few other places. Also not a blocker (since it wasn't a change in this PR), but in context of adding the shared.native.js file for optimization prop values, it may also help provide some context for other developers in the future if all non-default scroll behavior values are exported from the same place.
# Conflicts: # packages/react-native-editor/CHANGELOG.md
Fixes #53265
Related PRs:
What?
This PR enables virtualization for the block list on iOS.
Why?
Since we had the
FlatListnested within aScrollView, virtualization wasn't working.FlatList's base implementation comes from the VirtualizedList component which improves memory usage in long lists.
How?
It removes the
ScrollViewwrapper and changes theFlastListto anAnimatedFlatListsimilar to Android's implementation.Adds a new
sharedfile withOPTIMIZATION_PROPSto share the same optimization properties for both iOS and Android. This will only be set when there are more than30blocks. If there are less blocks, it'll use the default values.windowSize: The default value is 21, from the docs:So for our case it'll be 8 above, 8 below and one in between.
maxToRenderPerBatch: The default value is 10, from the docs:Note: I didn't want to tweak these props too much to prevent possibly blank spaces when scrolling or other undesired side effects. Let's see how virtualization works for iOS users to think about other possible optimizations if needed.
Lastly, a new check was added in the RichText component to clear the selection (if it's selected) when it's unmounted. This is already reproducible on Android for long posts, if you scroll down to the very end and select that text block, and then scroll to the top, the keyboard gets hidden. If you start scrolling down, as soon as the block is mounted again (due to virtualization) the selection will try to be restored. So for this scenario is best to remove the selection if the keyboard is being hidden anyway.
Testing Instructions
Note
Please use the following builds to test these changes:
Smoke testing
Scrolling with a long content
Warning
This example code has 195 blocks so it is expected for the performance to not be 100% smooth as blocks will mount and unmount while scrolling.
Example content
Testing Instructions for Keyboard
N/A
Screenshots or screencast
Loading a large post (over 700 blocks)
Loading a lot of media files
0418.mov