[DF] feat(#547): add `experimentalScrollPositionManagement` support to FlashList #824

friyiajr · 2023-04-24T22:55:36Z

Description

Fixes: #547

Adds support for maintaining vertical content position in iOS and Android. Also fixes resizing issues when changing device orientation from portrait to landscape

Reviewers’ hat-rack 🎩

Messages Test:

To test this out, paste this code over top of the Messages.tsx
Messages.tsx.txt
Add a lot of messages to the window until you feel there is a couple viewports worth
Scroll up halfway
Keep adding messages
The list should maintain the position of the content

Twitter Test:

Add the experimentalScrollPositionManagement property to the FlashList in Twitter.tsx
Scroll half way down the timeline
Switch the orientation
Slowly scroll up
You should have smooth scrolling and no more jumps

Screenshots or videos (if needed)

iOS Demo

Messages:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-04-24.at.18.36.26.mp4

Twitter:

Screen.Recording.2023-04-24.at.6.38.56.PM.mov

Android Demo

Messages:

a_messages.mov

Twitter:

a_twitter.mov

Checklist

I have added a changelog entry following the Keep a Changelog guidelines.

geo-vi · 2023-04-29T11:14:04Z

keep it up! we need this merged asap! 🔥 🚀

DLkhazan · 2023-04-30T06:29:26Z

would some be kind enough to approve this?

DLkhazan · 2023-04-30T08:01:12Z

@naqvitalha would you please approve this?

HuuNguyen312 · 2023-05-03T06:28:45Z

How can I install flash-list from this branch
https://github.com/friyiajr/flash-list/tree/maintainPositionAndroid

aweffr · 2023-05-05T08:59:29Z

does this require react native version greater than some number?

I tried this on our 0.62.3 project and got
'_reactNative.ScrollView.render().type' TypeError, seems that this version doesn't support code in

https://github.com/friyiajr/flash-list/blob/6dbf5d7f55849ad115081d3b0dbefc26508178f9/src/BiDirectionalScrollView.tsx

// ...
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
const ScrollViewRNRaw: Component<ScrollViewProps> = ScrollViewRN.render().type;
// ...

perunt · 2023-05-20T18:37:33Z

Super excited about this!

I've been testing out this change for a while now, and it's been mostly awesome. But when I tried to scroll while adding a new item, I noticed some jitters. I made a quick video to show what's going on. In the video, you'll notice I'm also firing off a new message every second.
Honestly, I caught the worst scenario in that video. Sometimes, the jitters only happened while scrolling. So, yeah, that's what's been happening.

telegram-cloud-document-2-5424748569082999196.mp4

terrysahaidak · 2023-06-02T12:02:43Z

Tested this PR on iOS and it seems like it does not work, at least in my case.

I'm implementing similar to Instagram navigation where user needs to navigate from a grid to a particular item in a list.
Since scrollToIndex doesn't really work, i tried to render only items i need and the item i navigate to being the first one, and after the mount I request the animation frame and set rest of the data, and since it basically appends everything at the top, maintainVisibleContentPosition does the trick. But experimentalScrollPositionManagement seems to be not working in this case:

  const { data } = useListData()

   const [dataToDisplay, setDataToDisplay] = useState(() => {
     return data.slice(route.params.activeItemIndex, data.length)
   })

  useEffect(() => {
    requestAnimationFrame(() => {
      setDataToDisplay(data)
    })
  }, [])

I use @react-navigation/native-stack if this makes any difference.

PS. Great job so far, exciting for this feature to be part of flash-list. Let me know if you need more info on this.

rkstar · 2023-06-08T13:38:25Z

this PR has been open for 6 weeks now. is anyone going to review it?

naqvitalha · 2023-06-08T17:38:57Z

Folks, apologies for the delay on this feature. I'm gonna review this tomorrow! This might have some bugs but we'll continue to address and make it perfect overtime.

naqvitalha · 2023-06-09T17:18:26Z

android/src/main/kotlin/com/shopify/reactnative/flash_list/AutoLayoutShadow.kt

    /** Checks for overlaps or gaps between adjacent items and then applies a correction (Only Grid layouts with varying spans)
     * Performance: RecyclerListView renders very small number of views and this is not going to trigger multiple layouts on Android side. Not expecting any major perf issue. */
-    fun clearGapsAndOverlaps(sortedItems: Array<CellContainer>) {
+    fun clearGapsAndOverlaps(sortedItems: Array<CellContainer>, scrollView: ScrollView, maintainTopContentPosition: Boolean) {


I think the flag can be maintained as a variable within this class and final diff can be applied by AutoLayout. That will keep this class independent of view layer like it used to be,

naqvitalha · 2023-06-09T17:19:32Z

android/src/main/kotlin/com/shopify/reactnative/flash_list/AutoLayoutShadow.kt

@@ -15,13 +17,21 @@ class AutoLayoutShadow {
    private var lastMaxBound = 0 // Tracks where the last pixel is drawn in the visible window
    private var lastMinBound = 0 // Tracks where first pixel is drawn in the visible window

+    private var isInitialRender = true


What do we need the initial render for? A comment would be helpful

naqvitalha · 2023-06-09T17:21:29Z

src/BiDirectionalScrollView.tsx

+  View,
+} from "react-native";
+
+import { BidirectionalFlatlist } from "./BidirectionalFlatlist";


Can we call it something else? I got confused if we're using FlatList :D

How does BidirectionalList sound?

naqvitalha · 2023-06-09T17:22:45Z

src/FlashList.tsx

+      this.props.experimentalScrollPositionManagement &&
+      this.props.renderScrollComponent
+    ) {
+      throw new CustomError(ExceptionList.customMaintainScrollNotSupported);


I'm wondering if people can still use custom scroll component by using DoubleSidedScrollView instead of ScrollView in their component? In that case we can export DoubleSidedScrollView from FlashList too.

That could be possible. I think it makes sense to restrict users from doing this for now though. This feature is experimental at the moment and its probably easier to maintain if we restrict consumers of the library to using our implementation of the DoubleSidedScrollView for now.

If this implementation proves to be stable, then I think we can allow users to customize as they feel appropriate. What are your thoughts?

naqvitalha · 2023-06-09T17:23:40Z

src/FlashList.tsx

@@ -382,7 +398,10 @@ class FlashList<T> extends React.PureComponent<
          itemAnimator={this.itemAnimator}
          suppressBoundedSizeException
          externalScrollView={
-            renderScrollComponent as RecyclerListViewProps["externalScrollView"]
+            this.props.experimentalScrollPositionManagement &&
+            Platform.OS === "android"


Use the filers in config folder to add platform check. We discourage inline platform checks.

naqvitalha · 2023-06-09T17:24:12Z

src/FlashList.tsx

@@ -500,6 +522,10 @@ class FlashList<T> extends React.PureComponent<
          ...getCellContainerPlatformStyles(this.props.inverted!!, parentProps),
        }}
        index={parentProps.index}
+        stableId={
+          /* Empty string is used so the list can still render without an extractor */
+          this.props.keyExtractor?.(parentProps.data, parentProps.index) ?? ""


Should we skip this if the flag is off?

I don't think so, in Objective-C its always expecting a stable id to have some value.

rkstar · 2023-06-21T11:39:25Z

cc @friyiajr - seems like fairly minor comments. this is a huge feature to get shipped to the community.

tauntmachine · 2023-07-06T06:36:08Z

can we please ship this feature asap? @naqvitalha @friyiajr

HuuNguyen312 · 2023-07-06T07:19:01Z

@naqvitalha @friyiajr .

DLkhazan · 2023-07-17T07:46:35Z

How can I install flash-list from this branch
https://github.com/friyiajr/flash-list/tree/maintainPositionAndroid

any success on that?

HuuNguyen312 · 2023-07-17T07:49:26Z

How can I install flash-list from this branch
https://github.com/friyiajr/flash-list/tree/maintainPositionAndroid

any success on that?

You can try
yarn add @huunguyen312/flash-list

DLkhazan · 2023-07-17T08:56:10Z

How can I install flash-list from this branch
https://github.com/friyiajr/flash-list/tree/maintainPositionAndroid

any success on that?

You can try yarn add @huunguyen312/flash-list

unable to build after this. any work arounds?

HuuNguyen312 · 2023-07-17T09:17:00Z

How can I install flash-list from this branch
https://github.com/friyiajr/flash-list/tree/maintainPositionAndroid

any success on that?

You can try yarn add @huunguyen312/flash-list

unable to build after this. any work arounds?

You need to replace @shopify/flash-list to @huunguyen312/flash-list

DLkhazan · 2023-07-17T11:37:22Z

How can I install flash-list from this branch
https://github.com/friyiajr/flash-list/tree/maintainPositionAndroid

any success on that?

You can try yarn add @huunguyen312/flash-list

unable to build after this. any work arounds?

You need to replace @shopify/flash-list to @huunguyen312/flash-list

okay, I got it running. But did the experimentalScrollPositionManagement worked on android?
I'm adding only one element at the top and its not holding its position.
Any thoughts on that?

chj-damon · 2023-07-21T08:02:07Z

@naqvitalha @friyiajr any updates on this PR?

aweffr · 2023-07-24T07:00:35Z

Any updates on this +1 ?

DLkhazan · 2023-07-24T07:38:28Z

Any updates on this +1 ?

while this stuck for approval, you can do is append one element at top and then use scrollToIndex and put index = 1. it will maintain its position.
Also if the element hight is known, you should use scrollToOffset and put offset: elementHeight.

…s vertically

…s horizontally

… added to TypeScript

aweffr · 2023-08-21T03:14:07Z

hope someone to review this and get it merged.

irion94 · 2023-08-24T10:41:42Z

Hi there!
Has anyone tested it on Android using horizontal prop?

I found the app crashing due to incorrectness in alShadow.clearGapsAndOverlaps() method.
For my own purposes, I created a patch applying the original implementation of this method for horizontal lists to solve the problem but it's not a solution.

Btw. Those crashes occur even if the experimentalMaintainTopContentPosition prop isn't set.

Maybe I'm doing something wrong?

Below, is the brief of the error.

Thanks!

EDIT: idk why, it started working. 😱
EDIT2: False alarm 😝 , as I said, if experimentalMaintainTopContentPosition isn't set - the horizontal list crash
EDIT3: Also, if we set experimentalMaintainTopContentPosition, but we are using Reanimated.createAnimatedComponent(FlashList) - list items don't show up

java.lang.ClassCastException: com.facebook.react.views.scroll.ReactHorizontalScrollView cannot be cast to android.widget.ScrollView at com.shopify.reactnative.flash_list.AutoLayoutView.fixLayout(AutoLayoutView.kt:76) at com.shopify.reactnative.flash_list.AutoLayoutView.dispatchDraw(AutoLayoutView.kt:35) at android.view.View.updateDisplayListIfDirty(View.java:17297) at android.view.View.draw(View.java:18086) at android.view.ViewGroup.drawChild(ViewGroup.java:3966)

yayvery · 2023-08-24T16:19:18Z

@irion94 +1, also got that error testing this PR on Android in app with horizontal flashlists.

aweffr · 2023-08-30T02:09:21Z

I clone the repo https://github.com/friyiajr/flash-list/tree/maintainPositionAndroid and install it on ios, the FlashList encounter error once rendered.

The error is "TypeError (0 , PlatformHelper_1.getBidirectionalScrollView) is not a function"

This error is located at:
in FlashList (created by UIFlashListMessageContainer)
in RCTView (created by View)
in View (created by UIFlashListMessageContainer)

I fix it by export "getBidirectionalScrollView" function from both PlatformHelper.android.ts and PlatformHelper.ios.ts.

nandorojo · 2023-09-19T12:04:49Z

I'm willing to sponsor someone at Shopify to review and merge this. cc @naqvitalha feel free to DM me to set this up.

rkstar · 2023-12-18T19:22:48Z

happy holidays everyone! any update on this PR? i wish i could contribute instead of just coming in here and poking, but i literally don't have any more time in my days. 😭

mafintosh · 2024-02-28T08:40:37Z

Exciting stuff! Also willing to sponsor someone to get this landed if it helps.

robertontiu · 2024-02-28T10:40:40Z

This is so crucial to this library! Would love to see it implemented.

Freddy03h · 2024-04-25T09:14:09Z

Hi! Is there something new after one year? Thank you.

rkstar · 2024-04-25T14:48:13Z

please someone review and merge 🙏

irisjae · 2024-05-03T08:30:40Z

Super excited about this!

I've been testing out this change for a while now, and it's been mostly awesome. But when I tried to scroll while adding a new item, I noticed some jitters. I made a quick video to show what's going on. In the video, you'll notice I'm also firing off a new message every second. Honestly, I caught the worst scenario in that video. Sometimes, the jitters only happened while scrolling. So, yeah, that's what's been happening.
telegram-cloud-document-2-5424748569082999196.mp4

This is a limitation of the approach taken in this patch. Since recyclerlistview, which is used by flash-list, is responsible for layouting the list items, and recyclerlistview does not really support maintaining a "visible position" in its layouting algorithm, these issues cannot be solved without reimplementing the layout algorithm in recyclerlistview.

I have done so, and it seems to solve most issues that I have with flash-list, including choppy scrolling on bad size estimates, inaccurate scrollToIndex, and maintaining positions of visible items across data changes. I'll package it up in a bit after I clean the repo a bit more, and would be happy if you guys would want to test it.

irisjae · 2024-05-08T12:19:35Z

As I've alluded to, to maintain visible content position properly is related to choppy scrolling on bad size estimates and also inaccurate programmatic scrolling, and requires deeper changes to fix.

I've published an analysis of this problem and a patch here; feel free to test it out.

Note that instead of attempting to replicate ScrollView's maintainVisibleContentPosition prop, my patch creates a new prop preserveVisiblePosition instead.

friyiajr requested a review from naqvitalha April 24, 2023 22:55

friyiajr mentioned this pull request Apr 24, 2023

Add experimentalMaintainTopContentPosition support to FlashList #772

Closed

4 tasks

friyiajr force-pushed the maintainPositionAndroid branch from 1d653b3 to 556822a Compare April 24, 2023 23:02

friyiajr changed the title ~~[DF] feat(#547): add experimentalScrollPositionManagement support to FlashList~~ [DF] feat(#547): add experimentalScrollPositionManagement support to FlashList Apr 24, 2023

friyiajr force-pushed the maintainPositionAndroid branch from 556822a to 15e1f4c Compare April 27, 2023 19:29

hirbod mentioned this pull request May 5, 2023

[expo-image] recyclingKey breaks Blurhash placeholder size (FlashList) expo/expo#22206

Open

naqvitalha reviewed Jun 9, 2023

View reviewed changes

friyiajr added 3 commits August 8, 2023 08:20

[DF] feat(Shopify#547): experimentalMaintainTopContentPosition work…

711dd4f

…s vertically

[DF] feat(Shopify#547): experimentalMaintainTopContentPosition work…

f8409a1

…s horizontally

[DF] feat(Shopify#547): experimentalMaintainTopContentPosition prop…

8a9d280

… added to TypeScript

friyiajr added 8 commits August 8, 2023 08:21

[DF] feat(Shopify#547): maintain android list position

814d74d

Fix rebase

07122e9

Fix Rebase

bdafd16

Fix rebase

a3718f2

[DF] feat(Shopify#547): clean up kotlin code

f650e56

[DF] feat(Shopify#547): cleanup typescript code

a37e431

[DF] feat(Shopify#547): rename some variables

6cc3be7

[DF] feat(Shopify#547): fix kotlin pr requests

c044882

friyiajr force-pushed the maintainPositionAndroid branch 3 times, most recently from ddd4aaa to b677969 Compare August 16, 2023 17:51

[DF] feat(Shopify#547): fix typescript pr requests

788eb40

friyiajr force-pushed the maintainPositionAndroid branch from b677969 to 788eb40 Compare August 16, 2023 18:46

nandorojo mentioned this pull request Mar 19, 2024

Not show all text in IOS nandorojo/burnt#39

Open

irisjae mentioned this pull request May 8, 2024

Relative relayouting #1192

Open

6 tasks

perunt mentioned this pull request May 13, 2024

LOW: Migrate the main chat list to FlashList Expensify/App#33725

Open

irisjae mentioned this pull request May 25, 2024

maintainVisibleContentPosition Not Working #547

Open

2 tasks

nplasterer mentioned this pull request Aug 12, 2024

Tracking: Performance & Architecture ephemeraHQ/converse-app#504

Open

8 tasks

[DF] feat(#547): add experimentalScrollPositionManagement support to FlashList #824

Are you sure you want to change the base?

[DF] feat(#547): add experimentalScrollPositionManagement support to FlashList #824

Conversation

friyiajr commented Apr 24, 2023 • edited Loading

Description

Reviewers’ hat-rack 🎩

Screenshots or videos (if needed)

iOS Demo

Android Demo

Checklist

geo-vi commented Apr 29, 2023

DLkhazan commented Apr 30, 2023

DLkhazan commented Apr 30, 2023

HuuNguyen312 commented May 3, 2023

aweffr commented May 5, 2023

perunt commented May 20, 2023

terrysahaidak commented Jun 2, 2023 • edited Loading

rkstar commented Jun 8, 2023

naqvitalha commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkstar commented Jun 21, 2023

tauntmachine commented Jul 6, 2023

HuuNguyen312 commented Jul 6, 2023

DLkhazan commented Jul 17, 2023

HuuNguyen312 commented Jul 17, 2023

DLkhazan commented Jul 17, 2023

HuuNguyen312 commented Jul 17, 2023

DLkhazan commented Jul 17, 2023

chj-damon commented Jul 21, 2023

aweffr commented Jul 24, 2023

DLkhazan commented Jul 24, 2023

aweffr commented Aug 21, 2023

irion94 commented Aug 24, 2023 • edited Loading

yayvery commented Aug 24, 2023

aweffr commented Aug 30, 2023

nandorojo commented Sep 19, 2023 • edited Loading

rkstar commented Dec 18, 2023

mafintosh commented Feb 28, 2024

robertontiu commented Feb 28, 2024

Freddy03h commented Apr 25, 2024

rkstar commented Apr 25, 2024

irisjae commented May 3, 2024 • edited Loading

irisjae commented May 8, 2024 • edited Loading

[DF] feat(#547): add `experimentalScrollPositionManagement` support to FlashList #824

[DF] feat(#547): add `experimentalScrollPositionManagement` support to FlashList #824

friyiajr commented Apr 24, 2023 •

edited

Loading

terrysahaidak commented Jun 2, 2023 •

edited

Loading

irion94 commented Aug 24, 2023 •

edited

Loading

nandorojo commented Sep 19, 2023 •

edited

Loading

irisjae commented May 3, 2024 •

edited

Loading

irisjae commented May 8, 2024 •

edited

Loading