Improve inverted VirtualizedList implementation

[Sharcoux]

Use flex instead of complex transform inversion. Better for performance, readability and provides natural behaviour for onEndReached.

Summary

The current implementation of inverted in VirtualizedList introduces a lot of problems, especially combined with the onEndReached events. You can see it here.

The scroll is reverted in an unnatural way, the onEndReached is not emitted at the right moment, and the scroll bar seems to be teleported randomly.

Changelog

[General] [Fixed] - Better implementation for inverted and onEndReached on VirtualizedList

Test Plan

Would you mind helping me on this part? What should I do exactly?

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[cpojer]

@Sharcoux for the "Test Plan", can you fill out how exactly you verified that your PR fixes the described issue and how are you making sure it does not introduce regressions?

[sahrens]

How does this actually invert the scroll direction? It should start at the bottom, like a chat, rather than at the top, like a feed.

Can you describe more specifically what’s wrong with onEndReached?

A screencast if walking through all the options in the FlatListExample would be a good part of a test plan.

[Sharcoux]

The problem encountered is described here. You can experiment it with this sample.

@sahrens The idea here is to use the flex-direction : column-reverse to make the elements display from bottom to top

@cpojer Actually, I had trouble finding out how to test my changes. What I ended up doing was to replicate my changes directly into the nodes_modules of the sample project provided above. What is the easiest way to import my changes into the project ? I don't understand the build process... Or should I create an account on npm and publish my own package?

Am I the only one thinking that reversing the coordinate system of the ScrollView container and reversing again the system for the children Views is kinda hacky and doomed to introduce weird behaviours?

[Sharcoux]

Sorry for closing, I misclicked...

[terrysahaidak]

Hi @Sharcoux, you can use the example for my issues about bugs with inverted lists for testing and fixing it. Let's work together to fix all of these.

https://github.com/terrysahaidak/ReactNative-SeactionList-Bug-Example

So far I've tested your changes with latest RN and my example here is the result:

[Sharcoux]

Oh perfect @terrysahaidak, thanks. I told you, I manually reported the changes I made from the built react-native files in the node_modules, so I probably messed up some part.

Still, I don't understand how I am supposed to import my changes within your project. I see that you used react-native: '0.59.0-rc.3'. What is this version?
Moreover, i made the changes, but I didn't build them. I guess it is part of the publication process? So should I publish a new version each time I need to test a change? I really don't understand the workflow to change something and test the changes...

[Sharcoux]

@terrysahaidak I just inverted row-reverse and column-reverse at the very bottom of the file.

[terrysahaidak]

@Sharcoux that's the latest one published on NPM. As for me, the easiest way will be making changes directly in the node_modules of that repo. That allows you to see them as soon as possible.

The other way is cloning RN's master somewhere, building from sources and linking the built version via npm link. That one is better but requires some time. Let me know if you'll choose a second one I'll help you to link RN to that example project because it's not a straight forward process (at least for now).

[Sharcoux]

@terrysahaidak so there is no build process, ok.
I successfully replicated my changes into your project. I discovered some issues with the way separator components are inserted. Let me fix that.

[Sharcoux]

Ok, I fixed the insertion of separators. The current code for VirtualizedList is quite unsatisfying in my opinion. But I'm afraid it might be too late for a good clean up. I just kept things in phase with the current spirit to not break anything, but I'm wondering if it won't reveal performance issues for large lists.
Please, let me know your thoughts and if I missed something. For me the PR looks ok now.

[terrysahaidak]

The main problem with your solution and why the previous implementation used transform – it should be bottom-to-top scroll instead of top-to-bottom.

Here is your solution:

Here is the original one:

[Sharcoux]

I'm sorry, I'm not sure I get the difference. From your screencast, the only difference I see is that the scroll initial position is at the bottom, which I could probably fix if it's what is intended. Could you confirm?

[sahrens]

I'm not sure the issue is in Yoga - it's more about the behavior of the native ScrollView component. Just setting the initial scroll position is also not sufficient - you also need to change the behavior when appending new content - if you're looking at the first items (first elements of the children array) and then you add more, without doing the transform and relying only on that initial scroll position, the new items will push the first item down off the bottom of the viewport, one of the main things reverse mode is supposed to avoid.

[sahrens]

More fundamentally, I don't understand your concerns and motivations in the first place. What is the problem with performance? What are you trying to achieve? What's the problem with onEndReached? When you invert, the semantics change. If you need different semantics, just use onScroll to trigger whatever logic you want however you want - that's how onEndReached works under the hood.

[sahrens]

And yes I agree the code is not in a great state - it has evolved a ton with a bunch of complex features getting tacked on over the years that have complex interactions with each other. You don't have to use it though - you can use ScrollView directly, or you can use https://github.com/Flipkart/recyclerlistview, or you can fork VirtualizedList and strip out the features you don't need to simplify it - it depends on what you need to accomplish.

[sahrens]

And I agree that ScrollView with row-reverse should follow the w3c spec, but that's a whole project that is pretty low-pri at the moment. I think it can be done outside of Yoga though, but it will probably be pretty tricky.

[dblachut-adb]

Thank you very much for your commitment to the topic.

you also need to change the behavior when appending new content
[...]

I think that is already covered by column-reverse and row-reverse modes.

I don't understand your concerns and motivations in the first place

I am not sure about others but I am facing some problems on AndroidTV where such transformation is causing the scroll to work in opposite direction (AFAIK it is not yet reported anywhere). It happens when the user navigates using remote control and directional navigation takes place - the element in opposite direction to the desired one gets focused and the scroll direction is therefore reversed.

Correct me if I am wrong, but the current implementation is a bit hacky. It works by transforming whole view along one axis and transform every children (Cell) back to its original form. I am not sure how such transformations work under the hood, but surely this is not the most performant and elegant way to achieve the result.

IMO the implementation with column-reverse and row-reverse would be more straightforward.
The current implementation could possibly be simplified by transforming ScrollComponent through style prop, and transforming the content back through contentContainerStyle, but still this would not fix the problems that some of us are facing.

[sahrens]

Using a transform should actually be very performant because it's a simply matrix operation performed by the GPU. We haven't seen any noticeable performance impact in our usage.

Can you elaborate more on the issues you're seeing on AndroidTV? Why are you using inverted in the first place? The whole point is to reverse the scroll direction.

[dblachut-adb]

I see, so it looks like the performance should not be our concern even on some low-end GPUs and when there are a lot of elements?

I think it can be done outside of Yoga though, but it will probably be pretty tricky.

How about introducing some new props on ScrollView like initialScrollPosition and/or scrollMode which could contain something like:

• messageList - auto scroll to end on new element insertion (if the scroll is at the end)
• initiallyEnd - scroll to end on initial layout
• etc...

Then it could be simply used in FlatList/VirtualizedList in inverted mode.
If I am not mistaken some of these features are natively supported in Android. I have found things like stackFromBottom setReverseLayout setStackFromEnd. Maybe the scroll component could be implemented on ListView which have stackFromBottom attribute support.
I am not sure if I am not over-complicating it a bit though...

[dblachut-adb]

The current implementation could possibly be simplified by transforming ScrollComponent through style prop, and transforming the content back through contentContainerStyle, but still this would not fix the problems that some of us are facing.

I think I was wrong on this one - this would only help for the initial scroll position, but the order of elements would be incorrect as they would get mirrored-back.

[dblachut-adb]

Can you elaborate more on the issues you're seeing on AndroidTV? Why are you using inverted in the first place? The whole point is to reverse the scroll direction.

Hmmm... So on mobile devices the user needs to swipe in reverse direction than usual? I thought it was meant to reverse the item order and initial scroll position so that e.g. the element list grow upward.

I have something like a bottom navigation bar from which I am choosing the screen. If the screen contains list of tiles it is natural to start from the first one after pressing UP and scroll upward to further ones. The problem is that the directional navigation using DPAD on remote is working in opposite direction. Please see the description that I have added in my previous comment: #23632 (comment) (starting from It happens)

Edit:
I forgot to add very important thing: on AppleTV it works just fine!

[sahrens]

Oh I see, yeah the DPAD and touch gestures should all move the scroll in the same natural direction. Sounds like a bug in the AndroidTV ScrollView implementation or whatever primitive it's based on, especially if it works correctly on iOS, Android, and AppleTV. Who maintains AndroidTV? I'd focus on them.

[sahrens]

Maybe ask @krzysztofciombor - he added DPAD stuff in b7bb2e5. Or @aamalric for 00c8b3c

Maybe muck with stuff in here?

react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

Lines 301 to 309 in 3b6f6ca

	public boolean executeKeyEvent(KeyEvent event) {
	int eventKeyCode = event.getKeyCode();
	if (!mScrollEnabled
	&& (eventKeyCode == KeyEvent.KEYCODE_DPAD_UP
	|| eventKeyCode == KeyEvent.KEYCODE_DPAD_DOWN)) {
	return false;
	}
	return super.executeKeyEvent(event);
	}

[dblachut-adb]

Thanks! 👍 I will look on it a bit deeper but I still can't help the feeling that it could be fixed by improved version of this PR. :-)

[Sharcoux]

If you check here, you can notice the bug that @dblachut-adb was talking about, but there is worse. The onEndReached event is introducing weird behaviours when you scroll up and load more than 100 items.

This is caused by the current hack in the Flat-List. The only explaination I have to explain why the current code works on Android and iOS is that there should be a "counter-hack" in the native views that will fix the hack we are introducing here.

IMO, this is really not sane.

[Sharcoux]

@sahrens @dblachut-adb I really don't understand guys... Instead of fixing yoga implementation of column-reverse so that it can follow the specs, you'd rather keep a crazy hack in VirtualizedList, keep a hack somewhere in the Views, and keep a non standard implementation of column-reverse?

[dblachut-adb]

@Sharcoux I don't have a very good understanding on the responsibility of each react-native component, but if I am not mistaken yoga is the library which calculates the measurements for views and defines how the views are laid out on the screen. It does not contain any information about the initial scroll position nor it is not able to set it.

That was my concern and I asked react-native team if the potential fix should really be there.

[dblachut-adb]

BTW with your changes the initial scroll position would have to be changed on each platform. It will also have problems with the scroll position as reported by @terrysahaidak which means that in inverted mode:

• scrollToEnd will scroll to the bottom, which is the "beginning" in inverted mode
• all other scrollTo*(x) functions will have similar problem
• some events may report wrong scroll position
• maybe something else?

Unless I am wrong and this can really be fixed in yoga, which is common for all platforms, and hopefully the scroll position would then be reported/processed correctly.

[Sharcoux]

But you need to separate the concerns. The objective of VirtualizedList is to provide a React-Native implementation of a list based on CSS/flex styling.
The concern of the scroll initial position is relative to the implementation of column-reverse, and this should not be addressed in VirtualizedList, but in Yoga as it seems.

"some events may report wrong scroll position" -> That's exactly the opposite don't you think? If you draw all components on a reversed axis but with reversed-reversed transformation, don't you think that it is very likely that the detected scroll position will be wrong, or at least hard to make sense of?

What I mean is that VirtualizedList should solely rely on the implementation of row-reversed (and column-reversed respectively), and consider that those implementation respects the specs. If the current implementation on Android and iOS doesn't respects the specs, this is a totally different matters that should be addressed where it belongs.

I'm not sure I'm being clear, so sorry if I'm confusing.

[Sharcoux]

What scrollToEnd and onEndReached should be taking as reference for the "End" given if the list is inverted or not, this has no relation with the implementation of the View.

And with the current implementation, if I want to scroll to the start of an item in an inverted FlatList, what will it be? Because according to the VirtualizedList, the start of the item is actually the end of this item being drawn backwards... So there is 100% chance of what I'm trying to accomplish giving me headhache after 10 minutes.

[dblachut-adb]

The concern of the scroll initial position is relative to the implementation of column-reverse, and this should not be addressed in VirtualizedList, but in Yoga as it seems.

Yes, maybe the issue should be addressed in Yoga, but my concern was that the Yoga possibly does not maintain the position of scroll. In such situation it would need to send some information outside it or it should be implemented elsewhere.

That's exactly the opposite don't you think? If you draw all components on a reversed axis but with reversed-reversed transformation, don't you think that it is very likely that the detected scroll position will be wrong, or at least hard to make sense of?

I was under impression that the goal is to change the underlying implementation without breaking existing API and applications. If the goal is to follow the spec of row-reverse and column-reverse then it should be properly documented as a breaking change and the doc should clearly say what position you should use and will receive in events.

It may have more sense to have common position calculation on both modes. I guess it basically depends what do we want from "inverted" mode. If it should simply reverse the item order and change initial scroll position (the same as row/column-reverse styles), then it possibly should just be removed as it could be simply applied through contentContainerStyle property.

Currently the "inverted" mode is something more than the web row/column-reverse. Also please keep in mind that react-native does not fully follow the web standards, e.g. the default flex-direction is different.

What scrollToEnd and onEndReached should be taking as reference for the "End" given if the list is inverted or not, this has no relation with the implementation of the View.

If those methods are implemented in the View (VirtualizedList) itself then how it has no relation with it? What is your proposal on where it should be implemented then?

Don't get me wrong - I understand your concerns about the position calculation and the "badness" of current implementation. I also like your approach to implementing the "inverted" mode, but currently, it is a bit different in usage than previous solution. :)

[dblachut-adb]

this.props.contentContainerStyle is lost, please use array of styles or maybe StyleSheet.compose (I am not sure what is the difference).

[Sharcoux]

I probably got wrong what inversionStyle was supposed to be for. Ok, I should fix that indeed if it actually is the contentContainerStyle.

[Sharcoux]

Yes, maybe the issue should be addressed in Yoga, but my concern was that the Yoga possibly does not maintain the position of scroll. In such situation it would need to send some information outside it or it should be implemented elsewhere.

The position of the scroll is probably managed in the ScrollView, so everything related should stay there don't you think?

I was under impression that the goal is to change the underlying implementation without breaking existing API and applications. If the goal is to follow the spec of row-reverse and column-reverse then it should be properly documented as a breaking change and the doc should clearly say what position you should use and will receive in events.

I think that there might be some minor braking changes if people already hacked their way through scrollTo. But most cases should still behave the same. Inverted FlatList will start at the bottom and onEndReached / scrollToEnd will be relative to the top.

It may have more sense to have common position calculation on both modes. I guess it basically depends what do we want from "inverted" mode. If it should simply reverse the item order and change initial scroll position (the same as row/column-reverse styles), then it possibly should just be removed as it could be simply applied through contentContainerStyle property.

I think that as long as it is consistent it should be ok. We can reverse the axis or keep the same axis both ways. My concern is just that with current implementation, the offset inside one child goes on the opposite direction as the offset of the scroll view. Usually, yOffsetInParentView = yOffsetInChild + yOffsetOfChildInParent. Here, we have the very hacky and counter intuitive situation where yOffsetInParentView = yOffsetOfChildInParent + childHeight - yOffsetInChild. You see what I mean?

Currently the "inverted" mode is something more than the web row/column-reverse. Also please keep in mind that react-native does not fully follow the web standards, e.g. the default flex-direction is different.

As long as you don't pretend that View = div I don't see why the defaults should be the same. Thus, I don't feel that RN doesn't follow the standards. Anyway, here we are currently following the standards in this VirtualizedList. The issue is that the implementation of Yoga doesn't follow the standards. But as I understand, the Yoga project aims at providing a native implementation of the Flex standards, so if it fails to do so, I feel more that it is a bug that should be fixed than a feature that should be accepted as is.

What scrollToEnd and onEndReached should be taking as reference for the "End" given if the list is inverted or not, this has no relation with the implementation of the View.

If those methods are implemented in the View (VirtualizedList) itself then how it has no relation with it? What is your proposal on where it should be implemented then?

What I mean is that it is unrelated because deciding this doesn't mean we should change the axis direction. I mean that if we decide that the End is the top in inverted lists, we just need to make it behave this way in the implementation of these functions. Reversing the axis will indeed have an effect on the implementation but should be transparent as for the result. I'm not sure if I'm clear...

Don't get me wrong - I understand your concerns about the position calculation and the "badness" of current implementation. I also like your approach to implementing the "inverted" mode, but currently, it is a bit different in usage than previous solution. :)

I think there could be a difference in the result of the scrollTo function which will be more accurate as I explained. It will also make a difference if other people used the column-reversed property implemented by Yoga and were fine with the fact that it didn't respect the specs, but I don't see what it will change for the high majority of cases... Do you have something in mind?

[dblachut-adb]

The position of the scroll is probably managed in the ScrollView, so everything related should stay there don't you think?

Yes, that was exactly my point.

I think that there might be some minor braking changes if people already hacked their way through scrollTo. But most cases should still behave the same. Inverted FlatList will start at the bottom and onEndReached / scrollToEnd will be relative to the top.

With such proposal, to keep the same functionality, we would need something like scrollToBeginning and onBeginningReached. Actually I would strongly recommend to rename it to scrollToTop and scrollToBottom to avoid the confusion in inverted mode.

I think that as long as it is consistent it should be ok. We can reverse the axis or keep the same axis both ways. My concern is just that with current implementation, the offset inside one child goes on the opposite direction as the offset of the scroll view. Usually, yOffsetInParentView = yOffsetInChild + yOffsetOfChildInParent. Here, we have the very hacky and counter intuitive situation where yOffsetInParentView = yOffsetOfChildInParent + childHeight - yOffsetInChild. You see what I mean?

Yes I understand you. For me the best solution would be if the functionality would be the same and the position would be consistent (independent from using inverted mode or not). The alternative is to keep the same functionality and maintain the reversed position in reversed mode.

Yoga project aims at providing a native implementation of the Flex standards, so if it fails to do so, I feel more that it is a bug that should be fixed than a feature that should be accepted as is.

My point was that it may have more value to provide some functionality than follow the standards of different specification in this case. If we can have both - even better.

What I mean is that it is unrelated because deciding this doesn't mean we should change the axis direction. I mean that if we decide that the End is the top in inverted lists, we just need to make it behave this way in the implementation of these functions. Reversing the axis will indeed have an effect on the implementation but should be transparent as for the result. I'm not sure if I'm clear...

Yes, I totally agree.

I think there could be a difference in the result of the scrollTo function which will be more accurate as I explained. It will also make a difference if other people used the column-reversed property implemented by Yoga and were fine with the fact that it didn't respect the specs, but I don't see what it will change for the high majority of cases... Do you have something in mind?

What I meant was that if this commit breaks something - the doc should be updated. And it should not remove some functionality that previously was present.

[Sharcoux]

Thanks for your reply.
There are still 2 points where I think we still fail to understand each other.

1. You seem to say that Yoga won't be able to communicate to the ScrollView the position it should start to. I don't see why: the ScrollView is supposed to represent a data. It should be able to behave the way we want it to and we can code this behaviour there if needed. Right now, the issue is that we are using a wrong representation of the data in yoga (wrong css), a wrong implementation of the ScrollView (not starting at the bottom when it should), and more or less make it work as expected in the FlatView by hacking the CSS thanks to the transform API (reversing the axis and double reversing the items).
   I propose to start with a correct CSS representation of what is a reverted FlatList, then a correct implementation of how to represent the CSS reversed-column and reversed-row in a ScrollView by updating Yoga (and/or the ScrollView if needed, but I don't think we do), and end up with a solution that will change nothing for users, except that things like offset and directions will now be consistant.

2. "it should not remove some functionality that previously was present." -> I don't see what functionality won't be present anymore. I don't see what you are referring to...

With such proposal, to keep the same functionality, we would need something like scrollToBeginning and onBeginningReached. Actually I would strongly recommend to rename it to scrollToTop and scrollToBottom to avoid the confusion in inverted mode.

I totally disagree on this. The End should always refer to the direction where the last item in the list is represented. Thus, the end should be the top on reversed lists, and the bottom on normal lists. If you create scrollToTop and scrollToBottom, you will make it very confusing for horizontal lists. And creating scrollToLeft and scrollToRight functions will make it overcomplicated for nothing. I don't think either that we really need a onBeginningReached event, but why not.

[dblachut-adb]

1. You seem to say that Yoga won't be able to communicate to the ScrollView the position it should start to. I don't see why [...]

I have just said that I wasn't sure if Yoga is able to do this. That was just the thing I was afraid of, nothing more (I don't have the knowledge in this area). ;-)

   I propose to start with a correct CSS representation of what is a reverted FlatList, then a correct implementation of how to represent the CSS `reversed-column` and `reversed-row` in a ScrollView by updating Yoga (and/or the ScrollView if needed, but I don't think we do), and end up with a solution that will change nothing for users, except that things like offset and directions will now be consistant.

Yes, that would be great.

2. I don't see what functionality won't be present anymore. I don't see what you are referring to...

Right now e.g. scrollToEnd does not work as expected and contentContainerStyle cannot be used.

I totally disagree on this. The End should always refer to the direction where the last item in the list is represented. Thus, the end should be the top on reversed lists, and the bottom on normal lists. I don't think either that we really need a onBeginningReached event, but why not.

You have understood me wrong, the rename would be needed in your current implementation, in which scrollToEnd always scrolls to bottom independently of the inverted mode or not.

If you create scrollToTop and scrollToBottom, you will make it very confusing for horizontal lists. And creating scrollToLeft and scrollToRight functions will make it overcomplicated for nothing.

Ah, my bad, I have forgot about the horizontal mode.

[Sharcoux]

Right now e.g. scrollToEnd does not work as expected and contentContainerStyle cannot be used.

I agree on contentContainerStyle, it should be fixed.

Oh! My bad for scrollToEnd. I didn't see it being implemented here. Indeed, it should be fixed too.

[mallenexpensify]

If anyone is looking for an incentive to fix this aspect Selecting, copying, and pasting text on web and desktop are weird and in inverted order We're willing to compensate. Issue is posted here
Expensify/App#1341

[mallenexpensify]

We're still looking for help to "Fix Inverted prop on the FlatList component with a RN Patch". The current link is Expensify/App#3381 job amount is $8000. Any help and recommendations are greatly appreciated.