From 9e48976bc2864c6ef6995179a7dc8d1c453dcf39 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Mon, 12 Aug 2024 21:41:13 -0700 Subject: [PATCH] Do not implicitly convert parsed LengthPercentage to pixels (#45987) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/45987 This is a confusing public API, because styles layer deals with DIPs, conversion only happens when parsing dynamic, and `POINT` (the `LengthPercentageType`) also maps to DIPs instead of physical pixels. This moves conversion to physical pixels to drawing layer, so everything above `BackgroundStyleApplicator` works with `style` types which are all in DIPs. To preserve compatibility with existing APIs using raw radii, we keep it so that (most) views operate in pixel units, while view managers operate under DIPs. Changelog: [Android][Breaking] Do not implicitly convert parsed LengthPercentage to pixels Reviewed By: rshest Differential Revision: D60507151 fbshipit-source-id: b90066af7b221304aded374627fc0e2165dfc08f --- .../ReactAndroid/api/ReactAndroid.api | 8 ++++++- .../uimanager/BackgroundStyleApplicator.kt | 1 - .../react/uimanager/LengthPercentage.kt | 8 +++---- .../drawable/CSSBackgroundDrawable.java | 16 ++++++++------ .../drawable/InsetBoxShadowDrawable.kt | 12 +++++++---- .../drawable/OutsetBoxShadowDrawable.kt | 13 +++++++++--- .../react/views/image/ReactImageManager.kt | 2 +- .../react/views/image/ReactImageView.kt | 5 +++-- .../scroll/ReactHorizontalScrollView.java | 3 ++- .../ReactHorizontalScrollViewManager.java | 3 +-- .../react/views/scroll/ReactScrollView.java | 3 ++- .../views/scroll/ReactScrollViewManager.java | 3 +-- .../text/ReactTextAnchorViewManager.java | 3 +-- .../react/views/text/ReactTextView.java | 3 ++- .../react/views/textinput/ReactEditText.java | 3 ++- .../textinput/ReactTextInputManager.java | 3 +-- .../react/views/view/ReactViewGroup.java | 21 ++++++++++++------- .../react/views/view/ReactViewManager.java | 2 +- 18 files changed, 69 insertions(+), 43 deletions(-) diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api index 1ddf3d0d010..175a526d3ae 100644 --- a/packages/react-native/ReactAndroid/api/ReactAndroid.api +++ b/packages/react-native/ReactAndroid/api/ReactAndroid.api @@ -4359,9 +4359,15 @@ public final class com/facebook/react/uimanager/LengthPercentage { public static final field Companion Lcom/facebook/react/uimanager/LengthPercentage$Companion; public fun ()V public fun (FLcom/facebook/react/uimanager/LengthPercentageType;)V - public final fun getUnit ()Lcom/facebook/react/uimanager/LengthPercentageType; + public final fun component2 ()Lcom/facebook/react/uimanager/LengthPercentageType; + public final fun copy (FLcom/facebook/react/uimanager/LengthPercentageType;)Lcom/facebook/react/uimanager/LengthPercentage; + public static synthetic fun copy$default (Lcom/facebook/react/uimanager/LengthPercentage;FLcom/facebook/react/uimanager/LengthPercentageType;ILjava/lang/Object;)Lcom/facebook/react/uimanager/LengthPercentage; + public fun equals (Ljava/lang/Object;)Z + public final fun getType ()Lcom/facebook/react/uimanager/LengthPercentageType; + public fun hashCode ()I public final fun resolve (FF)F public static final fun setFromDynamic (Lcom/facebook/react/bridge/Dynamic;)Lcom/facebook/react/uimanager/LengthPercentage; + public fun toString ()Ljava/lang/String; } public final class com/facebook/react/uimanager/LengthPercentage$Companion { diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BackgroundStyleApplicator.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BackgroundStyleApplicator.kt index a2c71bfad6f..3365481f1e4 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BackgroundStyleApplicator.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BackgroundStyleApplicator.kt @@ -74,7 +74,6 @@ public object BackgroundStyleApplicator { public fun setBorderRadius( view: View, corner: BorderRadiusProp, - // TODO: LengthPercentage silently converts from pixels to DIPs before here already radius: LengthPercentage? ): Unit { ensureCSSBackground(view).setBorderRadius(corner, radius) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/LengthPercentage.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/LengthPercentage.kt index c4b18fc50a1..158334aed02 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/LengthPercentage.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/LengthPercentage.kt @@ -18,9 +18,9 @@ public enum class LengthPercentageType { PERCENT, } -public class LengthPercentage( +public data class LengthPercentage( private val value: Float, - public val unit: LengthPercentageType, + public val type: LengthPercentageType, ) { public companion object { @JvmStatic @@ -29,7 +29,7 @@ public class LengthPercentage( ReadableType.Number -> { val value = dynamic.asDouble() if (value >= 0f) { - LengthPercentage(PixelUtil.toPixelFromDIP(value), LengthPercentageType.POINT) + LengthPercentage(value.toFloat(), LengthPercentageType.POINT) } else { null } @@ -62,7 +62,7 @@ public class LengthPercentage( } public fun resolve(width: Float, height: Float): Float { - if (unit == LengthPercentageType.PERCENT) { + if (type == LengthPercentageType.PERCENT) { return (value / 100) * Math.min(width, height) } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/CSSBackgroundDrawable.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/CSSBackgroundDrawable.java index 05dbfb0ff70..87c38e86266 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/CSSBackgroundDrawable.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/CSSBackgroundDrawable.java @@ -36,6 +36,7 @@ import com.facebook.react.modules.i18nmanager.I18nUtil; import com.facebook.react.uimanager.FloatUtil; import com.facebook.react.uimanager.LengthPercentage; import com.facebook.react.uimanager.LengthPercentageType; +import com.facebook.react.uimanager.PixelUtil; import com.facebook.react.uimanager.Spacing; import com.facebook.react.uimanager.style.BorderRadiusProp; import com.facebook.react.uimanager.style.BorderRadiusStyle; @@ -324,6 +325,9 @@ public class CSSBackgroundDrawable extends Drawable { return Math.max(computedRadius - borderWidth, 0); } + // TODO: This API is unsafe and should be removed when + // BackgroundStyleApplicator is rolled out + @Deprecated(forRemoval = true, since = "0.76.0") public ComputedBorderRadius getComputedBorderRadius() { return mComputedBorderRadius; } @@ -660,12 +664,12 @@ public class CSSBackgroundDrawable extends Drawable { mBorderRadius.resolve( getLayoutDirection(), mContext, - mOuterClipTempRectForBorderRadius.width(), - mOuterClipTempRectForBorderRadius.height()); - float topLeftRadius = mComputedBorderRadius.getTopLeft(); - float topRightRadius = mComputedBorderRadius.getTopRight(); - float bottomLeftRadius = mComputedBorderRadius.getBottomLeft(); - float bottomRightRadius = mComputedBorderRadius.getBottomRight(); + PixelUtil.toDIPFromPixel(mOuterClipTempRectForBorderRadius.width()), + PixelUtil.toDIPFromPixel(mOuterClipTempRectForBorderRadius.height())); + float topLeftRadius = PixelUtil.toPixelFromDIP(mComputedBorderRadius.getTopLeft()); + float topRightRadius = PixelUtil.toPixelFromDIP(mComputedBorderRadius.getTopRight()); + float bottomLeftRadius = PixelUtil.toPixelFromDIP(mComputedBorderRadius.getBottomLeft()); + float bottomRightRadius = PixelUtil.toPixelFromDIP(mComputedBorderRadius.getBottomRight()); final float innerTopLeftRadiusX = getInnerBorderRadius(topLeftRadius, borderWidth.left); final float innerTopLeftRadiusY = getInnerBorderRadius(topLeftRadius, borderWidth.top); diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/InsetBoxShadowDrawable.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/InsetBoxShadowDrawable.kt index 5e358094c86..0d1ee9c9b1b 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/InsetBoxShadowDrawable.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/InsetBoxShadowDrawable.kt @@ -130,17 +130,21 @@ internal class InsetBoxShadowDrawable( } } + // TODO: Remove usage of unsafe `CSSBackgroundDrawable.getComputedBorderRadius` + @Suppress("DEPRECATION") private fun getClearRegionBorderRadii( spread: Int, background: CSSBackgroundDrawable, ): BorderRadiusStyle { + // Accessing this is super duper unsafe and only works because the CSSBackgroundDrawable renders + // first val computedBorderRadii = background.computedBorderRadius val borderWidth = background.getDirectionAwareBorderInsets() - val topLeftRadius = computedBorderRadii.topLeft - val topRightRadius = computedBorderRadii.topRight - val bottomLeftRadius = computedBorderRadii.bottomLeft - val bottomRightRadius = computedBorderRadii.bottomRight + val topLeftRadius = PixelUtil.toPixelFromDIP(computedBorderRadii.topLeft) + val topRightRadius = PixelUtil.toPixelFromDIP(computedBorderRadii.topRight) + val bottomLeftRadius = PixelUtil.toPixelFromDIP(computedBorderRadii.bottomLeft) + val bottomRightRadius = PixelUtil.toPixelFromDIP(computedBorderRadii.bottomRight) val innerTopLeftRadius = background.getInnerBorderRadius(topLeftRadius, borderWidth.left) val innerTopRightRadius = background.getInnerBorderRadius(topRightRadius, borderWidth.right) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/OutsetBoxShadowDrawable.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/OutsetBoxShadowDrawable.kt index a3566d13d76..3a635ebc69b 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/OutsetBoxShadowDrawable.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/drawable/OutsetBoxShadowDrawable.kt @@ -86,10 +86,17 @@ internal class OutsetBoxShadowDrawable( val shadowShapeFrame = Rect(bounds).apply { inset(-spreadExtent, -spreadExtent) } val shadowShapeBounds = Rect(0, 0, shadowShapeFrame.width(), shadowShapeFrame.height()) - val resolutionWidth = bounds.width().toFloat() - val resolutionHeight = bounds.height().toFloat() + val resolutionWidth = PixelUtil.toDIPFromPixel(bounds.width().toFloat()) + val resolutionHeight = PixelUtil.toDIPFromPixel(bounds.height().toFloat()) val computedBorderRadii = - borderRadius?.resolve(layoutDirection, context, resolutionWidth, resolutionHeight) + borderRadius?.resolve(layoutDirection, context, resolutionWidth, resolutionHeight)?.let { + ComputedBorderRadius( + topLeft = PixelUtil.toPixelFromDIP(it.topLeft), + topRight = PixelUtil.toPixelFromDIP(it.topRight), + bottomLeft = PixelUtil.toPixelFromDIP(it.bottomLeft), + bottomRight = PixelUtil.toPixelFromDIP(it.bottomRight)) + } + val shadowBorderRadii = computedBorderRadii?.let { radii -> ComputedBorderRadius( 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 0686e708201..204d4ce4bbd 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 @@ -183,7 +183,7 @@ public constructor( if (ReactNativeFeatureFlags.enableBackgroundStyleApplicator()) { val radius = if (borderRadius.isNaN()) null - else LengthPercentage(toPixelFromDIP(borderRadius), LengthPercentageType.POINT) + else LengthPercentage(borderRadius, LengthPercentageType.POINT) BackgroundStyleApplicator.setBorderRadius(view, BorderRadiusProp.values()[index], radius) } else { val convertedBorderRadius = 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 32ca0defe75..f9dc66e0171 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 @@ -57,6 +57,7 @@ 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.toDIPFromPixel import com.facebook.react.uimanager.PixelUtil.toPixelFromDIP import com.facebook.react.uimanager.Spacing import com.facebook.react.uimanager.UIManagerHelper @@ -257,7 +258,7 @@ public class ReactImageView( if (enableBackgroundStyleApplicator()) { val radius = if (borderRadius.isNaN()) null - else LengthPercentage(borderRadius, LengthPercentageType.POINT) + else LengthPercentage(toDIPFromPixel(borderRadius), LengthPercentageType.POINT) BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.BORDER_RADIUS, radius) } else if (useNewReactImageViewBackgroundDrawing()) { reactBackgroundManager.setBorderRadius(borderRadius) @@ -271,7 +272,7 @@ public class ReactImageView( if (enableBackgroundStyleApplicator()) { val radius = if (borderRadius.isNaN()) null - else LengthPercentage(borderRadius, LengthPercentageType.POINT) + else LengthPercentage(toDIPFromPixel(borderRadius), LengthPercentageType.POINT) BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.values()[position], radius) } else if (useNewReactImageViewBackgroundDrawing()) { reactBackgroundManager.setBorderRadius(borderRadius, position + 1) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java index b1eac53ef9f..1d7800d648a 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java @@ -1351,7 +1351,8 @@ public class ReactHorizontalScrollView extends HorizontalScrollView LengthPercentage radius = Float.isNaN(borderRadius) ? null - : new LengthPercentage(borderRadius, LengthPercentageType.POINT); + : new LengthPercentage( + PixelUtil.toDIPFromPixel(borderRadius), LengthPercentageType.POINT); BackgroundStyleApplicator.setBorderRadius(this, BorderRadiusProp.values()[position], radius); } else { mReactBackgroundManager.setBorderRadius(borderRadius, position); diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java index 9eae96b6ef5..861ee180a91 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java @@ -269,8 +269,7 @@ public class ReactHorizontalScrollViewManager extends ViewGroupManager LengthPercentage radius = Float.isNaN(borderRadius) ? null - : new LengthPercentage( - PixelUtil.toPixelFromDIP(borderRadius), LengthPercentageType.POINT); + : new LengthPercentage(borderRadius, LengthPercentageType.POINT); BackgroundStyleApplicator.setBorderRadius(view, BorderRadiusProp.values()[index], radius); } else { if (!Float.isNaN(borderRadius)) { diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java index bd307c3eeee..7149f570d17 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java @@ -158,8 +158,7 @@ public abstract class ReactTextAnchorViewManager { // avoid developer surprise if it works on one platform but not another). if (ViewUtil.getUIManagerType(view) != UIManagerType.FABRIC && borderRadius != null - && borderRadius.getUnit() == LengthPercentageType.PERCENT) { + && borderRadius.getType() == LengthPercentageType.PERCENT) { borderRadius = null; }