Skip to content
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

Closed

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Jul 6, 2022

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)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 6, 2022
@fabOnReact fabOnReact changed the title Inverted flatlist accessibility 🚧 Inverted flatlist accessibility 🚧 Jul 6, 2022
@analysis-bot
Copy link

analysis-bot commented Jul 6, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,730,069 +1,784
android hermes armeabi-v7a 7,133,244 +1,776
android hermes x86 8,037,167 +1,778
android hermes x86_64 8,010,471 +1,783
android jsc arm64-v8a 9,597,944 +976
android jsc armeabi-v7a 8,364,259 +968
android jsc x86 9,542,170 +972
android jsc x86_64 10,134,579 +986

Base commit: 4f83498
Branch: main

@analysis-bot
Copy link

analysis-bot commented Jul 6, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 4f83498
Branch: main

@fabOnReact fabOnReact changed the title 🚧 Inverted flatlist accessibility 🚧 Inverted flatlist accessibility Jul 8, 2022
@fabOnReact fabOnReact changed the title Inverted flatlist accessibility avoid using transform scaleY to invert flatlist Jul 8, 2022
@fabOnReact fabOnReact changed the title avoid using transform scaleY to invert flatlist Avoid using transform scaleY to invert flatlist Jul 8, 2022
@rozele
Copy link
Contributor

rozele commented Jul 11, 2022

@fabriziobertoglio1987 In case you're interested, I recently worked on a fix for react-native-windows list inversion. It uses the flexDirection: "column-reverse" approach described here: #30373 (comment)

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?

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Jul 12, 2022

In case you're interested, I recently worked on a fix for react-native-windows list inversion. It uses the flexDirection: "column-reverse" approach described here: #30373 (comment)

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

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?

The current implementation uses setScaleX

The current implementation uses (setScaleX).

[
 Sx 0  0  0
 0  Sy 0  0
 0  0  Sz 0
 0  0  0  1
]

A negative value (scaleY: -1) results in a point reflection in that dimension. The value 1 has no effect.
In geometry, a point reflection or inversion in a point (or inversion through a point, or central inversion) is a type of isometry of Euclidean space.

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.
This solution would be used only when the screenreader is enabled.

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.

let initialState = {
first: this.props.initialScrollIndex || 0,
last:
Math.min(
this.props.getItemCount(this.props.data),
(this.props.initialScrollIndex || 0) +
initialNumToRenderOrDefault(this.props.initialNumToRender),
) - 1,
};

In the not-inverted FlatList, the optimization scrollToTop always adds the first page.

const lastInitialIndex = this.props.initialScrollIndex
? -1
: initialNumToRenderOrDefault(this.props.initialNumToRender) - 1;
const {first, last} = this.state;
this._pushCells(
cells,
stickyHeaderIndices,
stickyIndicesFromProps,
0,
lastInitialIndex,
inversionStyle,
);

The same optimization is added with my implementation of inverted FlatList

if (this.props.inverted) {
this._pushCells(
cells,
stickyHeaderIndices,
stickyIndicesFromProps,
0,
lastInitialIndex,
);

when the user scroll down/up, first/last are updated and the other cells are added to the VirtualizedList to be displayed

this._pushCells(
cells,
stickyHeaderIndices,
stickyIndicesFromProps,
firstAfterInitial,
last,
);

The for loop direction in case of inverted flatlist is changed:

  • inverted: the loop is from last to first (10 (last item) --> 0 (first item))
  • not inverted: the loop is from first to last (0 (first item) --> 10 (last item))

if (this.props.inverted) {
for (let ii = last; ii >= first; ii--) {
pushCell(ii);
}
} else {
for (let ii = first; ii <= last; ii++) {
pushCell(ii);
}
}

@fabOnReact fabOnReact changed the title Avoid using transform scaleY to invert flatlist Avoid using transform scaleY to invert flatlist when TalkBack is enabled Jul 12, 2022
the code triggers a scrollUp to the end of the inverted flatlist, when
we delete the item on top of the flatlist.
@fabOnReact
Copy link
Contributor Author

fabOnReact commented Jul 13, 2022

Inverting the FlatList order does not invert the following behaviors.

when a new item is prepended to the list, the ScrollView should synchronously scroll it into view

Main - Not inverted flatlist

2022-07-13.21-07-47.mp4

Main - inverted flatlist - position is not on first item, the new item does not scroll into view

when a new item is prepended to the list, the ScrollView should synchronously scroll it into view
if the position is not on the first item - the flatlist will not scroll the new item into view

2022-07-14.22-32-16.mp4
Branch - inverted flatlist - overriding the default ScrollView behaviour

maintainVisibleContentPosition is used to the default scrollview behaviour:

  • avoid the scrollToTop when pretending item, which corresponds to appending it to an inverted flatlist. This requires us to keep the scrollview position to the original position
  • triggering a scrollToBottom when appending item, which corresponds to pretending an item to an inverted flatlist
2022-07-13.22-11-48.mp4

when items are appended to the end of the list, the view needs to stay in the same position

Main - Not inverted flatlist

2022-07-13.21-05-42.mp4

Deleting an Item from the end of the List

Main - Not inverted flatlist

2022-07-13.21-10-29.mp4

@fabOnReact
Copy link
Contributor Author

@fabOnReact fabOnReact marked this pull request as ready for review September 28, 2022 00:30
@fabOnReact fabOnReact marked this pull request as draft September 28, 2022 01:09
@fabOnReact fabOnReact marked this pull request as ready for review September 28, 2022 01:13
@fabOnReact fabOnReact marked this pull request as draft September 28, 2022 02:03
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
@fabOnReact fabOnReact changed the title Avoid using transform scaleY or scaleX to invert flatlist when TalkBack is enabled 1/5 - Avoid using transform scaleY or scaleX in inverted flatlist when TalkBack and enabledTalkbackCompatibleInvertedList prop are enabled Sep 28, 2022
@fabOnReact fabOnReact changed the title 1/5 - Avoid using transform scaleY or scaleX in inverted flatlist when TalkBack and enabledTalkbackCompatibleInvertedList prop are enabled 1/5 Avoid using transform scaleY or scaleX in inverted flatlist when TalkBack and enabledTalkbackCompatibleInvertedList prop are enabled Sep 28, 2022
@fabOnReact fabOnReact marked this pull request as ready for review September 28, 2022 03:57
Copy link
Contributor

@NickGerleman NickGerleman left a 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inverted FlatList accessibility order
8 participants