From 2e631471092090e743245377742166ecae1d7e26 Mon Sep 17 00:00:00 2001 From: David Vacca Date: Tue, 5 Jan 2021 23:21:47 -0800 Subject: [PATCH] Refactor creation of views in Fabric Android Summary: This diff refactors the createViewInstance method in order to ensure that viewID is set before props are updated in the view. This is necessary because there are components that deliver events at the same time their props are set. This means that some components might not have their viewId set correctly when events are delivered. Since viewId is used to determine if a view belongs to Fabric or Paper, there are cases when the events are not delivered to the right renderer changelog: [internal] Reviewed By: JoshuaGross Differential Revision: D25667987 fbshipit-source-id: 4acfa8f80d66e9e59514354481957d7d3b571248 --- .../react/fabric/mounting/MountingManager.java | 2 +- .../uimanager/NativeViewHierarchyManager.java | 2 +- .../facebook/react/uimanager/ViewManager.java | 16 ++++++++-------- .../react/uimanager/SimpleViewPropertyTest.java | 10 ++++++++-- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index 2ea54f35e42..fe00e5d35b9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -533,7 +533,7 @@ public class MountingManager { // View Managers are responsible for dealing with initial state and props. view = viewManager.createView( - themedReactContext, propsDiffMap, stateWrapper, mJSResponderHandler); + reactTag, themedReactContext, propsDiffMap, stateWrapper, mJSResponderHandler); view.setId(reactTag); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java index ff1147ad3fe..26dc859215c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -278,7 +278,7 @@ public class NativeViewHierarchyManager { try { ViewManager viewManager = mViewManagers.get(className); - View view = viewManager.createView(themedContext, null, null, mJSResponderHandler); + View view = viewManager.createView(tag, themedContext, null, null, mJSResponderHandler); mTagsToViews.put(tag, view); mTagsToViewManagers.put(tag, viewManager); diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java index 5401620f715..cf1be7c7ce0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java @@ -67,19 +67,14 @@ public abstract class ViewManager return null; } - /** Creates a view and installs event emitters on it. */ - private final @NonNull T createView( - @NonNull ThemedReactContext reactContext, JSResponderHandler jsResponderHandler) { - return createView(reactContext, null, null, jsResponderHandler); - } - /** Creates a view with knowledge of props and state. */ public @NonNull T createView( + int reactTag, @NonNull ThemedReactContext reactContext, @Nullable ReactStylesDiffMap props, @Nullable StateWrapper stateWrapper, JSResponderHandler jsResponderHandler) { - T view = createViewInstance(reactContext, props, stateWrapper); + T view = createViewInstance(reactTag, reactContext, props, stateWrapper); if (view instanceof ReactInterceptingViewGroup) { ((ReactInterceptingViewGroup) view).setOnInterceptTouchEventListener(jsResponderHandler); } @@ -129,13 +124,18 @@ public abstract class ViewManager * that will call createViewInstance for you. Override it if you need props upon creation of the * view. * - * @param reactContext + * @param reactTag reactTag that should be set as ID of the view instance + * @param reactContext ReactContext used to initialize view instance + * @param initialProps initial props for the view instance + * @param stateWrapper initial state for the view instance */ protected @NonNull T createViewInstance( + int reactTag, @NonNull ThemedReactContext reactContext, @Nullable ReactStylesDiffMap initialProps, @Nullable StateWrapper stateWrapper) { T view = createViewInstance(reactContext); + view.setId(reactTag); addEventEmitters(reactContext, view); if (initialProps != null) { updateProperties(view, initialProps); diff --git a/ReactAndroid/src/test/java/com/facebook/react/uimanager/SimpleViewPropertyTest.java b/ReactAndroid/src/test/java/com/facebook/react/uimanager/SimpleViewPropertyTest.java index ed1d23990e4..91be74945d9 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/uimanager/SimpleViewPropertyTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/uimanager/SimpleViewPropertyTest.java @@ -36,6 +36,8 @@ public class SimpleViewPropertyTest { @Rule public PowerMockRule rule = new PowerMockRule(); + private static int sViewTag = 2; + private static class ConcreteViewManager extends SimpleViewManager { @ReactProp(name = "foo") @@ -75,7 +77,9 @@ public class SimpleViewPropertyTest { @Test public void testOpacity() { - View view = mManager.createView(mThemedContext, buildStyles(), null, new JSResponderHandler()); + View view = + mManager.createView( + sViewTag, mThemedContext, buildStyles(), null, new JSResponderHandler()); mManager.updateProperties(view, buildStyles()); assertThat(view.getAlpha()).isEqualTo(1.0f); @@ -89,7 +93,9 @@ public class SimpleViewPropertyTest { @Test public void testBackgroundColor() { - View view = mManager.createView(mThemedContext, buildStyles(), null, new JSResponderHandler()); + View view = + mManager.createView( + sViewTag, mThemedContext, buildStyles(), null, new JSResponderHandler()); mManager.updateProperties(view, buildStyles()); assertThat(view.getBackground()).isEqualTo(null);