From f70018b37532622f08f20b2c51cdbfca55d730ea Mon Sep 17 00:00:00 2001 From: Xin Chen Date: Tue, 30 Nov 2021 13:13:05 -0800 Subject: [PATCH] Fix quick small scroll gesture race issues Summary: This diff fixes two edge case (similar to a race condition) that caused unexpected behaviors. **Problem one** {F680816408} The previous fling animation is not canceled when user starts to scroll or drag. This is causing both the animation and scroll are setting the scroll position. Depends on the animation path and scroll speed, there may be cases where the [velocity calculation](https://fburl.com/code/010lsu72) ends up getting reversed values. See P467905091 as an example where you can see `mXFlingVelocity` goes back and forth from positive to negative. It's hard to see if the wrong values are in the middle, but if that happens in the end of user gesture, the velocity for the next fling would be wrong. It shows a "bounce back" effect, and can be triggered when user makes small quick joystick scrolls in one direction. **Problem two** {F680821494} There is a gap between animator's `onAnimationEnd` lifecycle method [finished](https://fburl.com/code/6baq04ne) and the `Animator#isRunning` API to return false. This is causing issues for `getPostAnimationScrollX` where we [decide to return](https://fburl.com/code/hzzugvch) the animated final value or the scroll value. User may see the `-1` value got used for the next fling start value, and the whole scroll view goes back to the beginning of scroll view and starts to fling. This happens when the previous fling animation finishes and the animated final value is set to -1, but at the same time the next fling starts before `isRunning` returns false for the previous animation. **Solution** The problems are fixed by - Do not reset animated final value to -1 in `onAnimationEnd` method - Add `mIsFinished` states and use it to track animation finish signal, instead of using `isRunning` API - Update logic where we decide to return the correct value for the next animation starts point. We will return previous animated final value when the animation got canceled, and user is going towards that value from the current scroll value. Changelog: [Android][Fixed] - Fixed edge case for quick small scrolls causing unexpected scrolling behaviors. Reviewed By: javache Differential Revision: D32487846 fbshipit-source-id: f1b0647656e021390e3a05de5846251a4a2647ff --- .../scroll/ReactHorizontalScrollView.java | 6 +- .../react/views/scroll/ReactScrollView.java | 6 +- .../views/scroll/ReactScrollViewHelper.java | 72 ++++++++++++++++--- 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java index c40b224774f..51066cc02b9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java @@ -457,6 +457,7 @@ public class ReactHorizontalScrollView extends HorizontalScrollView ReactScrollViewHelper.emitScrollBeginDragEvent(this); mDragging = true; enableFpsListener(); + getFlingAnimator().cancel(); return true; } } catch (IllegalArgumentException e) { @@ -819,7 +820,7 @@ public class ReactHorizontalScrollView extends HorizontalScrollView // predict where a fling would end up so we can scroll to the nearest snap offset int maximumOffset = Math.max(0, computeHorizontalScrollRange() - getWidth()); int width = getWidth() - ViewCompat.getPaddingStart(this) - ViewCompat.getPaddingEnd(this); - Point postAnimationScroll = ReactScrollViewHelper.getPostAnimationScroll(this); + Point postAnimationScroll = ReactScrollViewHelper.getPostAnimationScroll(this, velocityX > 0); scroller.fling( postAnimationScroll.x, // startX postAnimationScroll.y, // startY @@ -846,7 +847,8 @@ public class ReactHorizontalScrollView extends HorizontalScrollView } double interval = (double) getSnapInterval(); - double currentOffset = (double) (ReactScrollViewHelper.getPostAnimationScroll(this).x); + double currentOffset = + (double) (ReactScrollViewHelper.getPostAnimationScroll(this, velocity > 0).x); double targetOffset = (double) predictFinalScrollPosition(velocity); int previousPage = (int) Math.floor(currentOffset / interval); diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java index 98abcb48983..cf3de29afa4 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java @@ -337,6 +337,7 @@ public class ReactScrollView extends ScrollView ReactScrollViewHelper.emitScrollBeginDragEvent(this); mDragging = true; enableFpsListener(); + getFlingAnimator().cancel(); return true; } } catch (IllegalArgumentException e) { @@ -608,7 +609,7 @@ public class ReactScrollView extends ScrollView // predict where a fling would end up so we can scroll to the nearest snap offset int maximumOffset = getMaxScrollY(); int height = getHeight() - getPaddingBottom() - getPaddingTop(); - Point postAnimationScroll = ReactScrollViewHelper.getPostAnimationScroll(this); + Point postAnimationScroll = ReactScrollViewHelper.getPostAnimationScroll(this, velocityY > 0); scroller.fling( postAnimationScroll.x, // startX postAnimationScroll.y, // startY @@ -635,7 +636,8 @@ public class ReactScrollView extends ScrollView */ private void smoothScrollAndSnap(int velocity) { double interval = (double) getSnapInterval(); - double currentOffset = (double) (ReactScrollViewHelper.getPostAnimationScroll(this).y); + double currentOffset = + (double) (ReactScrollViewHelper.getPostAnimationScroll(this, velocity > 0).y); double targetOffset = (double) predictFinalScrollPosition(velocity); int previousPage = (int) Math.floor(currentOffset / interval); diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.java index 5dc993662d2..62f3ff94090 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.java @@ -226,6 +226,8 @@ public class ReactScrollViewHelper { private final Point mFinalAnimatedPositionScroll = new Point(); private int mScrollAwayPaddingTop = 0; private final Point mLastStateUpdateScroll = new Point(-1, -1); + private boolean mIsCanceled = false; + private boolean mIsFinished = true; public ReactScrollViewScrollState( final int layoutDirection, final ReactScrollViewScrollDirection scrollDirection) { @@ -283,6 +285,28 @@ public class ReactScrollViewHelper { mScrollAwayPaddingTop = scrollAwayPaddingTop; return this; } + + /** Get true if the previous animation was canceled */ + public boolean getIsCanceled() { + return mIsCanceled; + } + + /** Set the state of current animation is canceled or not */ + public ReactScrollViewScrollState setIsCanceled(boolean isCanceled) { + mIsCanceled = isCanceled; + return this; + } + + /** Get true if previous animation was finished */ + public boolean getIsFinished() { + return mIsFinished; + } + + /** Set the state of current animation is finished or not */ + public ReactScrollViewScrollState setIsFinished(boolean isFinished) { + mIsFinished = isFinished; + return this; + } } /** @@ -326,11 +350,36 @@ public class ReactScrollViewHelper { T extends ViewGroup & FabricViewStateManager.HasFabricViewStateManager & HasScrollState & HasFlingAnimator> - Point getPostAnimationScroll(final T scrollView) { - final ValueAnimator flingAnimator = scrollView.getFlingAnimator(); - return flingAnimator != null && flingAnimator.isRunning() - ? scrollView.getReactScrollViewScrollState().getFinalAnimatedPositionScroll() - : new Point(scrollView.getScrollX(), scrollView.getScrollY()); + Point getPostAnimationScroll(final T scrollView, final boolean isPositiveVelocity) { + final ReactScrollViewScrollState scrollState = scrollView.getReactScrollViewScrollState(); + final int velocityDirectionMask = isPositiveVelocity ? 1 : -1; + final Point animatedScrollPos = scrollState.getFinalAnimatedPositionScroll(); + final Point currentScrollPos = new Point(scrollView.getScrollX(), scrollView.getScrollY()); + + boolean isMovingTowardsAnimatedValue = false; + switch (scrollState.getScrollDirection()) { + case HORIZONTAL: + isMovingTowardsAnimatedValue = + velocityDirectionMask * (animatedScrollPos.x - currentScrollPos.x) > 0; + break; + + case VERTICAL: + isMovingTowardsAnimatedValue = + velocityDirectionMask * (animatedScrollPos.y - currentScrollPos.y) > 0; + break; + + default: + throw new IllegalArgumentException("ScrollView has unexpected scroll direction."); + } + + // When the fling animation is not finished, or it was canceled and now we are moving towards + // the final animated value, we will return the final animated value. This is because follow up + // animation should consider the "would be" animated location, so that previous quick small + // scrolls are still working. + return !scrollState.getIsFinished() + || (scrollState.getIsCanceled() && isMovingTowardsAnimatedValue) + ? animatedScrollPos + : currentScrollPos; } public static < @@ -432,16 +481,23 @@ public class ReactScrollViewHelper { .addListener( new Animator.AnimatorListener() { @Override - public void onAnimationStart(Animator animator) {} + public void onAnimationStart(Animator animator) { + final ReactScrollViewScrollState scrollState = + scrollView.getReactScrollViewScrollState(); + scrollState.setIsCanceled(false); + scrollState.setIsFinished(false); + } @Override public void onAnimationEnd(Animator animator) { - scrollView.getReactScrollViewScrollState().setFinalAnimatedPositionScroll(-1, -1); + scrollView.getReactScrollViewScrollState().setIsFinished(true); ReactScrollViewHelper.updateStateOnScroll(scrollView); } @Override - public void onAnimationCancel(Animator animator) {} + public void onAnimationCancel(Animator animator) { + scrollView.getReactScrollViewScrollState().setIsCanceled(true); + } @Override public void onAnimationRepeat(Animator animator) {}