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

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(
+              mStableFrames = 0;
-                  ReactHorizontalScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
+              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());
+  }
 }

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(
+              mStableFrames = 0;
-                  ReactScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
+              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());
+  }
 }