Add maintainVisibleContentPosition support on Android for legacy renderer by janicduplessis · Pull Request #35049 · react/react-native

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

+ 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

+ 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

 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

+ 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.

@janicduplessisfirstVisibleView 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 !!

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

* Update platform label for `maintainVisibleContentPosition` react/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.

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 c069ae3Compare 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
Closed

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 rendererMar 21, 2023

javache added a commit to react/react-native-website that referenced this pull request Apr 13, 2023
Update platform label for maintainVisibleContentPosition
bd8f4c1
react/react-native#35049 added support for this on Android

javache mentioned this pull request Apr 13, 2023
Update platform label for maintainVisibleContentPositionreact/react-native-website#3682
Merged

janicduplessis mentioned this pull request Aug 9, 2023
Fix null crash when using maintainVisibleContentPosition on Android #38891
Closed

Uh oh!

Conversation

janicduplessis commented Oct 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Oct 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Oct 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pull-bot commented Oct 22, 2022

Uh oh!

pull-bot commented Oct 22, 2022

Uh oh!

facebook-github-bot commented Oct 24, 2022

Uh oh!

mmmoussa commented Oct 31, 2022

Uh oh!

ryancat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweffr commented Nov 11, 2022

Uh oh!

pull-bot commented Nov 12, 2022

Uh oh!

pull-bot commented Nov 12, 2022

Uh oh!

facebook-github-bot commented Nov 15, 2022

Uh oh!

janicduplessis commented Dec 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

areebbajwa commented Dec 12, 2022

Uh oh!

paula-morales commented Dec 27, 2022

Uh oh!

codal-mpawar commented Jan 2, 2023

Uh oh!

skinsshark commented Jan 3, 2023

Uh oh!

siu666 commented Jan 7, 2023

Uh oh!

matt-alice commented Jan 12, 2023

Uh oh!

areebbajwa commented Jan 12, 2023

Uh oh!

janicduplessis commented Jan 24, 2023

Uh oh!

hadnet commented Apr 22, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janicduplessis commented Apr 22, 2023

Uh oh!

hantrungkien commented Jun 16, 2023

Uh oh!

haileyok commented Feb 2, 2024

Uh oh!

janicduplessis commented Feb 3, 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