Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for ScrollView.maintainVisibleContentPosition on Android #29466

Closed
wants to merge 28 commits into from
Closed
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b1df249
:construction: maintainVisibleContentPosition on Android
maxoumime Jul 16, 2020
8c29447
:construction: maintainVisibleContentPosition on Android
maxoumime Jul 17, 2020
a2509b3
:sparkles: maintainVisibleContentPosition on Android
maxoumime Jul 21, 2020
8d4fd42
:wrench: Add sample
maxoumime Jul 21, 2020
99b69db
:wrench: Cleanup and comments
maxoumime Jul 21, 2020
63cb074
:wrench: maintainVisibleContentPosition for Android - horizontal
maxoumime Jul 22, 2020
2f3110b
:wrench: Self-review
maxoumime Jul 22, 2020
344b9db
Merge remote-tracking branch 'facebook/master'
maxoumime Jul 22, 2020
84ecefc
:wrench: Lint
maxoumime Jul 22, 2020
9d5eef8
:wrench: Lint
maxoumime Jul 22, 2020
45c5ecf
:wrench: Method references not available
maxoumime Jul 22, 2020
3992154
:wrench: Fix CI
maxoumime Jul 22, 2020
19603e1
:wrench: Fix CI #2
maxoumime Jul 22, 2020
174f348
:wrench: Fix CI
maxoumime Jul 27, 2020
2e37d50
Merge branch 'master' of https://github.com/facebook/react-native
maxoumime Aug 12, 2020
04b8905
Merge remote-tracking branch 'facebook/master' into master
maxoumime Sep 8, 2020
7a67669
Merge branch 'master' into master
maxoumime Sep 9, 2020
1bc4b8d
Merge branch 'master' into master
maxoumime Sep 10, 2020
4039ecd
Merge branch 'master' into master
maxoumime Feb 5, 2021
9bed499
:tshirt:
maxoumime Feb 5, 2021
421f329
Merge remote-tracking branch 'facebook/master'
maxoumime Jun 8, 2021
7761851
:wrench: reactScrollTo doesn't exist anymore
maxoumime Jun 8, 2021
cc6bdf0
:tshirt:
maxoumime Jun 8, 2021
3257a58
Merge branch 'master' of https://github.com/facebook/react-native
maxoumime Jul 2, 2021
fca378d
:wrench: :robot:
maxoumime Jul 2, 2021
03f7d9d
Merge branch 'master' into master
maxoumime Jul 16, 2021
0a15381
:see_no_evil:
maxoumime Jul 16, 2021
51beb41
Merge remote-tracking branch 'facebook/main'
maxoumime Aug 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 26 additions & 28 deletions Libraries/Components/ScrollView/ScrollView.js
Original file line number Diff line number Diff line change
Expand Up @@ -249,34 +249,6 @@ type IOSProps = $ReadOnly<{|
* @platform ios
*/
canCancelContentTouches?: ?boolean,
/**
* When set, the scroll view will adjust the scroll position so that the first child that is
* currently visible and at or beyond `minIndexForVisible` will not change position. This is
* useful for lists that are loading content in both directions, e.g. a chat thread, where new
* messages coming in might otherwise cause the scroll position to jump. A value of 0 is common,
* but other values such as 1 can be used to skip loading spinners or other content that should
* not maintain position.
*
* The optional `autoscrollToTopThreshold` can be used to make the content automatically scroll
* to the top after making the adjustment if the user was within the threshold of the top before
* the adjustment was made. This is also useful for chat-like applications where you want to see
* new messages scroll into place, but not if the user has scrolled up a ways and it would be
* disruptive to scroll a bunch.
*
* Caveat 1: Reordering elements in the scrollview with this enabled will probably cause
* jumpiness and jank. It can be fixed, but there are currently no plans to do so. For now,
* don't re-order the content of any ScrollViews or Lists that use this feature.
*
* Caveat 2: This simply uses `contentOffset` and `frame.origin` in native code to compute
* visibility. Occlusion, transforms, and other complexity won't be taken into account as to
* whether content is "visible" or not.
*
* @platform ios
*/
maintainVisibleContentPosition?: ?$ReadOnly<{|
minIndexForVisible: number,
autoscrollToTopThreshold?: ?number,
|}>,
/**
* The maximum allowed zoom scale. The default value is 1.0.
* @platform ios
Expand Down Expand Up @@ -513,6 +485,32 @@ export type Props = $ReadOnly<{|
* - `true`, deprecated, use 'always' instead
*/
keyboardShouldPersistTaps?: ?('always' | 'never' | 'handled' | true | false),
/**
* When set, the scroll view will adjust the scroll position so that the first child that is
* currently visible and at or beyond `minIndexForVisible` will not change position. This is
* useful for lists that are loading content in both directions, e.g. a chat thread, where new
* messages coming in might otherwise cause the scroll position to jump. A value of 0 is common,
* but other values such as 1 can be used to skip loading spinners or other content that should
* not maintain position.
*
* The optional `autoscrollToTopThreshold` can be used to make the content automatically scroll
* to the top after making the adjustment if the user was within the threshold of the top before
* the adjustment was made. This is also useful for chat-like applications where you want to see
* new messages scroll into place, but not if the user has scrolled up a ways and it would be
* disruptive to scroll a bunch.
*
* Caveat 1: Reordering elements in the scrollview with this enabled will probably cause
* jumpiness and jank. It can be fixed, but there are currently no plans to do so. For now,
* don't re-order the content of any ScrollViews or Lists that use this feature.
*
* Caveat 2: This simply uses `contentOffset` and `frame.origin` in native code to compute
* visibility. Occlusion, transforms, and other complexity won't be taken into account as to
* whether content is "visible" or not.
*/
maintainVisibleContentPosition?: ?$ReadOnly<{|
minIndexForVisible: number,
autoscrollToTopThreshold?: ?number,
|}>,
/**
* Called when the momentum scroll starts (scroll which occurs as the ScrollView glides to a stop).
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
import android.graphics.Rect;
import android.graphics.drawable.ColorDrawable;
import android.graphics.drawable.Drawable;
import android.os.Handler;
import android.view.FocusFinder;
import android.view.KeyEvent;
import android.view.MotionEvent;
import android.view.View;
import android.view.ViewGroup;
import android.view.accessibility.AccessibilityEvent;
import android.widget.HorizontalScrollView;
import android.widget.OverScroller;
Expand All @@ -43,13 +45,19 @@
import com.facebook.react.uimanager.ViewProps;
import com.facebook.react.uimanager.events.NativeGestureUtil;
import com.facebook.react.views.view.ReactViewBackgroundManager;
import com.facebook.react.views.view.ReactViewGroup;

import java.lang.ref.WeakReference;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.List;

/** Similar to {@link ReactScrollView} but only supports horizontal scrolling. */
public class ReactHorizontalScrollView extends HorizontalScrollView
implements ReactClippingViewGroup, FabricViewStateManager.HasFabricViewStateManager {
implements ReactClippingViewGroup,
ViewGroup.OnHierarchyChangeListener,
View.OnLayoutChangeListener,
FabricViewStateManager.HasFabricViewStateManager {

private static boolean DEBUG_MODE = false && ReactBuildConfig.DEBUG;
private static String TAG = ReactHorizontalScrollView.class.getSimpleName();
Expand Down Expand Up @@ -91,11 +99,23 @@ public class ReactHorizontalScrollView extends HorizontalScrollView
private @Nullable List<Integer> mSnapOffsets;
private boolean mSnapToStart = true;
private boolean mSnapToEnd = true;
private View mContentView;
private ReactViewBackgroundManager mReactBackgroundManager;
private boolean mPagedArrowScrolling = false;
private int pendingContentOffsetX = UNSET_CONTENT_OFFSET;
private int pendingContentOffsetY = UNSET_CONTENT_OFFSET;
private final FabricViewStateManager mFabricViewStateManager = new FabricViewStateManager();
private @Nullable ReactScrollViewMaintainVisibleContentPositionData mMaintainVisibleContentPositionData;
private @Nullable WeakReference<View> firstVisibleViewForMaintainVisibleContentPosition = null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose to use a WeakReference here?

private @Nullable Rect prevFirstVisibleFrameForMaintainVisibleContentPosition = null;

private final Handler mHandler = new Handler();
private final Runnable mComputeFirstVisibleViewRunnable = new Runnable() {
@Override
public void run() {
computeFirstVisibleItemForMaintainVisibleContentPosition();
}
};

private @Nullable ValueAnimator mScrollAnimator;
private int mFinalAnimatedPositionScrollX = 0;
Expand Down Expand Up @@ -133,6 +153,7 @@ public void onInitializeAccessibilityNodeInfo(
});

mScroller = getOverScrollerFromParent();
setOnHierarchyChangeListener(this);
mLayoutDirection =
I18nUtil.getInstance().isRTL(context)
? ViewCompat.LAYOUT_DIRECTION_RTL
Expand Down Expand Up @@ -245,6 +266,13 @@ public void setOverflow(String overflow) {
invalidate();
}

public void setMaintainVisibleContentPosition(ReactScrollViewMaintainVisibleContentPositionData maintainVisibleContentPositionData) {
mMaintainVisibleContentPositionData = maintainVisibleContentPositionData;
if (maintainVisibleContentPositionData != null) {
computeFirstVisibleItemForMaintainVisibleContentPosition();
}
}

@Override
protected void onDraw(Canvas canvas) {
if (DEBUG_MODE) {
Expand Down Expand Up @@ -422,6 +450,13 @@ protected void onScrollChanged(int x, int y, int oldX, int oldY) {
mOnScrollDispatchHelper.getXFlingVelocity(),
mOnScrollDispatchHelper.getYFlingVelocity());
}

if (mMaintainVisibleContentPositionData != null) {
// We don't want to compute the first visible view everytime onScrollChanged gets called (can be multiple times per second).
// The following logic debounces the computation by 100ms (arbitrary value).
mHandler.removeCallbacks(mComputeFirstVisibleViewRunnable);
mHandler.postDelayed(mComputeFirstVisibleViewRunnable, 100);
}
}

@Override
Expand Down Expand Up @@ -1070,6 +1105,18 @@ public void setBorderStyle(@Nullable String style) {
mReactBackgroundManager.setBorderStyle(style);
}

@Override
public void onChildViewAdded(View parent, View child) {
mContentView = child;
mContentView.addOnLayoutChangeListener(this);
}

@Override
public void onChildViewRemoved(View parent, View child) {
mContentView.removeOnLayoutChangeListener(this);
mContentView = null;
}

/**
* Calls `smoothScrollTo` and updates state.
*
Expand Down Expand Up @@ -1225,6 +1272,91 @@ private void updateStateOnScroll() {
updateStateOnScroll(getScrollX(), getScrollY());
}

/**
* Called when a mContentView's layout has changed. Fixes the scroll position depending on maintainVisibleContentPosition
*/
@Override
public void onLayoutChange(
View v,
int left,
int top,
int right,
int bottom,
int oldLeft,
int oldTop,
int oldRight,
int oldBottom) {
if (mContentView == null) {
return;
}

if (this.mMaintainVisibleContentPositionData != null) {
scrollMaintainVisibleContentPosition();
}
}

/**
* Called when maintainVisibleContentPosition is used and after a scroll.
* Finds the first completely visible view in the ScrollView and stores it for later use.
*/
private void computeFirstVisibleItemForMaintainVisibleContentPosition() {
ReactScrollViewMaintainVisibleContentPositionData maintainVisibleContentPositionData = mMaintainVisibleContentPositionData;
if (maintainVisibleContentPositionData == null) return;

int currentScrollX = getScrollX();
int minIdx = maintainVisibleContentPositionData.minIndexForVisible;

ReactViewGroup contentView = (ReactViewGroup) getChildAt(0);
if (contentView == null) return;

for (int i = minIdx; i < contentView.getChildCount(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use binary search here to reduce computational complexity.

Normally this is not a big deal, but here we are calling this method on every scroll ends (with the debounce it's better, but still will call this a lot). If we have many items in the list, this could lead to a slow perf issue.

// Find the first entirely visible view. This must be done after we update the content offset
// or it will tend to grab rows that were made visible by the shift in position
View child = contentView.getChildAt(i);
if (child.getX() >= currentScrollX || i == contentView.getChildCount() - 1) {
firstVisibleViewForMaintainVisibleContentPosition = new WeakReference<>(child);
Rect frame = new Rect();
child.getHitRect(frame);
prevFirstVisibleFrameForMaintainVisibleContentPosition = frame;
break;
}
}
}

/**
* Called when maintainVisibleContentPosition is used and after a layout change.
* Detects if the layout change impacts the scroll position and corrects it if needed.
*/
private void scrollMaintainVisibleContentPosition() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some problems with this approach.

  1. When the scroll view first loads and no scroll events occur before items are prepended, this approach would not maintain scroll position because firstVisibleView is only set after at least one scroll event.
  2. There could be a race condition between when the VirtualizedList prepends an item and when we invoke a scroll event (onScroll vs. onLayoutChange). This can cause the delta of the cached item to incorrectly be zero, leading to the scroll position not being maintained.
  3. We noticed that when there are many items in the list, onLayoutChange does not get called and the scroll position is not maintained. (This may be due to the loop computation being too expensive.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be a race condition between when the VirtualizedList prepends an item and when we invoke a scroll event (onScroll vs. onLayoutChange). This can cause the delta of the cached item to incorrectly be zero, leading to the scroll position not being maintained.

I've been digging into this problem and trying to find a solution. Any suggestions?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm mistaken, the same problem exists with the current iOS implementation of maintainVisibleContentPosition and may be the cause of #25239

ReactScrollViewMaintainVisibleContentPositionData maintainVisibleContentPositionData = this.mMaintainVisibleContentPositionData;
if (maintainVisibleContentPositionData == null) return;

int currentScrollX = getScrollX();

View firstVisibleView = firstVisibleViewForMaintainVisibleContentPosition != null ? firstVisibleViewForMaintainVisibleContentPosition.get() : null;
if (firstVisibleView == null) return;
Rect prevFirstVisibleFrame = this.prevFirstVisibleFrameForMaintainVisibleContentPosition;
if (prevFirstVisibleFrame == null) return;

Rect newFrame = new Rect();
firstVisibleView.getHitRect(newFrame);
int deltaX = newFrame.left - prevFirstVisibleFrame.left;

if (Math.abs(deltaX) > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to store this value using a constant.

Suggested change
if (Math.abs(deltaX) > 1) {
if (Math.abs(deltaX) > SCROLL_DELTA_EPSILON) {

int scrollXTo = getScrollX() + deltaX;

scrollTo(scrollXTo, getScrollY());

Integer autoScrollThreshold = maintainVisibleContentPositionData.autoScrollToTopThreshold;
if (autoScrollThreshold != null) {
// If the offset WAS within the threshold of the start, animate to the start.
if (currentScrollX - deltaX <= autoScrollThreshold) {
reactSmoothScrollTo(0, getScrollY());
}
}
}
}

@Override
public FabricViewStateManager getFabricViewStateManager() {
return mFabricViewStateManager;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,4 +311,16 @@ public void setContentOffset(ReactHorizontalScrollView view, ReadableMap value)
view.scrollTo(0, 0);
}
}

@ReactProp(name = "maintainVisibleContentPosition")
public void setMaintainVisibleContentPosition(ReactHorizontalScrollView view, ReadableMap value) {
if (value != null) {
int minIndexForVisible = value.getInt("minIndexForVisible");
Integer autoScrollToTopThreshold = value.hasKey("autoscrollToTopThreshold") ? value.getInt("autoscrollToTopThreshold") : null;
ReactScrollViewMaintainVisibleContentPositionData maintainVisibleContentPositionData = new ReactScrollViewMaintainVisibleContentPositionData(minIndexForVisible, autoScrollToTopThreshold);
view.setMaintainVisibleContentPosition(maintainVisibleContentPositionData);
} else {
view.setMaintainVisibleContentPosition(null);
}
}
}
Loading