react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Support for `ScrollView.maintainVisibleContentPosition` on Android

Open maxoumime opened this issue 4 years ago • 54 comments

Summary

This PR adds the support for ScrollView's maintainVisibleContentPosition property to Android. This property is currently only available on iOS and is especially useful for chat-like scrollviews, where you want the scroll position to stick after layout changes.

Fixes #29055

This PR will impact the documentation, so I opened a draft PR on facebook/react-native-website#2088.

Changelog

[Android] [Added] - ScrollView.maintainVisibleContentPosition. This property is not iOS-only anymore.

Test Plan

Most of the new code is based on the iOS code. The implementation differs a bit but is working great. You can try it out on the RNTester app, I added a new example called ScrollViewExpandingExample, available both on Android and iOS. GIFs below.

react-horizontal-compressed react-vertical-compressed

maxoumime avatar Jul 22 '20 09:07 maxoumime

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,196,911 +6,442
android hermes armeabi-v7a 8,722,544 +6,440
android hermes x86 9,638,946 +6,443
android hermes x86_64 9,606,540 +6,448
android jsc arm64-v8a 10,832,618 +6,166
android jsc armeabi-v7a 9,749,519 +6,170
android jsc x86 10,870,053 +6,173
android jsc x86_64 11,479,152 +6,171

Base commit: 9b4f8e01442356f820e135fae3849063b2b8c92c

analysis-bot avatar Jul 22 '20 10:07 analysis-bot

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

Base commit: 9b4f8e01442356f820e135fae3849063b2b8c92c

analysis-bot avatar Jul 22 '20 10:07 analysis-bot

Here is an alternative native module implementation, for those who need maintainVisibleContentPosition on Android now but don't want to maintain your own RN fork.

package com.yourapp;

import android.view.View;

import com.facebook.react.bridge.Promise;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.bridge.ReactContextBaseJavaModule;
import com.facebook.react.bridge.ReactMethod;
import com.facebook.react.uimanager.IllegalViewOperationException;
import com.facebook.react.uimanager.NativeViewHierarchyManager;
import com.facebook.react.uimanager.ReactShadowNode;
import com.facebook.react.uimanager.UIBlock;
import com.facebook.react.uimanager.UIImplementation;
import com.facebook.react.uimanager.UIManagerModule;
import com.facebook.react.uimanager.UIManagerModuleListener;
import com.facebook.react.views.scroll.ReactScrollView;
import com.facebook.react.views.view.ReactViewGroup;

import java.util.HashMap;

public class ScrollViewMagicModule extends ReactContextBaseJavaModule {

  private final ReactApplicationContext reactContext;
  private HashMap<Integer, UIManagerModuleListener> uiManagerModuleListeners;

  public ScrollViewMagicModule(ReactApplicationContext reactContext) {
    super(reactContext);
    this.reactContext = reactContext;
  }

  @Override
  public String getName() {
    return "ScrollViewMagic";
  }

  @Override
  public void initialize() {
    super.initialize();
    this.uiManagerModuleListeners = new HashMap<>();
  }

  @ReactMethod
  public void enableMaintainVisibleContentPosition(final int viewTag, final Promise promise) {
    final UIManagerModule uiManagerModule = this.reactContext.getNativeModule(UIManagerModule.class);
    this.reactContext.runOnUiQueueThread(new Runnable() {
      @Override
      public void run() {
        try {
          final ReactScrollView scrollView = (ReactScrollView)uiManagerModule.resolveView(viewTag);
          final UIManagerModuleListener uiManagerModuleListener = new UIManagerModuleListener() {
            private int minIndexForVisible = 0;
            private int prevFirstVisibleTop = 0;
            private View firstVisibleView = null;
            @Override
            public void willDispatchViewUpdates(final UIManagerModule uiManagerModule) {
              uiManagerModule.prependUIBlock(new UIBlock() {
                @Override
                public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) {
                  ReactViewGroup mContentView = (ReactViewGroup)scrollView.getChildAt(0);
                  if (mContentView == null) return;
                  for (int ii = minIndexForVisible; ii < mContentView.getChildCount(); ++ii) {
                    View subview = mContentView.getChildAt(ii);
                    if (subview.getTop() >= scrollView.getScrollY()) {
                      prevFirstVisibleTop = subview.getTop();
                      firstVisibleView = subview;
                      break;
                    }
                  }
                }
              });

              UIImplementation.LayoutUpdateListener layoutUpdateListener = new UIImplementation.LayoutUpdateListener() {
                @Override
                public void onLayoutUpdated(ReactShadowNode root) {
                  if (firstVisibleView == null) return;
                  int deltaY = firstVisibleView.getTop() - prevFirstVisibleTop;
                  if (Math.abs(deltaY) > 0) {
                    scrollView.setScrollY(scrollView.getScrollY() + deltaY);
                  }
                  uiManagerModule.getUIImplementation().removeLayoutUpdateListener();
                }
              };

              uiManagerModule.getUIImplementation().setLayoutUpdateListener(layoutUpdateListener);
            }
          };
          uiManagerModule.addUIManagerListener(uiManagerModuleListener);
          int key = uiManagerModuleListeners.size() + 1;
          uiManagerModuleListeners.put(key, uiManagerModuleListener);
          promise.resolve(key);
        } catch(IllegalViewOperationException e) {
          promise.resolve(-1);
        }
      }
    });
  }

  @ReactMethod
  public void disableMaintainVisibleContentPosition(int key, Promise promise) {
    if (key >= 0) {
      final UIManagerModule uiManagerModule = this.reactContext.getNativeModule(UIManagerModule.class);
      uiManagerModule.removeUIManagerListener(uiManagerModuleListeners.remove(key));
    }
    promise.resolve(null);
  }
}

JS side:

const nativeModule: {
  enableMaintainVisibleContentPosition(viewTag: number): Promise<number>;
  disableMaintainVisibleContentPosition(handle: number): Promise<void>;
} = NativeModules.ScrollViewMagic;
  // `ref` is the ref to your ScrollView / FlatList
  useEffect(() => {
    let cleanupPromise: Promise<number> | undefined;
    if (Constants.isAndroid) {
      const viewTag = findNodeHandle(ref.current);
      cleanupPromise = nativeModule.enableMaintainVisibleContentPosition(
        viewTag
      );
    }
    return () => {
      void cleanupPromise?.then((handle) => {
        void nativeModule.disableMaintainVisibleContentPosition(handle);
      });
    };
  }, [ref]);

stackia avatar Jul 27 '20 12:07 stackia

Hi @chrisglein I saw you roaming around issue #29055 that this PR aims to fix. Since you are a contributor on this repo do you know if there are any next steps I'm missing to get this PR merged? It's been opened for a bit more than a month now and we'd like to have this scroll improvement for our app.

maxoumime avatar Sep 09 '20 09:09 maxoumime

Hi @chrisglein I saw you roaming around issue #29055 that this PR aims to fix. Since you are a contributor on this repo do you know if there are any next steps I'm missing to get this PR merged? It's been opened for a bit more than a month now and we'd like to have this scroll improvement for our app.

I'm mainly acting as an issue first responder, helping logged issues have the best chance of success. I've tagged the issue for this to see if that helps it get attention. You may also want to ping the discord to see if you can get some eyes on this.

chrisglein avatar Sep 12 '20 01:09 chrisglein

Thank you Chris! I joined the Discord 🤞

maxoumime avatar Sep 12 '20 06:09 maxoumime

I would prefer @stackia's implementation, because it matches iOS implementation, and do not use debounce, which leads to possible lags in implementation

Strate avatar Sep 17 '20 09:09 Strate

And another thing to check: on android just calling setScrollY() does not affect scroll animation - if list scrolling by inertia via OverScroller class, altering scrollY on ScrollView does not affects internal values in android.widget.OverScroller - so you need to stop it and re-launch again.

Strate avatar Sep 17 '20 10:09 Strate

Note to anyone using @stackia's implementation (thank you @stackia!) with a FlatList: if you add more than (windowSize-1)/2 items to the top of your data, it loses the scroll position (because the previously visible item will have been reclaimed). You can either increase your window size, or ~~add data in (windowSize/2 - 1) sized chunks in a useEffect hook~~. (Turns out the adding-in-chunks thing was just wishful thinking.)

I have no idea if the same problem holds for the code in this pull request, as I have not tried to use it yet.

jjd314 avatar Oct 01 '20 13:10 jjd314

@maxoumime, have you given up on getting this merged? If not, have you seen @Strate's comments regarding @stackia's implementation? Curious your thoughts.

tslater avatar Nov 08 '20 05:11 tslater

@shergin any plans of merging this?

vishalnarkhede avatar Dec 02 '20 20:12 vishalnarkhede

@stackia Thanks for the solution provided. Is it okay if I use the code to publish it as npm package? It will be useful for other devs as well who don't want to get into native module setup :)

vishalnarkhede avatar Dec 03 '20 10:12 vishalnarkhede

@stackia Thanks for the solution provided. Is it okay if I use the code to publish it as npm package? It will be useful for other devs as well who don't want to get into native module setup :)

It'll be great to publish as a much-easier-to-use npm package!

stackia avatar Dec 03 '20 10:12 stackia

Awesome!! I will update here once it is ready

vishalnarkhede avatar Dec 03 '20 10:12 vishalnarkhede

@stackia I have published an easy to use npm package that can be used as an alternative until react-native provides built-in fix for this. I have managed to test it properly for FlatList. Hopefully it will help some devs here :)

  • https://www.npmjs.com/package/@stream-io/flat-list-mvcp
  • https://github.com/GetStream/flat-list-mvcp

(PRs are welcome)

vishalnarkhede avatar Dec 09 '20 13:12 vishalnarkhede

if you include the following text in your pr summary

fixes https://github.com/facebook/react-native/issues/29055

it will be linked to the relevant issue and other developers will be able to filter issues without an open pr. Thanks a lot for the great work

fabOnReact avatar Feb 05 '21 08:02 fabOnReact

Hi all, sorry for the (long) delay here.

@Strate I believe we need to debounce so we don't search for the first visible view everytime onScrollChanged gets called. Removing it probably wouldn't impact most of the apps, but I don't think we should try it. About reactScrollTo (not setScrollY) affecting the OverScroller, I didn't notice this issue but I agree it might happen in some scenarios. I don't think this PR should aim at fixing this though.

I like @stackia 's implementation too, it looks more like the iOS implementation but I'm not familiar enough with RN to know if it would be a better candidate to get merged. My initial goal for this PR was to be able to use it with an Expo managed workflow, which doesn't let you write your own Java modules without ejecting.

maxoumime avatar Feb 05 '21 09:02 maxoumime

@maxoumime i've send some feedback directly to your code. I've already faced that kind of issues while implementing this flag in my project. Please check my gist, I've tried to resolve that issues already: https://gist.github.com/Strate/1bfb72b66379db7e73f89c57cd2103e7 This implementation may be not optimal, but works well.

Strate avatar Feb 06 '21 09:02 Strate

@Strate Thanks for your implementation! Could you share the VisibleItemsRange?

aanation avatar Mar 04 '21 18:03 aanation

@aanation you're welcome! I've added that class to gist: https://gist.github.com/Strate/1bfb72b66379db7e73f89c57cd2103e7

Strate avatar Mar 04 '21 18:03 Strate

@aanation I've also added ReactScrollViewManager. There is some cool features, like event onVisibleItemsRangeChange, and initialScroll based on item index (not contentOffset)

Strate avatar Mar 04 '21 18:03 Strate

@Strate :heart: :sunglasses: We will try your implementation

aanation avatar Mar 06 '21 08:03 aanation

@Strate Have you tested your implementation with FlatList?

aanation avatar Mar 07 '21 03:03 aanation

@aanation Yes, locally in my project both FlatList and ScrollView uses same native component.

Strate avatar Mar 08 '21 11:03 Strate

Any update on this ? The support of maintainVisibleContentPosition for Android would be a huge improvement for the Flatlist component.

ALexandreM75013 avatar Mar 21 '21 22:03 ALexandreM75013

Hey @maxoumime is it possible for you to resolve the conflicts on this one? I am trying to catch attention on this PR!! https://twitter.com/vishtree1992/status/1382064000721162241?s=20

vishalnarkhede avatar Apr 13 '21 20:04 vishalnarkhede

Not sure about the process, but what is the step required for this PR to get merged? Only resolving this conflict? Can't wait for it :)

markoj3s avatar Jul 16 '21 09:07 markoj3s

Thanks @maxoumime :) I am watching CircleCI checks with the same enthusiasm as when I was watching Wimbledon tennis finals

markoj3s avatar Jul 16 '21 10:07 markoj3s

@markoj3s 😂 Thanks for letting me know about the conflict. I'm not sure what the next step is... I think we need the "Needs: React Native team attention" label to get to the review stage. My last PR was merged quickly, but it was small and fixing a crash. This one is a bit more complex and might be breaking current Android apps that expect this property to be iOS-only.

maxoumime avatar Jul 16 '21 10:07 maxoumime

@maxoumime @gabrieldonadel @chrisglein Seems like everything's looking good :)

markoj3s avatar Jul 16 '21 11:07 markoj3s