From 6d6268e903def9e4a8609e5dee6de2d2c4ab8c5e Mon Sep 17 00:00:00 2001 From: David Vacca Date: Sat, 13 Jun 2020 22:18:06 -0700 Subject: [PATCH] Fix padding flickering visible during initial render of text input Summary: This diff fixes a race condition that caused a flicker during initial rendering of TextInput in Fabric The root cause is that the TextInput's State is sometimes initialized with no information from the Theme, this causes text input to be rendered with 0 padding. Later ReactTextInput manager updates TextInput state with theme data and the TextInput is re-rendered with the correct padding information. changelog: [Internal] Reviewed By: sammy-SC Differential Revision: D22034849 fbshipit-source-id: a2fa91f34a8340878f2ec8d231ef6c86cee08f05 --- .../react/fabric/FabricUIManager.java | 31 +++++++++ .../com/facebook/react/fabric/jni/Binding.cpp | 63 +++++++++---------- .../com/facebook/react/fabric/jni/Binding.h | 3 + .../react/uimanager/UIManagerHelper.java | 21 +++++++ .../textinput/ReactTextInputManager.java | 19 +++--- .../AndroidTextInputComponentDescriptor.h | 25 ++++++++ 6 files changed, 121 insertions(+), 41 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index f70b0fc5632..e6762217abc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -14,6 +14,10 @@ import static com.facebook.react.fabric.mounting.LayoutMetricsConversions.getMax import static com.facebook.react.fabric.mounting.LayoutMetricsConversions.getMinSize; import static com.facebook.react.fabric.mounting.LayoutMetricsConversions.getYogaMeasureMode; import static com.facebook.react.fabric.mounting.LayoutMetricsConversions.getYogaSize; +import static com.facebook.react.uimanager.UIManagerHelper.PADDING_BOTTOM_INDEX; +import static com.facebook.react.uimanager.UIManagerHelper.PADDING_END_INDEX; +import static com.facebook.react.uimanager.UIManagerHelper.PADDING_START_INDEX; +import static com.facebook.react.uimanager.UIManagerHelper.PADDING_TOP_INDEX; import static com.facebook.react.uimanager.common.UIManagerType.FABRIC; import android.annotation.SuppressLint; @@ -73,6 +77,7 @@ import com.facebook.react.uimanager.ReactRoot; import com.facebook.react.uimanager.ReactRootViewTagGenerator; import com.facebook.react.uimanager.StateWrapper; import com.facebook.react.uimanager.ThemedReactContext; +import com.facebook.react.uimanager.UIManagerHelper; import com.facebook.react.uimanager.ViewManagerPropertyUpdater; import com.facebook.react.uimanager.ViewManagerRegistry; import com.facebook.react.uimanager.events.EventDispatcher; @@ -496,6 +501,32 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { attachmentsPositions); } + /** + * @param surfaceID {@link int} surface ID + * @param defaultTextInputPadding {@link float[]} output parameter will contain the default theme + * padding used by RN Android TextInput. + * @return if theme data is available in the output parameters. + */ + @DoNotStrip + public boolean getThemeData(int surfaceID, float[] defaultTextInputPadding) { + ThemedReactContext themedReactContext = mReactContextForRootTag.get(surfaceID); + if (themedReactContext == null) { + // TODO T68526882: Review if this should cause a crash instead. + ReactSoftException.logSoftException( + TAG, + new ReactNoCrashSoftException( + "Unable to find ThemedReactContext associated to surfaceID: " + surfaceID)); + return false; + } + float[] defaultTextInputPaddingForTheme = + UIManagerHelper.getDefaultTextInputPadding(themedReactContext); + defaultTextInputPadding[0] = defaultTextInputPaddingForTheme[PADDING_START_INDEX]; + defaultTextInputPadding[1] = defaultTextInputPaddingForTheme[PADDING_END_INDEX]; + defaultTextInputPadding[2] = defaultTextInputPaddingForTheme[PADDING_TOP_INDEX]; + defaultTextInputPadding[3] = defaultTextInputPaddingForTheme[PADDING_BOTTOM_INDEX]; + return true; + } + @Override @UiThread @ThreadConfined(UI) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index 7eb300c1924..3bb5ad4e167 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -43,9 +43,6 @@ struct JMountItem : public JavaClass { "Lcom/facebook/react/fabric/mounting/mountitems/MountItem;"; }; -static constexpr auto UIManagerJavaDescriptor = - "com/facebook/react/fabric/FabricUIManager"; - struct RemoveDeleteMetadata { Tag tag; Tag parentTag; @@ -366,7 +363,7 @@ local_ref createUpdateEventEmitterMountItem( cEventEmitter->eventEmitter = eventEmitter; static auto updateEventEmitterInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod(jint, jobject)>( "updateEventEmitterMountItem"); @@ -387,7 +384,7 @@ local_ref createUpdatePropsMountItem( local_ref readableMap = castReadableMap(ReadableNativeMap::newObjectCxxArgs(newProps)); static auto updatePropsInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod(jint, ReadableMap::javaobject)>( "updatePropsMountItem"); @@ -404,7 +401,7 @@ local_ref createUpdateLayoutMountItem( if (newChildShadowView.layoutMetrics != EmptyLayoutMetrics && oldChildShadowView.layoutMetrics != newChildShadowView.layoutMetrics) { static auto updateLayoutInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod( jint, jint, jint, jint, jint, jint)>("updateLayoutMountItem"); auto layoutMetrics = newChildShadowView.layoutMetrics; @@ -436,7 +433,7 @@ local_ref createUpdatePaddingMountItem( } static auto updateLayoutInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod(jint, jint, jint, jint, jint)>( "updatePaddingMountItem"); @@ -457,7 +454,7 @@ local_ref createInsertMountItem( const jni::global_ref &javaUIManager, const ShadowViewMutation &mutation) { static auto insertInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod(jint, jint, jint)>( "insertMountItem"); @@ -472,7 +469,7 @@ local_ref createUpdateStateMountItem( const jni::global_ref &javaUIManager, const ShadowViewMutation &mutation) { static auto updateStateInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod(jint, jobject)>( "updateStateMountItem"); @@ -498,7 +495,7 @@ local_ref createRemoveMountItem( const jni::global_ref &javaUIManager, const ShadowViewMutation &mutation) { static auto removeInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod(jint, jint, jint)>( "removeMountItem"); @@ -513,7 +510,7 @@ local_ref createDeleteMountItem( const jni::global_ref &javaUIManager, const ShadowViewMutation &mutation) { static auto deleteInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod(jint)>("deleteMountItem"); return deleteInstruction(javaUIManager, mutation.oldChildShadowView.tag); @@ -536,7 +533,7 @@ local_ref createRemoveAndDeleteMultiMountItem( } static auto removeDeleteMultiInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod(jintArray)>( "removeDeleteMultiMountItem"); @@ -561,7 +558,7 @@ local_ref createCreateMountItem( const ShadowViewMutation &mutation, const Tag surfaceId) { static auto createJavaInstruction = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod( jstring, ReadableMap::javaobject, jobject, jint, jint, jboolean)>( "createMountItem"); @@ -850,7 +847,7 @@ void Binding::schedulerDidFinishTransaction( } static auto createMountItemsBatchContainer = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod( jint, jtypeArray, jint, jint)>( "createBatchMountItem"); @@ -864,17 +861,18 @@ void Binding::schedulerDidFinishTransaction( position, commitNumber); - static auto scheduleMountItem = jni::findClassStatic(UIManagerJavaDescriptor) - ->getMethod("scheduleMountItem"); + static auto scheduleMountItem = + jni::findClassStatic(Binding::UIManagerJavaDescriptor) + ->getMethod("scheduleMountItem"); auto finishTransactionEndTime = telemetryTimePointNow(); @@ -903,7 +901,7 @@ void Binding::onAnimationStarted() { } static auto layoutAnimationsStartedJNI = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod("onAnimationStarted"); layoutAnimationsStartedJNI(localJavaUIManager); @@ -916,7 +914,7 @@ void Binding::onAllAnimationsComplete() { } static auto allAnimationsCompleteJNI = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod("onAllAnimationsComplete"); allAnimationsCompleteJNI(localJavaUIManager); @@ -947,7 +945,7 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation( } static auto preallocateView = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod( "preallocateView"); @@ -988,7 +986,7 @@ void Binding::schedulerDidDispatchCommand( } static auto dispatchCommand = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod( "dispatchCommand"); @@ -1013,7 +1011,7 @@ void Binding::schedulerDidSetJSResponder( } static auto setJSResponder = - jni::findClassStatic(UIManagerJavaDescriptor) + jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod("setJSResponder"); setJSResponder( @@ -1031,8 +1029,9 @@ void Binding::schedulerDidClearJSResponder() { return; } - static auto clearJSResponder = jni::findClassStatic(UIManagerJavaDescriptor) - ->getMethod("clearJSResponder"); + static auto clearJSResponder = + jni::findClassStatic(Binding::UIManagerJavaDescriptor) + ->getMethod("clearJSResponder"); clearJSResponder(localJavaUIManager); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h index 27f0fba1b9f..e42883d35ca 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h @@ -32,6 +32,9 @@ class Binding : public jni::HybridClass, constexpr static const char *const kJavaDescriptor = "Lcom/facebook/react/fabric/Binding;"; + constexpr static auto UIManagerJavaDescriptor = + "com/facebook/react/fabric/FabricUIManager"; + static void registerNatives(); private: diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerHelper.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerHelper.java index bd978336c2d..30133a53888 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerHelper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerHelper.java @@ -13,7 +13,9 @@ import static com.facebook.react.uimanager.common.ViewUtil.getUIManagerType; import android.content.Context; import android.content.ContextWrapper; import android.view.View; +import android.widget.EditText; import androidx.annotation.Nullable; +import androidx.core.view.ViewCompat; import com.facebook.react.bridge.CatalystInstance; import com.facebook.react.bridge.JSIModuleType; import com.facebook.react.bridge.ReactContext; @@ -27,6 +29,11 @@ import com.facebook.react.uimanager.events.EventDispatcherProvider; /** Helper class for {@link UIManager}. */ public class UIManagerHelper { + public static final int PADDING_START_INDEX = 0; + public static final int PADDING_END_INDEX = 1; + public static final int PADDING_TOP_INDEX = 2; + public static final int PADDING_BOTTOM_INDEX = 3; + /** @return a {@link UIManager} that can handle the react tag received by parameter. */ @Nullable public static UIManager getUIManagerForReactTag(ReactContext context, int reactTag) { @@ -110,4 +117,18 @@ public class UIManagerHelper { } return (ReactContext) context; } + + /** + * @return the default padding used by Android EditText's. This method returns the padding in an + * array to avoid extra classloading during hot-path of RN Android. + */ + public static float[] getDefaultTextInputPadding(ThemedReactContext context) { + EditText editText = new EditText(context); + float[] padding = new float[4]; + padding[PADDING_START_INDEX] = PixelUtil.toDIPFromPixel(ViewCompat.getPaddingStart(editText)); + padding[PADDING_END_INDEX] = PixelUtil.toDIPFromPixel(ViewCompat.getPaddingEnd(editText)); + padding[PADDING_TOP_INDEX] = PixelUtil.toDIPFromPixel(editText.getPaddingTop()); + padding[PADDING_BOTTOM_INDEX] = PixelUtil.toDIPFromPixel(editText.getPaddingBottom()); + return padding; + } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java index 8b5b13a6a5b..05084627a6d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java @@ -7,6 +7,10 @@ package com.facebook.react.views.textinput; +import static com.facebook.react.uimanager.UIManagerHelper.PADDING_BOTTOM_INDEX; +import static com.facebook.react.uimanager.UIManagerHelper.PADDING_END_INDEX; +import static com.facebook.react.uimanager.UIManagerHelper.PADDING_START_INDEX; +import static com.facebook.react.uimanager.UIManagerHelper.PADDING_TOP_INDEX; import static com.facebook.react.uimanager.UIManagerHelper.getReactContext; import android.content.Context; @@ -31,7 +35,6 @@ import android.widget.EditText; import android.widget.TextView; import androidx.annotation.Nullable; import androidx.core.content.ContextCompat; -import androidx.core.view.ViewCompat; import com.facebook.common.logging.FLog; import com.facebook.infer.annotation.Assertions; import com.facebook.react.bridge.Dynamic; @@ -1248,7 +1251,6 @@ public class ReactTextInputManager extends BaseViewManager #include #include "AndroidTextInputShadowNode.h" @@ -48,6 +49,26 @@ class AndroidTextInputComponentDescriptor final defaultThemePaddingEnd = ((YGValue)theme[YGEdgeEnd]).value; defaultThemePaddingTop = ((YGValue)theme[YGEdgeTop]).value; defaultThemePaddingBottom = ((YGValue)theme[YGEdgeBottom]).value; + } else { + const jni::global_ref &fabricUIManager = + contextContainer_->at>("FabricUIManager"); + + auto env = jni::Environment::current(); + auto defaultTextInputPaddingArray = env->NewFloatArray(4); + static auto getThemeData = + jni::findClassStatic(UIManagerJavaDescriptor) + ->getMethod("getThemeData"); + + if (getThemeData( + fabricUIManager, surfaceId, defaultTextInputPaddingArray)) { + jfloat *defaultTextInputPadding = + env->GetFloatArrayElements(defaultTextInputPaddingArray, 0); + defaultThemePaddingStart = defaultTextInputPadding[0]; + defaultThemePaddingEnd = defaultTextInputPadding[1]; + defaultThemePaddingTop = defaultTextInputPadding[2]; + defaultThemePaddingBottom = defaultTextInputPadding[3]; + } + env->DeleteLocalRef(defaultTextInputPaddingArray); } return std::make_shared( @@ -177,6 +198,10 @@ class AndroidTextInputComponentDescriptor final } private: + // TODO T68526882: Unify with Binding::UIManagerJavaDescriptor + constexpr static auto UIManagerJavaDescriptor = + "com/facebook/react/fabric/FabricUIManager"; + SharedTextLayoutManager textLayoutManager_; mutable better::map surfaceIdToThemePaddingMap_; };