Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up experiment to fix incorrect state updates in smooth scroll animations on Android #45237

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<9a33a6bc10cdb2b0f9fcb15805a06982>>
* @generated SignedSource<<1cad606bd06ab650004955138228d227>>
*/

/**
Expand Down Expand Up @@ -94,12 +94,24 @@ public object ReactNativeFeatureFlags {
@JvmStatic
public fun enableUIConsistency(): Boolean = accessor.enableUIConsistency()

/**
* When doing a smooth scroll animation, it stops setting the state with the final scroll position in Fabric before the animation starts.
*/
@JvmStatic
public fun fixIncorrectScrollViewStateUpdateOnAndroid(): Boolean = accessor.fixIncorrectScrollViewStateUpdateOnAndroid()

/**
* Uses the default event priority instead of the discreet event priority by default when dispatching events from Fabric to React.
*/
@JvmStatic
public fun fixMappingOfEventPrioritiesBetweenFabricAndReact(): Boolean = accessor.fixMappingOfEventPrioritiesBetweenFabricAndReact()

/**
* Enables a fix to prevent the possibility of state updates in Fabric being missed due to race conditions with previous state updates.
*/
@JvmStatic
public fun fixMissedFabricStateUpdatesOnAndroid(): Boolean = accessor.fixMissedFabricStateUpdatesOnAndroid()

/**
* Fixes a leak in SurfaceMountingManager.mRemoveDeleteTreeUIFrameCallback
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<5d67280406c16b01ba71b7b75e814a79>>
* @generated SignedSource<<b03fe019d1bacd13740a3e783197c17b>>
*/

/**
Expand All @@ -31,7 +31,9 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
private var enableMicrotasksCache: Boolean? = null
private var enableSynchronousStateUpdatesCache: Boolean? = null
private var enableUIConsistencyCache: Boolean? = null
private var fixIncorrectScrollViewStateUpdateOnAndroidCache: Boolean? = null
private var fixMappingOfEventPrioritiesBetweenFabricAndReactCache: Boolean? = null
private var fixMissedFabricStateUpdatesOnAndroidCache: Boolean? = null
private var fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeakCache: Boolean? = null
private var forceBatchingMountItemsOnAndroidCache: Boolean? = null
private var fuseboxEnabledDebugCache: Boolean? = null
Expand Down Expand Up @@ -145,6 +147,15 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
return cached
}

override fun fixIncorrectScrollViewStateUpdateOnAndroid(): Boolean {
var cached = fixIncorrectScrollViewStateUpdateOnAndroidCache
if (cached == null) {
cached = ReactNativeFeatureFlagsCxxInterop.fixIncorrectScrollViewStateUpdateOnAndroid()
fixIncorrectScrollViewStateUpdateOnAndroidCache = cached
}
return cached
}

override fun fixMappingOfEventPrioritiesBetweenFabricAndReact(): Boolean {
var cached = fixMappingOfEventPrioritiesBetweenFabricAndReactCache
if (cached == null) {
Expand All @@ -154,6 +165,15 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
return cached
}

override fun fixMissedFabricStateUpdatesOnAndroid(): Boolean {
var cached = fixMissedFabricStateUpdatesOnAndroidCache
if (cached == null) {
cached = ReactNativeFeatureFlagsCxxInterop.fixMissedFabricStateUpdatesOnAndroid()
fixMissedFabricStateUpdatesOnAndroidCache = cached
}
return cached
}

override fun fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeak(): Boolean {
var cached = fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeakCache
if (cached == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<154baea748cdf7b8e05a1e4448053673>>
* @generated SignedSource<<1b39ae121d8238bbb34c54330700c9e8>>
*/

/**
Expand Down Expand Up @@ -50,8 +50,12 @@ public object ReactNativeFeatureFlagsCxxInterop {

@DoNotStrip @JvmStatic public external fun enableUIConsistency(): Boolean

@DoNotStrip @JvmStatic public external fun fixIncorrectScrollViewStateUpdateOnAndroid(): Boolean

@DoNotStrip @JvmStatic public external fun fixMappingOfEventPrioritiesBetweenFabricAndReact(): Boolean

@DoNotStrip @JvmStatic public external fun fixMissedFabricStateUpdatesOnAndroid(): Boolean

@DoNotStrip @JvmStatic public external fun fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeak(): Boolean

@DoNotStrip @JvmStatic public external fun forceBatchingMountItemsOnAndroid(): Boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<34bbd584a612fa88cc6adf2d2bc51b92>>
* @generated SignedSource<<0584cef0a5e682b2b3ba6d46161e0286>>
*/

/**
Expand Down Expand Up @@ -45,8 +45,12 @@ public open class ReactNativeFeatureFlagsDefaults : ReactNativeFeatureFlagsProvi

override fun enableUIConsistency(): Boolean = false

override fun fixIncorrectScrollViewStateUpdateOnAndroid(): Boolean = false

override fun fixMappingOfEventPrioritiesBetweenFabricAndReact(): Boolean = false

override fun fixMissedFabricStateUpdatesOnAndroid(): Boolean = false

override fun fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeak(): Boolean = false

override fun forceBatchingMountItemsOnAndroid(): Boolean = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<d991cac9311ee91e5e9401b143b69145>>
* @generated SignedSource<<a97af69ca22746594d72410f72a0a1cb>>
*/

/**
Expand Down Expand Up @@ -35,7 +35,9 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
private var enableMicrotasksCache: Boolean? = null
private var enableSynchronousStateUpdatesCache: Boolean? = null
private var enableUIConsistencyCache: Boolean? = null
private var fixIncorrectScrollViewStateUpdateOnAndroidCache: Boolean? = null
private var fixMappingOfEventPrioritiesBetweenFabricAndReactCache: Boolean? = null
private var fixMissedFabricStateUpdatesOnAndroidCache: Boolean? = null
private var fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeakCache: Boolean? = null
private var forceBatchingMountItemsOnAndroidCache: Boolean? = null
private var fuseboxEnabledDebugCache: Boolean? = null
Expand Down Expand Up @@ -160,6 +162,16 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
return cached
}

override fun fixIncorrectScrollViewStateUpdateOnAndroid(): Boolean {
var cached = fixIncorrectScrollViewStateUpdateOnAndroidCache
if (cached == null) {
cached = currentProvider.fixIncorrectScrollViewStateUpdateOnAndroid()
accessedFeatureFlags.add("fixIncorrectScrollViewStateUpdateOnAndroid")
fixIncorrectScrollViewStateUpdateOnAndroidCache = cached
}
return cached
}

override fun fixMappingOfEventPrioritiesBetweenFabricAndReact(): Boolean {
var cached = fixMappingOfEventPrioritiesBetweenFabricAndReactCache
if (cached == null) {
Expand All @@ -170,6 +182,16 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
return cached
}

override fun fixMissedFabricStateUpdatesOnAndroid(): Boolean {
var cached = fixMissedFabricStateUpdatesOnAndroidCache
if (cached == null) {
cached = currentProvider.fixMissedFabricStateUpdatesOnAndroid()
accessedFeatureFlags.add("fixMissedFabricStateUpdatesOnAndroid")
fixMissedFabricStateUpdatesOnAndroidCache = cached
}
return cached
}

override fun fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeak(): Boolean {
var cached = fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeakCache
if (cached == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<aae9b8936680c1cd6db2c1c0135e1cef>>
* @generated SignedSource<<de2cd46cfe4a934b6584e782c4d9d213>>
*/

/**
Expand Down Expand Up @@ -45,8 +45,12 @@ public interface ReactNativeFeatureFlagsProvider {

@DoNotStrip public fun enableUIConsistency(): Boolean

@DoNotStrip public fun fixIncorrectScrollViewStateUpdateOnAndroid(): Boolean

@DoNotStrip public fun fixMappingOfEventPrioritiesBetweenFabricAndReact(): Boolean

@DoNotStrip public fun fixMissedFabricStateUpdatesOnAndroid(): Boolean

@DoNotStrip public fun fixStoppedSurfaceRemoveDeleteTreeUIFrameCallbackLeak(): Boolean

@DoNotStrip public fun forceBatchingMountItemsOnAndroid(): Boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ 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.internal.featureflags.ReactNativeFeatureFlags
import com.facebook.react.uimanager.PixelUtil.toDIPFromPixel
import com.facebook.react.uimanager.StateWrapper
import com.facebook.react.uimanager.UIManagerHelper
Expand Down Expand Up @@ -249,7 +250,9 @@ public object ReactScrollViewHelper {
if (scrollY != y) {
scrollView.startFlingAnimator(scrollY, y)
}
updateFabricScrollState<T>(scrollView, x, y)
if (ReactNativeFeatureFlags.fixIncorrectScrollViewStateUpdateOnAndroid()) {
updateFabricScrollState<T>(scrollView, x, y)
}
}

/** Get current position or position after current animation finishes, if any. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ void FabricMountingManager::executeMount(
if (mountItem.newChildShadowView.state != nullptr) {
javaStateWrapper = StateWrapperImpl::newObjectJavaArgs();
StateWrapperImpl* cStateWrapper = cthis(javaStateWrapper);
cStateWrapper->state_ = mountItem.newChildShadowView.state;
cStateWrapper->setState(mountItem.newChildShadowView.state);
}

// Do not hold a reference to javaEventEmitter from the C++ side.
Expand Down Expand Up @@ -615,7 +615,7 @@ void FabricMountingManager::executeMount(
if (state != nullptr) {
javaStateWrapper = StateWrapperImpl::newObjectJavaArgs();
StateWrapperImpl* cStateWrapper = cthis(javaStateWrapper);
cStateWrapper->state_ = state;
cStateWrapper->setState(state);
}

(*objBufferArray)[objBufferPosition++] =
Expand Down Expand Up @@ -823,7 +823,7 @@ void FabricMountingManager::preallocateShadowView(
if (shadowView.state != nullptr) {
javaStateWrapper = StateWrapperImpl::newObjectJavaArgs();
StateWrapperImpl* cStateWrapper = cthis(javaStateWrapper);
cStateWrapper->state_ = shadowView.state;
cStateWrapper->setState(shadowView.state);
}

// Do not hold a reference to javaEventEmitter from the C++ side.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include "StateWrapperImpl.h"
#include <fbjni/fbjni.h>
#include <react/featureflags/ReactNativeFeatureFlags.h>
#include <react/jni/ReadableNativeMap.h>
#include <react/renderer/mapbuffer/MapBuffer.h>
#include <react/renderer/mapbuffer/MapBufferBuilder.h>
Expand All @@ -25,30 +26,65 @@ jni::local_ref<StateWrapperImpl::jhybriddata> StateWrapperImpl::initHybrid(

jni::local_ref<ReadableNativeMap::jhybridobject>
StateWrapperImpl::getStateDataImpl() {
if (auto state = state_.lock()) {
folly::dynamic map = state->getDynamic();
return ReadableNativeMap::newObjectCxxArgs(std::move(map));
if (ReactNativeFeatureFlags::fixMissedFabricStateUpdatesOnAndroid()) {
if (state_) {
folly::dynamic map = state_->getDynamic();
return ReadableNativeMap::newObjectCxxArgs(std::move(map));
} else {
return nullptr;
}
} else {
return nullptr;
if (auto state = weakState_.lock()) {
folly::dynamic map = state->getDynamic();
return ReadableNativeMap::newObjectCxxArgs(std::move(map));
} else {
return nullptr;
}
}
}

jni::local_ref<JReadableMapBuffer::jhybridobject>
StateWrapperImpl::getStateMapBufferDataImpl() {
if (auto state = state_.lock()) {
MapBuffer map = state->getMapBuffer();
return JReadableMapBuffer::createWithContents(std::move(map));
if (ReactNativeFeatureFlags::fixMissedFabricStateUpdatesOnAndroid()) {
if (state_) {
MapBuffer map = state_->getMapBuffer();
return JReadableMapBuffer::createWithContents(std::move(map));
} else {
return nullptr;
}
} else {
return nullptr;
if (auto state = weakState_.lock()) {
MapBuffer map = state->getMapBuffer();
return JReadableMapBuffer::createWithContents(std::move(map));
} else {
return nullptr;
}
}
}

void StateWrapperImpl::updateStateImpl(NativeMap* map) {
if (auto state = state_.lock()) {
// Get folly::dynamic from map
auto dynamicMap = map->consume();
// Set state
state->updateState(std::move(dynamicMap));
if (ReactNativeFeatureFlags::fixMissedFabricStateUpdatesOnAndroid()) {
if (state_) {
// Get folly::dynamic from map
auto dynamicMap = map->consume();
// Set state
state_->updateState(std::move(dynamicMap));
}
} else {
if (auto state = weakState_.lock()) {
// Get folly::dynamic from map
auto dynamicMap = map->consume();
// Set state
state->updateState(std::move(dynamicMap));
}
}
}

void StateWrapperImpl::setState(std::shared_ptr<const State> state) {
if (ReactNativeFeatureFlags::fixMissedFabricStateUpdatesOnAndroid()) {
state_ = state;
} else {
weakState_ = state;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ class StateWrapperImpl : public jni::HybridClass<StateWrapperImpl> {
jni::local_ref<JReadableMapBuffer::jhybridobject> getStateMapBufferDataImpl();
jni::local_ref<ReadableNativeMap::jhybridobject> getStateDataImpl();
void updateStateImpl(NativeMap* map);

std::weak_ptr<const State> state_;
void setState(std::shared_ptr<const State> state);

private:
jni::alias_ref<StateWrapperImpl::jhybriddata> jhybridobject_;
std::weak_ptr<const State> weakState_;
std::shared_ptr<const State> state_;

static jni::local_ref<jhybriddata> initHybrid(jni::alias_ref<jclass>);
};
Expand Down
Loading
Loading