-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
1/5 Avoid using transform scaleY or scaleX in inverted flatlist when TalkBack and enabledTalkbackCompatibleInvertedList prop are enabled #34141
Conversation
Base commit: 4f83498 |
Base commit: 4f83498 |
addresses issues explained in facebook#30373 (comment) handles cases when adding new item to inverted flatlist (flatlist has to scroll up to the new item) test cases facebook#30373 (comment)
https://app.circleci.com/pipelines/github/facebook/react-native/14085/workflows/4d1b9303-2f2b-4cd1-877a-6e863b1a0afc/jobs/264493 https://app.circleci.com/pipelines/github/facebook/react-native/14085/workflows/6f1e7d35-d7cd-4971-80ec-c94417293378/jobs/264496 https://app.circleci.com/pipelines/github/facebook/react-native/14085/workflows/4d1b9303-2f2b-4cd1-877a-6e863b1a0afc/jobs/264500
removing this logic creates a regression in not-nested inverted flatlist
@fabriziobertoglio1987 In case you're interested, I recently worked on a fix for react-native-windows list inversion. It uses the Here is the PR: microsoft/react-native-windows#8440 The PR for Windows also addresses the issues with infinite scroll and virtualization described here: #30373 (comment) I wrote up a more detailed requirement for the platform requirements and what they map to on Windows here: https://gist.github.com/rozele/47a2ad44f6d44561da1d293454ae8418 The most important bits are the view and edge anchoring behaviors, i.e., when a new item is prepended to the list, the ScrollView should synchronously scroll it into view (i.e., drawn to view port and scrolled to bottom in same frame), and when items are appended to the end of the list, the view needs to stay in the same position. Windows has specific features to implement these behaviors, but I imagine it's possible to achieve this in Android with something like the maintainVisibleContentPosition API (which there is a WIP draft for Android): #29466 I haven't looked to closely at the approach you've implemented here. Do you have a design doc for it? If the approach is just to reverse the content items, any considerations for the cost of reversing the array in very long lists vs. the Yoga/flex reverse approach? |
Thanks, I will try to implement your solution. I will publish a second alternative solution based on your Pull Request microsoft/react-native-windows#8440
The current implementation uses setScaleX
The current implementation uses (
Original: Tomruen Vector: KCVelaga Related Transformation Matrix on Wikipedia, Scale Transformation, scale() on mdn docs #30373 (comment) The solution in this Pull Request
The runtimes of rendering a not-inverted or inverted VirtualizedList are similar. The current VirtualizedList implementation keeps track of the first and last item on the screen. These are the items rendered while the users scroll up/down. react-native/Libraries/Lists/VirtualizedList.js Lines 719 to 727 in 873ff0c
In the not-inverted FlatList, the optimization scrollToTop always adds the first page. react-native/Libraries/Lists/VirtualizedList.js Lines 944 to 955 in 873ff0c
The same optimization is added with my implementation of inverted FlatList react-native/Libraries/Lists/VirtualizedList.js Lines 1011 to 1018 in 6271850
when the user scroll down/up, first/last are updated and the other cells are added to the VirtualizedList to be displayed react-native/Libraries/Lists/VirtualizedList.js Lines 1003 to 1009 in 6271850
The for loop direction in case of inverted flatlist is changed:
react-native/Libraries/Lists/VirtualizedList.js Lines 847 to 855 in 6271850
|
the code triggers a scrollUp to the end of the inverted flatlist, when we delete the item on top of the flatlist.
Inverting the FlatList order does not invert the following behaviors.
Main - Not inverted flatlist
2022-07-13.21-07-47.mp4Main - inverted flatlist - position is not on first item, the new item does not scroll into view
2022-07-14.22-32-16.mp4Branch - inverted flatlist - overriding the default ScrollView behaviour
maintainVisibleContentPosition is used to the default scrollview behaviour:
2022-07-13.22-11-48.mp4
Main - Not inverted flatlist
2022-07-13.21-05-42.mp4Deleting an Item from the end of the List Main - Not inverted flatlist
2022-07-13.21-10-29.mp4 |
- Make sure flatlist start from the end by default when using the functionality - remove state contentLength and visibleLength, as already included in this._scrollMetrics
Reversed commit 193e9f4 The original issue facebook#34141 (comment) does not reproduce anymore
the logic with contentOffset does not properly work and requires future functionalities to detect different edge case. Will be part of the next PR Reintroduce changes from facebook@e18ac41 in VirtList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from any specific nits about implementation, for some of the reasons I hinted at earlier, I'm not sure this overall change makes sense.
Having an alternate but incomplete implementation of inversion, specifically when talkback is enabled, means we will need to support two separate implementations of inversion long term, and means that VirtualizedList is no longer platform agnostic. There was a mention that we didn't want to use the same inversion implementation in both in case the one not working was a result of a bug in talkback, but I'm a little bit confused about why we think that is, or that talkback behavior around transforms will change. Though it either way leaves the API and implementation in a bit of a messy place, for as long as we would need to support the Android devices on the market today.
I think it's righteous to change how we do inversion, but if we did that, I don't know we should do it along such specific lines, and I think we would want it more fully compatible with the rest of the list functionality.
If we get to a place where we have an approach that we feel confident in, we should be adding those unit tests I mentioned, along with applying any changes made to VirtualizedList to VirtualizedList_EXPERIMENTAL, since the latter is in the process of actively replacing the former (it is partially in production internally, and will replace the original in OSS once that's fully enabled).
Summary
FEATURE 1/5 - Avoid using transform scaleY or scaleX in inverted flatlist when TalkBack and enabledTalkbackCompatibleInvertedList prop are enabled.
The implementation, instead of using scaleX, inverts the rendering order if the screenreader and enabledTalkbackCompatibleInvertedList prop are enabled.
The other functionalities are included as draft in the rn-tester flatlist-inverted example and will be part of the upcoming PRs:
FEATURE 2/5 - Starting the inverted flatlist from the end. The current implementation is included in flatlist-inverted example.
FEATURE 3/5 - Remove Platform check, adapt and test the functionality for iOS.
FEATURE 4/5 - onEndReached to be compatible with the infinite list. Example of implementation in the flatlist-inverted. Requires to disable in the ScrollView Implementation
when a new item is prepended (appended in this case) to the list, the ScrollView should synchronously scroll it into view
.FEATURE 5/5 - Add Support for ScrollView.maintainVisibleContentPosition on Android #29466
fixes #30373
Changelog
[Android] [Changed] - Avoid using transform scaleY or scaleX in inverted flatlist when TalkBack and enabledTalkbackCompatibleInvertedList prop are enabled
Test Plan
Video Test Cases summaries are included in the following discussion and in the pull request comments.
The functionality is enabled on the FlatList-inverted example in rn-tester and FlatList-nested example in rn-tester.
Update 23/09/2022 #34141 (comment)