From 4ad4426daf51903574f81aff1b773645a16831e0 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 7 Jul 2021 15:57:39 -0700 Subject: [PATCH] Ship "state update scroll race" to ScrollView and HorizontalScrollView Summary: As a followup to T91209139, ship "state update scroll race" in code. This also ships it for HorizontalScrollView since it's been validated for vertical scroll views. Changelog: [Internal] Reviewed By: sammy-SC Differential Revision: D29595601 fbshipit-source-id: 64b6a23e2dab2c13123e132d9d899fb769d03172 --- .../react/config/ReactFeatureFlags.java | 13 ----- .../scroll/ReactHorizontalScrollView.java | 6 +++ .../react/views/scroll/ReactScrollView.java | 54 ++++--------------- .../views/scroll/ReactScrollViewManager.java | 1 - 4 files changed, 15 insertions(+), 59 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 168badc57f2..1be66e61b2f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -69,17 +69,4 @@ public class ReactFeatureFlags { public static boolean enableLockFreeEventDispatcher = false; public static boolean enableAggressiveEventEmitterCleanup = false; - - // - // ScrollView C++ UpdateState vs onScroll race fixes - // - - /* Enables a "state race condition fix" for ScrollViews StateUpdate + onScroll event emitter */ - public static boolean enableScrollViewStateEventRaceFix = false; - - /* Enables another "state race condition fix" for ScrollViews StateUpdate + onScroll event emitter. Races a StateUpdate with every onScroll event. */ - public static boolean enableScrollViewStateEventAlwaysRace = false; - - /* Configure a min scroll delta for UpdateState to be called while still actively scrolling. */ - public static int scrollViewUpdateStateMinScrollDelta = 0; } 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 0304f0e0678..0784d5ab229 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 @@ -417,6 +417,12 @@ public class ReactHorizontalScrollView extends HorizontalScrollView updateClippingRect(); } + // Race an UpdateState with every onScroll. This makes it more likely that, in Fabric, + // when JS processes the scroll event, the C++ ShadowNode representation will have a + // "more correct" scroll position. It will frequently be /incorrect/ but this decreases + // the error as much as possible. + updateStateOnScroll(); + ReactScrollViewHelper.emitScrollEvent( this, mOnScrollDispatchHelper.getXFlingVelocity(), 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 6a2273b9899..e56ddb8ace1 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 @@ -30,7 +30,6 @@ import com.facebook.react.bridge.ReactContext; import com.facebook.react.bridge.WritableMap; import com.facebook.react.bridge.WritableNativeMap; import com.facebook.react.common.ReactConstants; -import com.facebook.react.config.ReactFeatureFlags; import com.facebook.react.uimanager.FabricViewStateManager; import com.facebook.react.uimanager.MeasureSpecAssertions; import com.facebook.react.uimanager.PixelUtil; @@ -101,7 +100,6 @@ public class ReactScrollView extends ScrollView private int mScrollAwayPaddingTop = 0; - private boolean mWaitingForStateUpdateRoundTrip = false; private int mLastStateUpdateScrollX = -1; private int mLastStateUpdateScrollY = -1; @@ -288,22 +286,16 @@ public class ReactScrollView extends ScrollView updateClippingRect(); } - // Another potential UpdateState vs onScroll fix: race an UpdateState with every onScroll - // TODO T91209139: if this mechanism works well, port it to HorizontalScrollView - if (ReactFeatureFlags.enableScrollViewStateEventAlwaysRace) { - updateStateOnScroll(); - } + // Race an UpdateState with every onScroll. This makes it more likely that, in Fabric, + // when JS processes the scroll event, the C++ ShadowNode representation will have a + // "more correct" scroll position. It will frequently be /incorrect/ but this decreases + // the error as much as possible. + updateStateOnScroll(); - // TODO T91209139: if this mechanism works well, port it to HorizontalScrollView - boolean deferEvent = - ReactFeatureFlags.enableScrollViewStateEventRaceFix - && (mWaitingForStateUpdateRoundTrip || updateStateOnScroll()); - if (!deferEvent) { - ReactScrollViewHelper.emitScrollEvent( - this, - mOnScrollDispatchHelper.getXFlingVelocity(), - mOnScrollDispatchHelper.getYFlingVelocity()); - } + ReactScrollViewHelper.emitScrollEvent( + this, + mOnScrollDispatchHelper.getXFlingVelocity(), + mOnScrollDispatchHelper.getYFlingVelocity()); } } @@ -1013,23 +1005,6 @@ public class ReactScrollView extends ScrollView setRemoveClippedSubviews(mRemoveClippedSubviews); } - /** - * If we know we are sending a State update, we defer emitting scroll events until the State - * update makes it back to Java. When that happens, we should immediately emit the scroll event. - */ - void onStateUpdate() { - mWaitingForStateUpdateRoundTrip = false; - - // For now we don't dedupe these - we want to send an event whenever the metrics have changed - // from the perspective of C++ - if (ReactFeatureFlags.enableScrollViewStateEventRaceFix) { - ReactScrollViewHelper.emitScrollEvent( - this, - mOnScrollDispatchHelper.getXFlingVelocity(), - mOnScrollDispatchHelper.getYFlingVelocity()); - } - } - /** * Called on any stabilized onScroll change to propagate content offset value to a Shadow Node. */ @@ -1043,20 +1018,9 @@ public class ReactScrollView extends ScrollView return false; } - // Require a certain delta if we're still scrolling - if (mActivelyScrolling) { - int MIN_DELTA_TO_UPDATE_SCROLL_STATE = ReactFeatureFlags.scrollViewUpdateStateMinScrollDelta; - int deltaX = Math.abs(mLastStateUpdateScrollX - scrollX); - int deltaY = Math.abs(mLastStateUpdateScrollY - scrollY); - if (deltaX < MIN_DELTA_TO_UPDATE_SCROLL_STATE && deltaY < MIN_DELTA_TO_UPDATE_SCROLL_STATE) { - return false; - } - } - mLastStateUpdateScrollX = scrollX; mLastStateUpdateScrollY = scrollY; - mWaitingForStateUpdateRoundTrip = true; forceUpdateState(); return true; diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java index 8c6b6070a80..974584cd807 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java @@ -325,7 +325,6 @@ public class ReactScrollViewManager extends ViewGroupManager public Object updateState( ReactScrollView view, ReactStylesDiffMap props, @Nullable StateWrapper stateWrapper) { view.getFabricViewStateManager().setStateWrapper(stateWrapper); - view.onStateUpdate(); return null; }