From 30c7e9dfa41348e96e946384c76f038ccd859896 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Thu, 27 Jul 2023 16:26:18 -0700 Subject: [PATCH] Generalize RTL Scroll Correction Logic (#38526) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/38526 This changes the logic we use to correct scroll position on RTL ScrollView content change, to a new more generalized strategy of keeping a constant right-edge offset on layout change. This includes first layout, so that the initial scroll position is correct. Changelog: [Android][Fixed] - Generalize RTL Scroll Correction Logic Reviewed By: yungsters Differential Revision: D47627115 fbshipit-source-id: 33b2aae0cb603b7f7f2e2e6c127622fd531230e8 --- .../ReactHorizontalScrollContainerView.java | 21 +------ .../scroll/ReactHorizontalScrollView.java | 60 ++++++++++++++++++- 2 files changed, 59 insertions(+), 22 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollContainerView.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollContainerView.java index 3ac3d425659bec..fdf992e574dd40 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollContainerView.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollContainerView.java @@ -16,7 +16,6 @@ public class ReactHorizontalScrollContainerView extends ReactViewGroup { private int mLayoutDirection; - private int mCurrentWidth; public ReactHorizontalScrollContainerView(Context context) { super(context); @@ -24,7 +23,6 @@ public ReactHorizontalScrollContainerView(Context context) { I18nUtil.getInstance().isRTL(context) ? ViewCompat.LAYOUT_DIRECTION_RTL : ViewCompat.LAYOUT_DIRECTION_LTR; - mCurrentWidth = 0; } @Override @@ -50,24 +48,7 @@ protected void onLayout(boolean changed, int left, int top, int right, int botto int newLeft = 0; int width = right - left; int newRight = newLeft + width; - setLeft(newLeft); - setRight(newRight); - - /** - * Note: in RTL mode, *when layout width changes*, we adjust the scroll position. Practically, - * this means that on the first (meaningful) layout we will go from position 0 to position - * (right - screenWidth). In theory this means if the width of the view ever changes during - * layout again, scrolling could jump. Which shouldn't happen in theory, but... if you find a - * weird product bug that looks related, keep this in mind. - */ - if (mCurrentWidth != getWidth()) { - // Call with the present values in order to re-layout if necessary - ReactHorizontalScrollView parent = (ReactHorizontalScrollView) getParent(); - // Fix the ScrollX position when using RTL language - int offsetX = parent.getScrollX() + getWidth() - mCurrentWidth - parent.getWidth(); - parent.scrollTo(offsetX, parent.getScrollY()); - } + setLeftTopRightBottom(newLeft, top, newRight, bottom); } - mCurrentWidth = getWidth(); } } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java index 57a208c36ab96d..e5a9e12e4bc83e 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java @@ -20,6 +20,7 @@ import android.graphics.Rect; import android.graphics.drawable.ColorDrawable; import android.graphics.drawable.Drawable; +import android.os.Build; import android.view.FocusFinder; import android.view.KeyEvent; import android.view.MotionEvent; @@ -130,6 +131,8 @@ public ReactHorizontalScrollView(Context context, @Nullable FpsListener fpsListe mScroller = getOverScrollerFromParent(); mReactScrollViewScrollState = new ReactScrollViewScrollState( + // TODO: The ScrollView content may be laid out in a different direction than the + // instance if the `direction` style is set on the ScrollView or above it. I18nUtil.getInstance().isRTL(context) ? ViewCompat.LAYOUT_DIRECTION_RTL : ViewCompat.LAYOUT_DIRECTION_LTR); @@ -606,7 +609,9 @@ public void fling(int velocityX) { // Hence, we can use the absolute value from whatever the OS gives // us and use the sign of what mOnScrollDispatchHelper has tracked. final int correctedVelocityX = - (int) (Math.abs(velocityX) * Math.signum(mOnScrollDispatchHelper.getXFlingVelocity())); + Build.VERSION.SDK_INT == Build.VERSION_CODES.P + ? (int) (Math.abs(velocityX) * Math.signum(mOnScrollDispatchHelper.getXFlingVelocity())) + : velocityX; if (mPagingEnabled) { flingAndSnap(correctedVelocityX); @@ -1294,11 +1299,62 @@ public void onLayoutChange( return; } - if (mMaintainVisibleContentPositionHelper != null) { + // Adjust the scroll position to follow new content. In RTL, this means we keep a constant + // offset from the right edge instead of the left edge, so content added to the end of the flow + // does not shift layout. If `maintainVisibleContentPosition` is enabled, we try to adjust + // position so that the viewport keeps the same insets to previously visible views. TODO: MVCP + // does not work in RTL. + if (mReactScrollViewScrollState.getLayoutDirection() == LAYOUT_DIRECTION_RTL) { + adjustPositionForContentChangeRTL(left, right, oldLeft, oldRight); + } else if (mMaintainVisibleContentPositionHelper != null) { mMaintainVisibleContentPositionHelper.updateScrollPosition(); } } + private void adjustPositionForContentChangeRTL(int left, int right, int oldLeft, int oldRight) { + // If we have any pending custon flings (e.g. from aninmated `scrollTo`, or flinging to a snap + // point), finish them, commiting the final `scrollX`. + // TODO: Can we be more graceful (like OverScroller flings)? + if (getFlingAnimator().isRunning()) { + getFlingAnimator().end(); + } + + int distanceToRightEdge = oldRight - getScrollX(); + int newWidth = right - left; + int scrollX = newWidth - distanceToRightEdge; + scrollTo(scrollX, getScrollY()); + + // If we are in the middle of a fling animation from the user removing their finger + // (OverScroller is in `FLING_MODE`), we must cancel and recreate the existing fling animation + // since it was calculated against outdated scroll offsets. + if (mScroller != null && !mScroller.isFinished()) { + // Calculate the veliocity and position of the fling animation at the time of this layout + // event, which may be later than the last ScrollView tick. These values are not commited to + // the underlying ScrollView, which will recalculate positions on its next tick. + int scrollerXBeforeTick = mScroller.getCurrX(); + boolean hasMoreTicks = mScroller.computeScrollOffset(); + + // Stop the existing animation at the current state of the scroller. We will then recreate + // it starting at the adjusted x offset. + mScroller.forceFinished(true); + + if (hasMoreTicks) { + // OverScroller.getCurrVelocity() returns an absolute value of the velocity a current fling + // animation (only FLING_MODE animations). We derive direction along the X axis from the + // start and end of the, animation assuming HorizontalScrollView never fires vertical fling + // animations. + // TODO: This does not fully handle overscroll. + float direction = Math.signum(mScroller.getFinalX() - mScroller.getStartX()); + float flingVelocityX = mScroller.getCurrVelocity() * direction; + + mScroller.fling( + scrollX, getScrollY(), (int) flingVelocityX, 0, 0, newWidth - getWidth(), 0, 0); + } else { + scrollTo(scrollX + (mScroller.getCurrX() - scrollerXBeforeTick), getScrollY()); + } + } + } + @Override public FabricViewStateManager getFabricViewStateManager() { return mFabricViewStateManager;