From 03ef81bfa29dd1e896d58ee8a70a3df7db76d60d Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Sun, 10 May 2020 12:56:51 -0700 Subject: [PATCH] Fix taps not working after scrolling / scroll position not being updated in C++ Summary: Problem: ScrollView offset was not being reported to the C++ ScrollView side of Fabric. This results in taps not working correctly, for example if you tap a button inside scroll view after you scrolled, the tap might not trigger anything. The root cause of this is our implementation of detecting whether scroll view has stopped scrolling. To make this more robust, I now require that multiple "frames" have not scrolled because it's easy to trigger race conditions by scrolling very fast. We also explicitly call `updateStateOnScroll` in a couple more places. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D21496396 fbshipit-source-id: 2e565dd2fc4fc1ce582daa8a449c520e7cb19be0 --- .../scroll/ReactHorizontalScrollView.java | 54 ++++++++++++++++--- .../react/views/scroll/ReactScrollView.java | 54 ++++++++++++++++--- 2 files changed, 92 insertions(+), 16 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 1b2c5312381..61d40f287c8 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 @@ -91,6 +91,9 @@ public class ReactHorizontalScrollView extends HorizontalScrollView private int mFinalAnimatedPositionScrollX = 0; private int mFinalAnimatedPositionScrollY = 0; + private int mLastStateUpdateScrollX = -1; + private int mLastStateUpdateScrollY = -1; + private final Rect mTempRect = new Rect(); public ReactHorizontalScrollView(Context context) { @@ -409,7 +412,7 @@ public class ReactHorizontalScrollView extends HorizontalScrollView mVelocityHelper.calculateVelocity(ev); int action = ev.getAction() & MotionEvent.ACTION_MASK; if (action == MotionEvent.ACTION_UP && mDragging) { - updateStateOnScroll(getScrollX(), getScrollY()); + updateStateOnScroll(); float velocityX = mVelocityHelper.getXVelocity(); float velocityY = mVelocityHelper.getYVelocity(); @@ -606,12 +609,14 @@ public class ReactHorizontalScrollView extends HorizontalScrollView private void handlePostTouchScrolling(int velocityX, int velocityY) { // If we aren't going to do anything (send events or snap to page), we can early exit out. if (!mSendMomentumEvents && !mPagingEnabled && !isScrollPerfLoggingEnabled()) { + updateStateOnScroll(); return; } // Check if we are already handling this which may occur if this is called by both the touch up // and a fling call if (mPostTouchRunnable != null) { + updateStateOnScroll(); return; } @@ -624,16 +629,29 @@ public class ReactHorizontalScrollView extends HorizontalScrollView new Runnable() { private boolean mSnappingToPage = false; + private boolean mRunning = true; + private int mStableFrames = 0; @Override public void run() { if (mActivelyScrolling) { - // We are still scrolling so we just post to check again a frame later + // We are still scrolling. mActivelyScrolling = false; - ViewCompat.postOnAnimationDelayed( - ReactHorizontalScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY); + mStableFrames = 0; + mRunning = true; } else { - updateStateOnScroll(getScrollX(), getScrollY()); + // There has not been a scroll update since the last time this Runnable executed. + updateStateOnScroll(); + + // We keep checking for updates until the ScrollView has "stabilized" and hasn't + // scrolled for N consecutive frames. This number is arbitrary: big enough to catch + // a number of race conditions, but small enough to not cause perf regressions, etc. + // In anecdotal testing, it seemed like a decent number. + // Without this check, sometimes this Runnable stops executing too soon - it will + // fire before the first scroll event of an animated scroll/fling, and stop + // immediately. + mStableFrames++; + mRunning = (mStableFrames < 3); if (mPagingEnabled && !mSnappingToPage) { // Only if we have pagingEnabled and we have not snapped to the page do we @@ -647,14 +665,21 @@ public class ReactHorizontalScrollView extends HorizontalScrollView if (mSendMomentumEvents) { ReactScrollViewHelper.emitScrollMomentumEndEvent(ReactHorizontalScrollView.this); } - ReactHorizontalScrollView.this.mPostTouchRunnable = null; disableFpsListener(); } } + + // We are still scrolling so we just post to check again a frame later + if (mRunning) { + ViewCompat.postOnAnimationDelayed( + ReactHorizontalScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY); + } else { + mPostTouchRunnable = null; + } } }; ViewCompat.postOnAnimationDelayed( - ReactHorizontalScrollView.this, mPostTouchRunnable, ReactScrollViewHelper.MOMENTUM_DELAY); + this, mPostTouchRunnable, ReactScrollViewHelper.MOMENTUM_DELAY); } /** Get current X position or position after current animation finishes, if any. */ @@ -960,7 +985,7 @@ public class ReactHorizontalScrollView extends HorizontalScrollView public void onAnimationUpdate(ValueAnimator valueAnimator) { int scrollValueX = (Integer) valueAnimator.getAnimatedValue("scrollX"); int scrollValueY = (Integer) valueAnimator.getAnimatedValue("scrollY"); - ReactHorizontalScrollView.this.scrollTo(scrollValueX, scrollValueY); + scrollTo(scrollValueX, scrollValueY); } }); mScrollAnimator.addListener( @@ -973,6 +998,7 @@ public class ReactHorizontalScrollView extends HorizontalScrollView mFinalAnimatedPositionScrollX = -1; mFinalAnimatedPositionScrollY = -1; mScrollAnimator = null; + updateStateOnScroll(); } @Override @@ -1031,10 +1057,22 @@ public class ReactHorizontalScrollView extends HorizontalScrollView return; } + // Dedupe events to reduce JNI traffic + if (scrollX == mLastStateUpdateScrollX && scrollY == mLastStateUpdateScrollY) { + return; + } + + mLastStateUpdateScrollX = scrollX; + mLastStateUpdateScrollY = scrollY; + WritableMap map = new WritableNativeMap(); map.putDouble(CONTENT_OFFSET_LEFT, PixelUtil.toDIPFromPixel(scrollX)); map.putDouble(CONTENT_OFFSET_TOP, PixelUtil.toDIPFromPixel(scrollY)); mStateWrapper.updateState(map); } + + private void updateStateOnScroll() { + updateStateOnScroll(getScrollX(), getScrollY()); + } } 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 7a77b954415..bc3f3da39c1 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 @@ -96,6 +96,9 @@ public class ReactScrollView extends ScrollView private int mFinalAnimatedPositionScrollX; private int mFinalAnimatedPositionScrollY; + private int mLastStateUpdateScrollX = -1; + private int mLastStateUpdateScrollY = -1; + public ReactScrollView(ReactContext context) { this(context, null); } @@ -318,7 +321,7 @@ public class ReactScrollView extends ScrollView mVelocityHelper.calculateVelocity(ev); int action = ev.getAction() & MotionEvent.ACTION_MASK; if (action == MotionEvent.ACTION_UP && mDragging) { - updateStateOnScroll(getScrollX(), getScrollY()); + updateStateOnScroll(); float velocityX = mVelocityHelper.getXVelocity(); float velocityY = mVelocityHelper.getYVelocity(); @@ -493,12 +496,14 @@ public class ReactScrollView extends ScrollView private void handlePostTouchScrolling(int velocityX, int velocityY) { // If we aren't going to do anything (send events or snap to page), we can early exit out. if (!mSendMomentumEvents && !mPagingEnabled && !isScrollPerfLoggingEnabled()) { + updateStateOnScroll(); return; } // Check if we are already handling this which may occur if this is called by both the touch up // and a fling call if (mPostTouchRunnable != null) { + updateStateOnScroll(); return; } @@ -512,16 +517,29 @@ public class ReactScrollView extends ScrollView new Runnable() { private boolean mSnappingToPage = false; + private boolean mRunning = true; + private int mStableFrames = 0; @Override public void run() { if (mActivelyScrolling) { - // We are still scrolling so we just post to check again a frame later + // We are still scrolling. mActivelyScrolling = false; - ViewCompat.postOnAnimationDelayed( - ReactScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY); + mStableFrames = 0; + mRunning = true; } else { - updateStateOnScroll(getScrollX(), getScrollY()); + // There has not been a scroll update since the last time this Runnable executed. + updateStateOnScroll(); + + // We keep checking for updates until the ScrollView has "stabilized" and hasn't + // scrolled for N consecutive frames. This number is arbitrary: big enough to catch + // a number of race conditions, but small enough to not cause perf regressions, etc. + // In anecdotal testing, it seemed like a decent number. + // Without this check, sometimes this Runnable stops executing too soon - it will + // fire before the first scroll event of an animated scroll/fling, and stop + // immediately. + mStableFrames++; + mRunning = (mStableFrames < 3); if (mPagingEnabled && !mSnappingToPage) { // Only if we have pagingEnabled and we have not snapped to the page do we @@ -535,14 +553,21 @@ public class ReactScrollView extends ScrollView if (mSendMomentumEvents) { ReactScrollViewHelper.emitScrollMomentumEndEvent(ReactScrollView.this); } - ReactScrollView.this.mPostTouchRunnable = null; disableFpsListener(); } } + + // We are still scrolling so we just post to check again a frame later + if (mRunning) { + ViewCompat.postOnAnimationDelayed( + ReactScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY); + } else { + mPostTouchRunnable = null; + } } }; ViewCompat.postOnAnimationDelayed( - ReactScrollView.this, mPostTouchRunnable, ReactScrollViewHelper.MOMENTUM_DELAY); + this, mPostTouchRunnable, ReactScrollViewHelper.MOMENTUM_DELAY); } /** Get current X position or position after current animation finishes, if any. */ @@ -831,7 +856,7 @@ public class ReactScrollView extends ScrollView public void onAnimationUpdate(ValueAnimator valueAnimator) { int scrollValueX = (Integer) valueAnimator.getAnimatedValue("scrollX"); int scrollValueY = (Integer) valueAnimator.getAnimatedValue("scrollY"); - ReactScrollView.this.scrollTo(scrollValueX, scrollValueY); + scrollTo(scrollValueX, scrollValueY); } }); mScrollAnimator.addListener( @@ -844,6 +869,7 @@ public class ReactScrollView extends ScrollView mFinalAnimatedPositionScrollX = -1; mFinalAnimatedPositionScrollY = -1; mScrollAnimator = null; + updateStateOnScroll(); } @Override @@ -954,10 +980,22 @@ public class ReactScrollView extends ScrollView return; } + // Dedupe events to reduce JNI traffic + if (scrollX == mLastStateUpdateScrollX && scrollY == mLastStateUpdateScrollY) { + return; + } + + mLastStateUpdateScrollX = scrollX; + mLastStateUpdateScrollY = scrollY; + WritableMap map = new WritableNativeMap(); map.putDouble(CONTENT_OFFSET_LEFT, PixelUtil.toDIPFromPixel(scrollX)); map.putDouble(CONTENT_OFFSET_TOP, PixelUtil.toDIPFromPixel(scrollY)); mStateWrapper.updateState(map); } + + private void updateStateOnScroll() { + updateStateOnScroll(getScrollX(), getScrollY()); + } }