Skip to content

Commit

Permalink
Fix quick small scroll gesture race issues
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ryancat authored and facebook-github-bot committed Nov 30, 2021
1 parent 79d20a1 commit f70018b
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ public boolean onInterceptTouchEvent(MotionEvent ev) {
ReactScrollViewHelper.emitScrollBeginDragEvent(this);
mDragging = true;
enableFpsListener();
getFlingAnimator().cancel();
return true;
}
} catch (IllegalArgumentException e) {
Expand Down Expand Up @@ -819,7 +820,7 @@ private int predictFinalScrollPosition(int velocityX) {
// 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
Expand All @@ -846,7 +847,8 @@ private void smoothScrollAndSnap(int velocity) {
}

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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ public boolean onInterceptTouchEvent(MotionEvent ev) {
ReactScrollViewHelper.emitScrollBeginDragEvent(this);
mDragging = true;
enableFpsListener();
getFlingAnimator().cancel();
return true;
}
} catch (IllegalArgumentException e) {
Expand Down Expand Up @@ -608,7 +609,7 @@ private int predictFinalScrollPosition(int velocityY) {
// 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
Expand All @@ -635,7 +636,8 @@ private View getContentView() {
*/
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ public static class ReactScrollViewScrollState {
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) {
Expand Down Expand Up @@ -283,6 +285,28 @@ public ReactScrollViewScrollState setScrollAwayPaddingTop(int scrollAwayPaddingT
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;
}
}

/**
Expand Down Expand Up @@ -326,11 +350,36 @@ void smoothScrollTo(final T scrollView, final int x, final int y) {
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 <
Expand Down Expand Up @@ -432,16 +481,23 @@ void registerFlingAnimator(final T scrollView) {
.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) {}
Expand Down

0 comments on commit f70018b

Please sign in to comment.