Add maintainVisibleContentPosition support on Android for legacy renderer #35049

janicduplessis · 2022-10-22T04:10:42Z

Summary

This adds support for maintainVisibleContentPosition on Android. The implementation is heavily inspired from iOS, it works by finding the first visible view and its frame before views are update, then adjusting the scroll position once the views are updated.

Most of the logic is abstracted away in MaintainVisibleScrollPositionHelper to be used in both vertical and horizontal scrollview implementations.

Note that this only works for the old architecture, I have a follow up ready to add fabric support.

Changelog

[Android] [Added] - Add maintainVisibleContentPosition support on Android

Test Plan

Test in RN tester example on Android

Screen.Recording.2022-10-22.at.00.32.27.mov

analysis-bot · 2022-10-22T04:27:52Z

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

Base commit: 8c2a4d0
Branch: main

analysis-bot · 2022-10-22T04:35:23Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,759,751	+658,847
android	hermes	armeabi-v7a	7,163,330	+692,730
android	hermes	x86	8,071,503	+552,756
android	hermes	x86_64	8,042,324	+664,951
android	jsc	arm64-v8a	9,614,859	+649,826
android	jsc	armeabi-v7a	8,380,813	+684,396
android	jsc	x86	9,561,682	+534,159
android	jsc	x86_64	10,154,595	+648,941

Base commit: 8c2a4d0
Branch: main

pull-bot · 2022-10-22T05:15:04Z

PR build artifact for c069ae3 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-10-22T05:15:21Z

PR build artifact for c069ae3 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

facebook-github-bot · 2022-10-24T17:15:32Z

@skinsshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mmmoussa · 2022-10-31T20:25:33Z

I have tested this PR out in my app (closed-source) which is using VirtualizedList with maintainVisibleContentPosition={{ minIndexForVisible: 0 }} and it is not working. The property works correctly for my use case on iOS and I was previously using a modified version of https://github.com/GetStream/flat-list-mvcp for Android support which was working. From my understanding this property only affects the ScrollView directly and all of the lists built on top such as VirtualizedList should not need any modification to integrate it, but please let me know if that understanding is incorrect and additional changes are required to support the lists built on top of ScrollView.

ryancat

Please take a look at the comment and make sure there's no race possible here.

ryancat · 2022-10-27T20:38:48Z

...droid/src/main/java/com/facebook/react/views/scroll/MaintainVisibleScrollPositionHelper.java

+    Rect newFrame = new Rect();
+    firstVisibleView.getHitRect(newFrame);
+
+    if (mHorizontal) {


If you could get rid of the branching logic for horizontal vs vertical scroll view, that would be great!

Branching and interface are two different ways we handle variants. Adding branching will usually be cleaner, but will create duplicate code easily. On the other hand, using interface to abstract the logic will be more challenging in coding, but the result is more flexible.
You may use ReactScrollViewHelper as an example. There's no branching there so duplication is reduced.

Do you have an example of the interface you have in mind?

My thinking when organizing the code like this was to reduce duplication between the 2 implementations while also moving all code related to maintain visible content position out of the scrollview classes to avoid making them too big. The common logic when the variant specific is also very intertwined in some cases, see https://github.com/facebook/react-native/pull/35049/files/c069ae3e39595e7d7e7a3259c6004d1313ad6ba8#diff-b0c8b7c192810637724d9d2ed35db2b8921c45e153ebbda1ef8f34cb53837473R153 for example.

ryancat · 2022-10-27T20:39:55Z

...droid/src/main/java/com/facebook/react/views/scroll/MaintainVisibleScrollPositionHelper.java

+      if (deltaX != 0) {
+        int scrollX = mScrollView.getScrollX();
+        mScrollView.scrollTo(scrollX + deltaX, mScrollView.getScrollY());
+        mPrevFirstVisibleFrame = newFrame;


If I understand correctly, the newFrame is the frame for the new firstVisibleView, and you've updated the scroll position back to the previous first visible frame. Why do you need to update the previous frame here?

I thought you want to keep the scroll position unchanged, which means you would preserve the first visible frame?

This is to avoid adjusting the scroll position multiple times. I had cases where this method would be called multiple times in a row.

ryancat · 2022-10-27T20:40:40Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java

@@ -107,6 +111,8 @@ public class ReactHorizontalScrollView extends HorizontalScrollView
  private PointerEvents mPointerEvents = PointerEvents.AUTO;
  private long mLastScrollDispatchTime = 0;
  private int mScrollEventThrottle = 0;
+  private @Nullable View mContentView;
+  private @Nullable MaintainVisibleScrollPositionHelper mMaintainVisibleContentPositionHelper = null;


By default it will be null, so this is technically not needed (https://stackoverflow.com/questions/16699593/uninitialized-object-vs-object-initialized-to-null)

ryancat · 2022-10-27T20:50:57Z

...droid/src/main/java/com/facebook/react/views/scroll/MaintainVisibleScrollPositionHelper.java

+      return;
+    }
+
+    View firstVisibleView = mFirstVisibleView.get();


updateScrollPosition is getting called on layout change. Is this always going to happen after computing mFirstVisibleView, which here it does in listener to willDispatchViewUpdates?

In theory I think it should, as we will dispatch the mounting instruction in JS thread or UI thread (when sync update) before the native platform to update the layout. However, since we are doing the computing on UI thread asynchronously, I am not sure if there will be a race condition where the first visible view is computed after we called this method.

cc @javache what do you think?

If willDispatchViewUpdates is called from ui thread (which I’m not sure if it ever does) the dispatch to ui thread will be done synchronously by runOnUIThread. Ideally we could have a way to observe before / after views are updated on the ui thread but wanted to avoid adding more methods to UIManagerObserver for old arch. In my tests this always worked so I was fine with this solution.

@janicduplessis firstVisibleView may be null, i ran into.
E/unknown:ReactNative: Exception in native call java.lang.NullPointerException: Attempt to invoke virtual method 'void android.view.View.getHitRect(android.graphics.Rect)' on a null object reference at com.facebook.react.views.scroll.MaintainVisibleScrollPositionHelper.updateScrollPositionInternal(MaintainVisibleScrollPositionHelper.java:107) at com.facebook.react.views.scroll.MaintainVisibleScrollPositionHelper.updateScrollPosition(MaintainVisibleScrollPositionHelper.java:97) at com.facebook.react.views.scroll.ReactScrollView.onLayoutChange(ReactScrollView.java:1128) at android.view.View.layout(View.java:20717) at android.view.ViewGroup.layout(ViewGroup.java:6198) at com.facebook.react.uimanager.NativeViewHierarchyManager.updateLayout(NativeViewHierarchyManager.java:254) at com.facebook.react.uimanager.NativeViewHierarchyManager.updateLayout(NativeViewHierarchyManager.java:222)

I see, yes it is missing a null check, I'll open a PR to fix it.

aweffr · 2022-11-11T05:24:46Z

any progress on this?

pull-bot · 2022-11-12T17:03:00Z

PR build artifact for 2764db0 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-12T17:03:17Z

PR build artifact for 2764db0 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

facebook-github-bot · 2022-11-15T21:00:34Z

@skinsshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

janicduplessis · 2022-12-02T18:35:39Z

@skinsshark @ryancat Any updates on this? I think the only unaddressed point is #35049 (comment).

areebbajwa · 2022-12-12T21:01:24Z

Need an update on this please.

paula-morales · 2022-12-27T16:00:07Z

Any update? 🙏

codal-mpawar · 2023-01-02T06:34:15Z

ANy update on this?

skinsshark · 2023-01-03T23:03:49Z

hey all! thank you for your patience and sorry for the delay, there are a couple things we want to test internally but haven't had the bandwidth just yet- i will get this prioritized and be back with updates! happy new year!

siu666 · 2023-01-07T17:40:27Z

@pull-bot could yours rebuild the tarball in CircleCI ?, cause it had been removed after 30days , thx~~~

matt-alice · 2023-01-12T11:17:51Z

Hi, many happy returns and thanks for the update.
Is there any rough estimate RE: when this will be released? could it happen this month or next month?
Many thanks for your help.

areebbajwa · 2023-01-12T15:58:39Z

I would like to know that as well

janicduplessis · 2023-01-24T17:20:44Z

Thanks @skinsshark !!

facebook/react-native#35049 added support for this on Android

* Update platform label for `maintainVisibleContentPosition` facebook/react-native#35049 added support for this on Android * Update scrollview.md

hadnet · 2023-04-22T02:06:36Z

So this maintanVisibleContentPosition prop is now available on what version of RN? And is available on FlatList as well, right?

janicduplessis · 2023-04-22T05:36:34Z

It is available in 0.72. It also works with FlatList but there are a few known issues that will be fixed in #35993 which isn’t merged yet.

Summary: This adds support for `maintainVisibleContentPosition` on Android. The implementation is heavily inspired from iOS, it works by finding the first visible view and its frame before views are update, then adjusting the scroll position once the views are updated. Most of the logic is abstracted away in MaintainVisibleScrollPositionHelper to be used in both vertical and horizontal scrollview implementations. Note that this only works for the old architecture, I have a follow up ready to add fabric support. ## Changelog  [Android] [Added] - Add maintainVisibleContentPosition support on Android Pull Request resolved: facebook#35049 Test Plan: Test in RN tester example on Android https://user-images.githubusercontent.com/2677334/197319855-d81ced33-a80b-495f-a688-4106fc699f3c.mov Reviewed By: ryancat Differential Revision: D40642469 Pulled By: skinsshark fbshipit-source-id: d60f3e2d0613d21af5f150ca0d099beeac6feb91

hantrungkien · 2023-06-16T03:44:08Z

@janicduplessis I have tried it in 0.72-rc06. Everything seems fine but I found a problem with my chat app with scenario:

In first request I load message from 30 to 0 and show it on FlatList with reverse.
In second request I load message from 35-31 and append to leading page.

If all messages are short content then maintainVisibleContentPosition is working fine. But messages from 35-31 is very long text (greater than 3000 char) then FlatList still auto scroll to first position.

Do you have any suggestions for me?

…38891) Summary: `mFirstVisibleView` is a weak ref so it can also be null when dereferencing. This was reported on the original PR here #35049 (comment) ## Changelog: [ANDROID] [FIXED] - Fix null crash when using maintainVisibleContentPosition on Android Pull Request resolved: #38891 Test Plan: Not sure exactly in what cases this can happen, but the fix is trivial and makes sense. Reviewed By: cortinico Differential Revision: D48192154 Pulled By: rshest fbshipit-source-id: 57a38a22a0e216a33603438355bde0013c014fbf

haileyok · 2024-02-02T22:03:56Z

@hantrungkien did you ever experiment further with that? thanks!

janicduplessis · 2024-02-03T01:43:29Z

We also have noticed some issues on android, I will look at it soon.

haileyok · 2024-02-05T21:38:14Z

@janicduplessis Following up in case this is helpful, I can open a new issue too:

https://snack.expo.dev/@haileyok/flatlist-repro

/**
 * To repro:
 * - On initial render, we can add 10 or 20 items to the list without issue while maxToRenderPerBatch is set to 20.
 * - If we increase this number to 30, we will get a jump in position after adding the items above.
 * - If we then increase the maxToRenderPerBatch to 30, we can add 30 items above without issue.
 * 
 * Notice that the maxToRenderPerBatch seems to need to be set to >= the number of items we are adding.
 * 
 * Notably, after resetting the list, the issue isn't present anymore. It is only on the initial render that we see problems.
 */

Video: https://streamable.com/almoal

janicduplessis · 2024-02-06T01:55:09Z

Thanks, this will help, planning to look at it this week.

janicduplessis · 2024-02-06T01:55:23Z

No need for an issue.

perunt · 2024-02-06T18:38:03Z

@haileyok, have you tried adding half the number of initial items? Also, as you mentioned, the maxToRenderPerBatch should not be less than the number of items you're adding.

haileyok · 2024-02-06T18:43:26Z

@haileyok, have you tried adding half the number of initial items? Also, as you mentioned, the maxToRenderPerBatch should not be less than the number of items you're adding.

my experience was that the number of initial items didn't affect anything. IIRC, even with just one initial item (and some padding on the contentContainerStyle to account for the needed space to maintain that position) we would still get a jump. If the item at index 0 (or I presume whatever the min index is set to) is the same height as the items being prepended though, there is never a jump regardless of any of the configuration.

I suppose my immediate question would be is this something that's a bug/could be fixed or is this a limit of the virtualization (I don't know all the specifics on how the virtualization works, just a quick review of the code here)?

I'm implementing a workaround right now to artificially "load in" items when we scroll up, which is a fine solution (and maybe better anyway for performance so we don't immediately load a bunch of items) but would still be interested in knowing if this could be fixed.

Appreciate it!

janicduplessis · 2024-02-06T18:57:11Z

For more context:

Usually the problem with maintainVisibleContentPosition and virtualization is that maintainVisibleContentPosition relies on tracking the first visible view to maintain its position when items are added. Then what can happen is that the first visible view is virtualized away and replaced with a spacer if too many items are added and the old first visible view is pushed out of the virtualization window.

We added some mitigations to try and prevent this from happening, but for some reason it seems to still happens some times on Android.

janicduplessis · 2024-02-06T19:46:52Z

@haileyok I just realized the issue you mentionned happens on iOS. It actually looks like an issue I addressed recently. Sadly I don't think the changes haven't made it into a release yet.

Can you try to apply the changes from this 3ed4bf9. This code should be in the @react-native/virtualized-lists package.

janicduplessis · 2024-02-06T19:48:11Z

The issue we are currently experiencing seems to happen only on Android if adding items at the start of the list during scrolling.

haileyok · 2024-02-06T19:55:37Z

Ah, awesome! I'll look later if I have time today or tomorrow if not and let you know. Appreciate the quick response. I take it this addresses both platforms since it's just the JS side?

janicduplessis · 2024-02-06T21:15:44Z

@haileyok It does yes

janicduplessis · 2024-02-06T21:33:15Z

There is also one more case of sliding that I know of. It happens when items size vary a lot. Internally VirtualizedList uses the average size of previously measured items to estimate the size of new items being added before they get measured and the actual size can be used.

In your repro if I increase number of item added to 60 it starts happening.

miafoo · 2024-02-27T11:48:57Z

There is also one more case of sliding that I know of. It happens when items size vary a lot. Internally VirtualizedList uses the average size of previously measured items to estimate the size of new items being added before they get measured and the actual size can be used.

I'm running into this as well unfortunately. Do you have an issue or PR tracking this, or will you keep us posted here? Just want to stay updated for when it's fixed 💜

haileyok · 2024-02-27T17:59:35Z

@miafoo Can't give you any updates on that status, but what I can pass over is a bit of how we worked around the problem.

One way that you might be able to get around this though is to only add items to the top of the list in increments. If you use onStartReached, you can achieve a bit of a "loading" effect, even if you already have all the items and don't have to fetch them. We did this here https://github.com/bluesky-social/social-app/blob/58aaad704aa971c5ebbf5a5f330a2e2129b557f6/src/view/com/post-thread/PostThread.tsx#L280.

janicduplessis · 2024-03-12T03:28:08Z

@haileyok I opened a PR to fix another issue on Android #43425

wqhui · 2024-10-22T08:18:22Z

awesome! Can 70.0.x still be supported? The react-native upgrade version is too difficult...

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Oct 22, 2022

react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Oct 22, 2022

Add maintainVisibleContentPosition prop for android scroll view

c069ae3

janicduplessis force-pushed the @janic/mvcp-android branch from 47f93e1 to c069ae3 Compare October 22, 2022 04:40

This was referenced Oct 24, 2022

maintainVisibleContentPosition is not available on android #29055

Closed

Support for ScrollView.maintainVisibleContentPosition on Android #29466

Closed

ryancat reviewed Oct 31, 2022

View reviewed changes

CptMaumau mentioned this pull request Nov 7, 2022

load new messages and keep scroll position FaridSafi/react-native-gifted-chat#1603

Closed

Choyeongdeok mentioned this pull request Nov 11, 2022

FlatList automatically scrolls after adding new items #25239

Open

Update ReactHorizontalScrollView.java

2764db0

janicduplessis deleted the @janic/mvcp-android branch January 24, 2023 17:20

lunaleaps changed the title ~~Add maintainVisibleContentPosition support on Android~~ Add maintainVisibleContentPosition support on Android for legacy renderer Mar 21, 2023

javache added a commit to facebook/react-native-website that referenced this pull request Apr 13, 2023

Update platform label for maintainVisibleContentPosition

bd8f4c1

facebook/react-native#35049 added support for this on Android

javache mentioned this pull request Apr 13, 2023

Update platform label for maintainVisibleContentPosition facebook/react-native-website#3682

Merged

janicduplessis mentioned this pull request Aug 9, 2023

Fix null crash when using maintainVisibleContentPosition on Android #38891

Closed

Add maintainVisibleContentPosition support on Android for legacy renderer #35049

Add maintainVisibleContentPosition support on Android for legacy renderer #35049

Conversation

janicduplessis commented Oct 22, 2022 • edited Loading

Summary

Changelog

Test Plan

analysis-bot commented Oct 22, 2022 • edited Loading

analysis-bot commented Oct 22, 2022 • edited Loading

pull-bot commented Oct 22, 2022

pull-bot commented Oct 22, 2022

facebook-github-bot commented Oct 24, 2022

mmmoussa commented Oct 31, 2022

ryancat left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweffr commented Nov 11, 2022

pull-bot commented Nov 12, 2022

pull-bot commented Nov 12, 2022

facebook-github-bot commented Nov 15, 2022

janicduplessis commented Dec 2, 2022 • edited Loading

areebbajwa commented Dec 12, 2022

paula-morales commented Dec 27, 2022

codal-mpawar commented Jan 2, 2023

skinsshark commented Jan 3, 2023

siu666 commented Jan 7, 2023

matt-alice commented Jan 12, 2023

areebbajwa commented Jan 12, 2023

janicduplessis commented Jan 24, 2023

hadnet commented Apr 22, 2023 • edited Loading

janicduplessis commented Apr 22, 2023

hantrungkien commented Jun 16, 2023

haileyok commented Feb 2, 2024

janicduplessis commented Feb 3, 2024

haileyok commented Feb 5, 2024 • edited Loading

janicduplessis commented Feb 6, 2024

janicduplessis commented Feb 6, 2024

perunt commented Feb 6, 2024

haileyok commented Feb 6, 2024

janicduplessis commented Feb 6, 2024

janicduplessis commented Feb 6, 2024

janicduplessis commented Feb 6, 2024

haileyok commented Feb 6, 2024

janicduplessis commented Feb 6, 2024

janicduplessis commented Feb 6, 2024

miafoo commented Feb 27, 2024

haileyok commented Feb 27, 2024

janicduplessis commented Mar 12, 2024

wqhui commented Oct 22, 2024

janicduplessis commented Oct 22, 2022 •

edited

Loading

analysis-bot commented Oct 22, 2022 •

edited

Loading

analysis-bot commented Oct 22, 2022 •

edited

Loading

janicduplessis commented Dec 2, 2022 •

edited

Loading

hadnet commented Apr 22, 2023 •

edited

Loading

haileyok commented Feb 5, 2024 •

edited

Loading