From 890970a61ff893ea76fc3e7f8abd0c35e9d3fc92 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Mon, 16 Sep 2024 19:14:53 -0700 Subject: [PATCH] Remove ReactImageView Legacy Background Path (#46167) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/46167 ## This Diff This removes the legacy path from ReactImageView and its view manager. ## This Stack This removes the non-Style-applicator background management paths of the different native components. There have been multiple conflicting changes, and bugs added bc harder to reason about, which motivates making this change as soon as possible. This also lets us formalize guarantees that BaseViewManager may safely manipulate background styling of all built in native components. There is one still known issue, where BackgroundStyleApplicator does not propagate I18nManager derived layout direction to borders (compared to Android derived root direction). This is mostly an issue for apps that with LTR and RTL context, or force a layout direction, which I would guess is relatively rare, so my plan is to forward fix this later this by enabling set_android_layout_direction which will solve that problem mopre generically. Changelog: [Internal] Reviewed By: cortinico Differential Revision: D61657253 fbshipit-source-id: 96cf1160e466de78c2f133f0e4fb2d9b2e7cf478 --- .../react/views/image/ReactImageManager.kt | 50 ++------ .../react/views/image/ReactImageView.kt | 112 ++---------------- .../views/image/ReactImagePropertyTest.kt | 62 +--------- 3 files changed, 24 insertions(+), 200 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageManager.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageManager.kt index dd5691fe380..0a93ec043fc 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageManager.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageManager.kt @@ -16,12 +16,10 @@ import com.facebook.drawee.controller.AbstractDraweeControllerBuilder import com.facebook.react.bridge.ReadableArray import com.facebook.react.bridge.ReadableMap import com.facebook.react.common.ReactConstants -import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags import com.facebook.react.module.annotations.ReactModule import com.facebook.react.uimanager.BackgroundStyleApplicator import com.facebook.react.uimanager.LengthPercentage import com.facebook.react.uimanager.LengthPercentageType -import com.facebook.react.uimanager.PixelUtil.dpToPx import com.facebook.react.uimanager.SimpleViewManager import com.facebook.react.uimanager.ThemedReactContext import com.facebook.react.uimanager.ViewProps @@ -141,15 +139,7 @@ public constructor( @ReactProp(name = "borderColor", customType = "Color") public fun setBorderColor(view: ReactImageView, borderColor: Int?) { - if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) { - BackgroundStyleApplicator.setBorderColor(view, LogicalEdge.ALL, borderColor) - } else { - if (borderColor == null) { - view.setBorderColor(Color.TRANSPARENT) - } else { - view.setBorderColor(borderColor) - } - } + BackgroundStyleApplicator.setBorderColor(view, LogicalEdge.ALL, borderColor) } @ReactProp(name = "overlayColor", customType = "Color") @@ -163,11 +153,7 @@ public constructor( @ReactProp(name = "borderWidth") public fun setBorderWidth(view: ReactImageView, borderWidth: Float) { - if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) { - BackgroundStyleApplicator.setBorderWidth(view, LogicalEdge.ALL, borderWidth) - } else { - view.setBorderWidth(borderWidth) - } + BackgroundStyleApplicator.setBorderWidth(view, LogicalEdge.ALL, borderWidth) } @ReactPropGroup( @@ -180,24 +166,10 @@ public constructor( ViewProps.BORDER_BOTTOM_LEFT_RADIUS], defaultFloat = Float.NaN) public fun setBorderRadius(view: ReactImageView, index: Int, borderRadius: Float) { - if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) { - val radius = - if (borderRadius.isNaN()) null - else LengthPercentage(borderRadius, LengthPercentageType.POINT) - BackgroundStyleApplicator.setBorderRadius(view, BorderRadiusProp.values()[index], radius) - } else { - val convertedBorderRadius = - if (!borderRadius.isNaN()) { - borderRadius.dpToPx() - } else { - borderRadius - } - if (index == 0) { - view.setBorderRadius(convertedBorderRadius) - } else { - view.setBorderRadius(convertedBorderRadius, index - 1) - } - } + val radius = + if (borderRadius.isNaN()) null + else LengthPercentage(borderRadius, LengthPercentageType.POINT) + BackgroundStyleApplicator.setBorderRadius(view, BorderRadiusProp.values()[index], radius) } @ReactProp(name = ViewProps.RESIZE_MODE) @@ -262,20 +234,14 @@ public constructor( @ReactProp(name = ViewProps.BOX_SHADOW, customType = "BoxShadow") public fun setBoxShadow(view: ReactImageView, shadows: ReadableArray?): Unit { - if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) { - BackgroundStyleApplicator.setBoxShadow(view, shadows) - } + BackgroundStyleApplicator.setBoxShadow(view, shadows) } public override fun setBackgroundColor( view: ReactImageView, @ColorInt backgroundColor: Int ): Unit { - if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) { - BackgroundStyleApplicator.setBackgroundColor(view, backgroundColor) - } else { - super.setBackgroundColor(view, backgroundColor) - } + BackgroundStyleApplicator.setBackgroundColor(view, backgroundColor) } public override fun getExportedCustomDirectEventTypeConstants(): MutableMap = diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt index 879f4c88f5a..318dddfd717 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt @@ -28,7 +28,6 @@ import com.facebook.drawee.controller.AbstractDraweeControllerBuilder import com.facebook.drawee.controller.ControllerListener import com.facebook.drawee.controller.ForwardingControllerListener import com.facebook.drawee.drawable.AutoRotateDrawable -import com.facebook.drawee.drawable.RoundedColorDrawable import com.facebook.drawee.drawable.ScalingUtils import com.facebook.drawee.generic.GenericDraweeHierarchyBuilder import com.facebook.drawee.generic.RoundingParams @@ -49,17 +48,13 @@ import com.facebook.react.common.annotations.UnstableReactNativeAPI import com.facebook.react.common.annotations.VisibleForTesting import com.facebook.react.common.build.ReactBuildConfig import com.facebook.react.config.ReactFeatureFlags -import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags.enableBackgroundStyleApplicator import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags.loadVectorDrawablesOnImages -import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags.useNewReactImageViewBackgroundDrawing import com.facebook.react.modules.fresco.ReactNetworkImageRequest import com.facebook.react.uimanager.BackgroundStyleApplicator -import com.facebook.react.uimanager.FloatUtil.floatsEqual import com.facebook.react.uimanager.LengthPercentage import com.facebook.react.uimanager.LengthPercentageType import com.facebook.react.uimanager.PixelUtil.dpToPx import com.facebook.react.uimanager.PixelUtil.pxToDp -import com.facebook.react.uimanager.Spacing import com.facebook.react.uimanager.UIManagerHelper import com.facebook.react.uimanager.style.BorderRadiusProp import com.facebook.react.uimanager.style.LogicalEdge @@ -76,8 +71,6 @@ import com.facebook.react.views.imagehelper.ImageSource import com.facebook.react.views.imagehelper.ImageSource.Companion.getTransparentBitmapImageSource import com.facebook.react.views.imagehelper.MultiSourceHelper.getBestSourceForSize import com.facebook.react.views.imagehelper.ResourceDrawableIdHelper.Companion.instance -import com.facebook.react.views.view.ReactViewBackgroundManager -import com.facebook.yoga.YogaConstants import kotlin.math.abs /** @@ -97,13 +90,7 @@ public class ReactImageView( private var cachedImageSource: ImageSource? = null private var defaultImageDrawable: Drawable? = null private var loadingImageDrawable: Drawable? = null - private var backgroundImageDrawable: RoundedColorDrawable? = null - private var backgroundColor = 0x00000000 - private var borderColor = 0 private var overlayColor = 0 - private var borderWidth = 0f - private var borderRadius = YogaConstants.UNDEFINED - private var borderCornerRadii: FloatArray? = null private var scaleType = defaultValue() private var tileMode = defaultTileMode() private var isDirty = false @@ -115,11 +102,9 @@ public class ReactImageView( private var progressiveRenderingEnabled = false private var headers: ReadableMap? = null private var resizeMultiplier = 1.0f - private val reactBackgroundManager = ReactViewBackgroundManager(this) private var resizeMethod = ImageResizeMethod.AUTO init { - reactBackgroundManager.setOverflow("hidden") // Workaround Android bug where ImageView visibility is not propagated to the Drawable, so you // have to manually update visibility. Will be resolved once we move to VitoView. setLegacyVisibilityHandlingEnabled(true) @@ -213,26 +198,11 @@ public class ReactImageView( } public override fun setBackgroundColor(backgroundColor: Int) { - if (enableBackgroundStyleApplicator()) { - BackgroundStyleApplicator.setBackgroundColor(this, backgroundColor) - } else if (useNewReactImageViewBackgroundDrawing()) { - reactBackgroundManager.backgroundColor = backgroundColor - } else if (this.backgroundColor != backgroundColor) { - this.backgroundColor = backgroundColor - backgroundImageDrawable = RoundedColorDrawable(backgroundColor) - isDirty = true - } + BackgroundStyleApplicator.setBackgroundColor(this, backgroundColor) } public fun setBorderColor(borderColor: Int) { - if (enableBackgroundStyleApplicator()) { - BackgroundStyleApplicator.setBorderColor(this, LogicalEdge.ALL, borderColor) - } else if (useNewReactImageViewBackgroundDrawing()) { - reactBackgroundManager.setBorderColor(Spacing.ALL, borderColor) - } else if (this.borderColor != borderColor) { - this.borderColor = borderColor - isDirty = true - } + BackgroundStyleApplicator.setBorderColor(this, LogicalEdge.ALL, borderColor) } public fun setOverlayColor(overlayColor: Int) { @@ -243,49 +213,21 @@ public class ReactImageView( } public fun setBorderWidth(borderWidth: Float) { - val newBorderWidth = borderWidth.dpToPx() - if (enableBackgroundStyleApplicator()) { - BackgroundStyleApplicator.setBorderWidth(this, LogicalEdge.ALL, borderWidth) - } else if (useNewReactImageViewBackgroundDrawing()) { - reactBackgroundManager.setBorderWidth(Spacing.ALL, newBorderWidth) - } else if (!floatsEqual(this.borderWidth, newBorderWidth)) { - this.borderWidth = newBorderWidth - isDirty = true - } + BackgroundStyleApplicator.setBorderWidth(this, LogicalEdge.ALL, borderWidth) } public fun setBorderRadius(borderRadius: Float) { - if (enableBackgroundStyleApplicator()) { - val radius = - if (borderRadius.isNaN()) null - else LengthPercentage(borderRadius.pxToDp(), LengthPercentageType.POINT) - BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.BORDER_RADIUS, radius) - } else if (useNewReactImageViewBackgroundDrawing()) { - reactBackgroundManager.setBorderRadius(borderRadius) - } else if (!floatsEqual(this.borderRadius, borderRadius)) { - this.borderRadius = borderRadius - isDirty = true - } + val radius = + if (borderRadius.isNaN()) null + else LengthPercentage(borderRadius.pxToDp(), LengthPercentageType.POINT) + BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.BORDER_RADIUS, radius) } public fun setBorderRadius(borderRadius: Float, position: Int) { - if (enableBackgroundStyleApplicator()) { - val radius = - if (borderRadius.isNaN()) null - else LengthPercentage(borderRadius.pxToDp(), LengthPercentageType.POINT) - BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.values()[position], radius) - } else if (useNewReactImageViewBackgroundDrawing()) { - reactBackgroundManager.setBorderRadius(borderRadius, position + 1) - } else { - if (borderCornerRadii == null) { - borderCornerRadii = FloatArray(4) { YogaConstants.UNDEFINED } - } - - if (!floatsEqual(borderCornerRadii?.get(position), borderRadius)) { - borderCornerRadii?.set(position, borderRadius) - isDirty = true - } - } + val radius = + if (borderRadius.isNaN()) null + else LengthPercentage(borderRadius.pxToDp(), LengthPercentageType.POINT) + BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.values()[position], radius) } public fun setScaleType(scaleType: ScalingUtils.ScaleType) { @@ -395,11 +337,7 @@ public class ReactImageView( public override fun hasOverlappingRendering(): Boolean = false public override fun onDraw(canvas: Canvas) { - if (enableBackgroundStyleApplicator()) { - BackgroundStyleApplicator.clipToPaddingBox(this, canvas) - } else if (useNewReactImageViewBackgroundDrawing()) { - reactBackgroundManager.maybeClipToPaddingBox(canvas) - } + BackgroundStyleApplicator.clipToPaddingBox(this, canvas) super.onDraw(canvas) } @@ -439,22 +377,8 @@ public class ReactImageView( hierarchy.setPlaceholderImage(loadingImageDrawable, ScalingUtils.ScaleType.CENTER) } - getCornerRadii(computedCornerRadii) - val roundingParams = hierarchy.roundingParams if (roundingParams != null) { - roundingParams.setCornersRadii( - computedCornerRadii[0], - computedCornerRadii[1], - computedCornerRadii[2], - computedCornerRadii[3]) - - backgroundImageDrawable?.let { background -> - background.setBorder(borderColor, borderWidth) - roundingParams.cornersRadii?.let { background.radii = it } - hierarchy.setBackgroundImage(background) - } - roundingParams.setBorder(borderColor, borderWidth) if (overlayColor != Color.TRANSPARENT) { roundingParams.setOverlayColor(overlayColor) } else { @@ -578,16 +502,6 @@ public class ReactImageView( } } - private fun getCornerRadii(computedCorners: FloatArray) { - val defaultBorderRadius = if (!YogaConstants.isUndefined(borderRadius)) borderRadius else 0f - - val radii = borderCornerRadii ?: FloatArray(4) { Float.NaN } - computedCorners[0] = if (!YogaConstants.isUndefined(radii[0])) radii[0] else defaultBorderRadius - computedCorners[1] = if (!YogaConstants.isUndefined(radii[1])) radii[1] else defaultBorderRadius - computedCorners[2] = if (!YogaConstants.isUndefined(radii[2])) radii[2] else defaultBorderRadius - computedCorners[3] = if (!YogaConstants.isUndefined(radii[3])) radii[3] else defaultBorderRadius - } - @VisibleForTesting public fun setControllerListener(controllerListener: ControllerListener?) { controllerForTesting = controllerListener @@ -709,8 +623,6 @@ public class ReactImageView( public companion object { public const val REMOTE_IMAGE_FADE_DURATION_MS: Int = 300 - private val computedCornerRadii = FloatArray(4) - // Fresco lacks support for repeating images, see https://github.com/facebook/fresco/issues/1575 // We implement it here as a postprocessing step. private val tileMatrix = Matrix() diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt index 0466c018ff0..f1329e88d8f 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt @@ -19,7 +19,7 @@ import com.facebook.react.bridge.JavaOnlyMap import com.facebook.react.bridge.ReactTestHelper.createMockCatalystInstance import com.facebook.react.bridge.WritableArray import com.facebook.react.bridge.WritableMap -import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags +import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests import com.facebook.react.uimanager.DisplayMetricsHolder import com.facebook.react.uimanager.ReactStylesDiffMap import com.facebook.react.uimanager.ThemedReactContext @@ -38,13 +38,7 @@ import org.mockito.Mockito.mockStatic import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment -/** - * Verify that [ScalingUtils] properties are being applied correctly by [ReactImageManager]. - * - * TODO T195191609: The border rendering tests rely on introspecting Fresco tree, which is no longer - * relevant when "useNewReactImageViewBackgroundDrawing" is enabled. These should be replaced by - * screenshot tests over the existing RNTester examples. - */ +/** Verify that [ScalingUtils] properties are being applied correctly by [ReactImageManager]. */ @RunWith(RobolectricTestRunner::class) class ReactImagePropertyTest { @@ -53,7 +47,6 @@ class ReactImagePropertyTest { private lateinit var themeContext: ThemedReactContext private lateinit var arguments: MockedStatic private lateinit var rnLog: MockedStatic - private lateinit var featureFlags: MockedStatic @Before fun setup() { @@ -64,12 +57,6 @@ class ReactImagePropertyTest { rnLog = mockStatic(RNLog::class.java) rnLog.`when` { RNLog.w(any(), anyString()) }.thenAnswer {} - // Avoid trying to load ReactNativeFeatureFlags JNI library - featureFlags = mockStatic(ReactNativeFeatureFlags::class.java) - featureFlags - .`when` { ReactNativeFeatureFlags.useNewReactImageViewBackgroundDrawing() } - .thenAnswer { false } - SoLoader.setInTestMode() context = BridgeReactContext(RuntimeEnvironment.getApplication()) catalystInstanceMock = createMockCatalystInstance() @@ -77,6 +64,8 @@ class ReactImagePropertyTest { themeContext = ThemedReactContext(context, context, null, -1) Fresco.initialize(context) DisplayMetricsHolder.setWindowDisplayMetrics(DisplayMetrics()) + + ReactNativeFeatureFlagsForTests.setUp() } @After @@ -84,55 +73,12 @@ class ReactImagePropertyTest { DisplayMetricsHolder.setWindowDisplayMetrics(null) arguments.close() rnLog.close() - featureFlags.close() } private fun buildStyles(vararg keysAndValues: Any?): ReactStylesDiffMap { return ReactStylesDiffMap(JavaOnlyMap.of(*keysAndValues)) } - @Test - fun testBorderColor() { - val viewManager = ReactImageManager() - val view = viewManager.createViewInstance(themeContext) - viewManager.updateProperties( - view, - buildStyles("src", JavaOnlyArray.of(JavaOnlyMap.of("uri", "http://mysite.com/mypic.jpg")))) - viewManager.updateProperties(view, buildStyles("borderColor", Color.argb(0, 0, 255, 255))) - var borderColor = view.hierarchy.roundingParams!!.borderColor - assertThat(Color.alpha(borderColor)).isEqualTo(0) - assertThat(Color.red(borderColor)).isEqualTo(0) - assertThat(Color.green(borderColor)).isEqualTo(255) - assertThat(Color.blue(borderColor)).isEqualTo(255) - viewManager.updateProperties(view, buildStyles("borderColor", Color.argb(0, 255, 50, 128))) - borderColor = view.hierarchy.roundingParams!!.borderColor - assertThat(Color.alpha(borderColor)).isEqualTo(0) - assertThat(Color.red(borderColor)).isEqualTo(255) - assertThat(Color.green(borderColor)).isEqualTo(50) - assertThat(Color.blue(borderColor)).isEqualTo(128) - viewManager.updateProperties(view, buildStyles("borderColor", null)) - borderColor = view.hierarchy.roundingParams!!.borderColor - assertThat(Color.alpha(borderColor)).isEqualTo(0) - assertThat(Color.red(borderColor)).isEqualTo(0) - assertThat(Color.green(borderColor)).isEqualTo(0) - assertThat(Color.blue(borderColor)).isEqualTo(0) - } - - @Test - fun testRoundedCorners() { - val viewManager = ReactImageManager() - val view = viewManager.createViewInstance(themeContext) - viewManager.updateProperties( - view, - buildStyles("src", JavaOnlyArray.of(JavaOnlyMap.of("uri", "http://mysite.com/mypic.jpg")))) - - // We can't easily verify if rounded corner was honored or not, this tests simply verifies - // we're not crashing.. - viewManager.updateProperties(view, buildStyles("borderRadius", 10.0)) - viewManager.updateProperties(view, buildStyles("borderRadius", 0.0)) - viewManager.updateProperties(view, buildStyles("borderRadius", null)) - } - @Test fun testAccessibilityFocus() { val viewManager = ReactImageManager()