Android support for maintainVisibleContentPosition #14

janicduplessis · 2022-07-25T22:32:18Z

Add android support for maintainVisibleContentPosition

janicduplessis · 2022-07-25T22:33:48Z

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

-      if (prevChild != null && prevChild.getGlobalVisibleRect(new Rect())) {
-        max = mid;
-        continue;
+    int minIndex = mMaintainVisibleContentPositionData.minIndexForVisible;


I had some issues with binary search infinite looping so I instead of debugging it I just ported the iOS logic pretty much line for line. I think this should be ok.

Julesssss · 2022-07-26T13:02:02Z

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

@@ -20,6 +20,7 @@
 import android.graphics.Rect;
 import android.graphics.drawable.ColorDrawable;
 import android.graphics.drawable.Drawable;
+import android.util.Log;


I guess this is still WIP, but leaving a note that we should remove this :)

Julesssss · 2022-07-26T13:03:01Z

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

@@ -103,6 +114,9 @@ public class ReactScrollView extends ScrollView
      new ReactScrollViewScrollState(ViewCompat.LAYOUT_DIRECTION_LTR);
  private final ValueAnimator DEFAULT_FLING_ANIMATOR = ObjectAnimator.ofInt(this, "scrollY", 0, 0);
  private PointerEvents mPointerEvents = PointerEvents.AUTO;
+  private @Nullable ReactScrollViewMaintainVisibleContentPositionData mMaintainVisibleContentPositionData;


Any reason to not initialize null?

This is correct, this holds the value of the maintainVisibleContent prop, which is null by default.

Oh I read this wrong, by default it will be initialized to null, so it isn’t needed, although we could add it to be more explicit

Not a big deal either way, might look a bit more consistent though.

Julesssss · 2022-07-26T13:10:30Z

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

@@ -228,6 +242,16 @@ public void setOverflow(String overflow) {
    invalidate();
  }

+  public void setMaintainVisibleContentPosition(ReactScrollViewMaintainVisibleContentPositionData maintainVisibleContentPositionData) {
+    if (maintainVisibleContentPositionData != null && mMaintainVisibleContentPositionData == null) {
+      getUIManagerModule().addUIManagerEventListener(this);


Just to clarify, we're using the value of maintainVisibleContentPositionData to register/unregister the listener? I'm curious why this is preferred instead of tying it to the class/lifecycle? (i.e class initialisation).

Is this because the invoked callback is expensive?

We want to avoid the overhead of tge listener if the prop is not set, iOS had this logic so that is mainly why I added it too.

Ah I see, that makes sense 👍

Julesssss · 2022-07-26T13:40:43Z

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

@@ -991,6 +1015,80 @@ private void setPendingContentOffsets(int x, int y) {
    }
  }

+  private UIManagerModule getUIManagerModule() {
+    ReactContext reactContext = UIManagerHelper.getReactContext(this);
+    return Assertions.assertNotNull(reactContext.getNativeModule(UIManagerModule.class));


I'm not familiar with ReactSoftExceptionLogger, so wanted to check that our assertion is treated as soft exception, instead of crashing the app?

This logs an assertion with ReactSoftExceptionLogger, which decides whether or not to actually throw.

Hard crash should be fine as I think it can never be null when this is called while setting props.

Julesssss · 2022-07-26T13:43:16Z

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

+      return;
+    }
+
+    ReactViewGroup contentView = (ReactViewGroup) mContentView;


Again a minor point. But I wonder if we could improve this by casting once and holding a reference to ReactViewGroup, instead of casting every time we compute.

I might be able to change mContentView to ReactViewGroup. Also wanna double check if the cast is actually needed.

I think I would rather keep the cast here, to avoid having to change mContentView and having to cast it somewhere else. Just to keep the diff as small as possible. I don't the cast should really have any impact in this case.

Julesssss · 2022-07-26T13:50:44Z

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

@@ -1011,6 +1109,8 @@ public void onLayoutChange(
      return;
    }

+    updateScrollPositionForMaintainVisibleContentPosition();


Asking mainly for my understanding, but I wanted to check that the following lines should be invoked if updateScrollPositionForMaintainVisibleContentPosition has already initiated a scrollTo event.

Yes, there are cases when removing views where even after adjustments we want to do the other adjustment (in the following lines) to make sure scroll position is not outside of the content view. Since we call getScrollY again it should already use the updated value from updateScrollPositionForMaintainVisibleContentPosition.

Of course in this case the scroll position is no longer maintained, but this is correct behavior. Basically whenever there's less content than the scroll view height, position is not maintained.

janicduplessis · 2022-07-26T17:56:28Z

This should work for our use case, but missing a few things to upstream:

Support for horizontal scroll view
Support autoscrollToTopThreshold

janicduplessis · 2022-07-26T20:05:59Z

Updated to target 0.69-stable

roryabraham · 2022-07-29T23:20:46Z

So without diving too much into the code – I tested this on a physical Pixel 4a with Fabric enabled, and found that many of the vertical ScrollView components would not scroll. Looking for a pattern now.

Edit: Ignore this ... I was already testing on 0.69-stable so this is a serious fabric bug but not one caused by any of this code

Edit 2: Just needed to add the nestedScrollEnabled prop 🤦 After adding that this is working great!

roryabraham · 2022-07-29T23:26:31Z

We'll want to move the example up so that it renders on iOS and Android

roryabraham

Overall works great, just a few things we should do before we merge it:

Include the maintainVisibleContentPosition example on Android as well as iOS
Pass the nestedScrollEnabled prop to child scroll views in rn-tester/js/examples/ScrollView.js as needed

I also think we should work on an implementation of autoScrollToTopThreshold ASAP, because there's a good chance we might want to use that in our app. We'll need that and horizontal scrolling implemented when we go to upstream, anyways.

roryabraham · 2022-07-30T00:02:42Z

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

+    for (int i = minIndex; i < contentView.getChildCount(); i++) {
+      View child = contentView.getChildAt(i);
+      if (child.getY() > currentScrollY || i == contentView.getChildCount() - 1) {
+        mFirstVisibleViewForMaintainVisibleContentPosition = new WeakReference<>(child);


Do you know why a WeakReference is needed here? I included it in my draft basically copying from an existing draft implementation in an open RN PR, but @Julesssss and I weren't sure if it was actually needed.

Not sure, I also kept it because it was there in your implementation. I think this is good to avoid retaining the first visible view in the case it gets removed from the scrollview.

roryabraham · 2022-07-30T00:11:19Z

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

+
+  @Override
+  public void willDispatchViewUpdates(final UIManager uiManager) {
+    UiThreadUtil.runOnUiThread(


NAB but I think we could use a lambda here to be a bit more concise:

UiThreadUtil.runOnUiThread(() -> { if (mContentView == null) { return; } computeTargetViewForMaintainVisibleContentPosition(); });

roryabraham · 2022-07-30T00:59:48Z

According to my testing, this unfortunately does not work with Fabric enabled (though, I don't think the iOS implementation does either). No worries here though, that's a problem for our future selves.

roryabraham

This looks and tests great! I don't love how much duplication there is between ReactScrollView and ReachHorizontalScrollView, but that's not on us and I won't ask you to DRY this up any further. Nice job creating the MaintainVisibleScrollPositionHelper.

My only comment is that it should probably be called MaintainVisibleContentPositionHelper to match the name of the prop. We can always adjust that with a follow-up and in the upstream PR though.

roryabraham · 2022-08-04T20:44:31Z

I renamed the file, tested, and just pushed the change directly to the 0.69-stable branch in 807d6e7

janicduplessis changed the title ~~Expensify mvcp~~ Android support for maintainVisibleContentPosition v2 Jul 25, 2022

janicduplessis commented Jul 25, 2022

View reviewed changes

Julesssss reviewed Jul 26, 2022

View reviewed changes

janicduplessis force-pushed the expensify-mvcp branch from 113f2f5 to c4b4a83 Compare July 26, 2022 20:04

janicduplessis changed the base branch from Rory-latest to 0.69-stable July 26, 2022 20:05

janicduplessis changed the title ~~Android support for maintainVisibleContentPosition v2~~ Android support for maintainVisibleContentPosition Jul 27, 2022

roryabraham assigned janicduplessis Jul 29, 2022

roryabraham requested changes Jul 30, 2022

View reviewed changes

janicduplessis force-pushed the expensify-mvcp branch from 2779082 to deff396 Compare August 4, 2022 05:29

Add maintainVisibleContentPosition prop for android scroll view

5d49268

janicduplessis force-pushed the expensify-mvcp branch from deff396 to 5d49268 Compare August 4, 2022 05:31

roryabraham approved these changes Aug 4, 2022

View reviewed changes

roryabraham merged commit fae196a into Expensify:0.69-stable Aug 4, 2022

roryabraham mentioned this pull request Sep 22, 2022

Support for ScrollView.maintainVisibleContentPosition on Android facebook/react-native#29466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android support for maintainVisibleContentPosition #14

Android support for maintainVisibleContentPosition #14

janicduplessis commented Jul 25, 2022 •

edited

Loading

janicduplessis Jul 25, 2022

Julesssss Jul 26, 2022

janicduplessis Jul 26, 2022

Julesssss Jul 26, 2022 •

edited

Loading

janicduplessis Jul 26, 2022

janicduplessis Jul 26, 2022

Julesssss Jul 26, 2022

Julesssss Jul 26, 2022

janicduplessis Jul 26, 2022

Julesssss Jul 26, 2022

Julesssss Jul 26, 2022 •

edited

Loading

janicduplessis Jul 26, 2022

Julesssss Jul 26, 2022

janicduplessis Jul 26, 2022

janicduplessis Jul 27, 2022

Julesssss Jul 26, 2022

janicduplessis Jul 26, 2022 •

edited

Loading

janicduplessis commented Jul 26, 2022

janicduplessis commented Jul 26, 2022

roryabraham commented Jul 29, 2022 •

edited

Loading

roryabraham commented Jul 29, 2022

roryabraham left a comment

roryabraham Jul 30, 2022

janicduplessis Jul 30, 2022

roryabraham Jul 30, 2022 •

edited

Loading

roryabraham commented Jul 30, 2022

roryabraham left a comment

roryabraham commented Aug 4, 2022

Android support for maintainVisibleContentPosition #14

Android support for maintainVisibleContentPosition #14

Conversation

janicduplessis commented Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julesssss Jul 26, 2022 • edited Loading

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

Julesssss Jul 26, 2022 • edited Loading

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

janicduplessis Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

janicduplessis commented Jul 26, 2022

janicduplessis commented Jul 26, 2022

roryabraham commented Jul 29, 2022 • edited Loading

roryabraham commented Jul 29, 2022

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

roryabraham commented Jul 30, 2022

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham commented Aug 4, 2022

janicduplessis commented Jul 25, 2022 •

edited

Loading

Julesssss Jul 26, 2022 •

edited

Loading

Julesssss Jul 26, 2022 •

edited

Loading

janicduplessis Jul 26, 2022 •

edited

Loading

roryabraham commented Jul 29, 2022 •

edited

Loading

roryabraham Jul 30, 2022 •

edited

Loading