Skip to content

Commit

Permalink
Ship "state update scroll race" to ScrollView and HorizontalScrollView
Browse files Browse the repository at this point in the history
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
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Jul 7, 2021
1 parent 421df26 commit 4ad4426
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,4 @@ public static boolean isMapBufferSerializationEnabled() {
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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,12 @@ protected void onScrollChanged(int x, int y, int oldX, int oldY) {
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(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -288,22 +286,16 @@ protected void onScrollChanged(int x, int y, int oldX, int oldY) {
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());
}
}

Expand Down Expand Up @@ -1013,23 +1005,6 @@ public void setScrollAwayTopPaddingEnabledUnstable(int topPadding) {
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.
*/
Expand All @@ -1043,20 +1018,9 @@ private boolean updateStateOnScroll(final int scrollX, final int scrollY) {
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,6 @@ public void setContentOffset(ReactScrollView view, ReadableMap value) {
public Object updateState(
ReactScrollView view, ReactStylesDiffMap props, @Nullable StateWrapper stateWrapper) {
view.getFabricViewStateManager().setStateWrapper(stateWrapper);
view.onStateUpdate();
return null;
}

Expand Down

0 comments on commit 4ad4426

Please sign in to comment.