Skip to content

Do not implicitly convert parsed LengthPercentage to pixels #45987

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.

Sign up for GitHub

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

Closed
NickGerleman wants to merge 2 commits into from
Closed

Do not implicitly convert parsed LengthPercentage to pixels #45987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42de92e
Clip to radii bounds in OutsetBoxShadowDrawable
NickGerleman Aug 12, 2024
131e3f1
Do not implicitly convert parsed LengthPercentage to pixels (#45987)
NickGerleman Aug 12, 2024
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
8 changes: 7 additions & 1 deletion packages/react-native/ReactAndroid/api/ReactAndroid.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <init> ()V
public fun <init> (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 {
Expand Down
1 change: 0 additions & 1 deletion ...tive/ReactAndroid/src/main/java/com/facebook/react/uimanager/BackgroundStyleApplicator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions .../react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/LengthPercentage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
}

Expand Down
16 changes: 10 additions & 6 deletions ...actAndroid/src/main/java/com/facebook/react/uimanager/drawable/CSSBackgroundDrawable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
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;
Expand Down Expand Up @@ -324,6 +325,9 @@ public float getInnerBorderRadius(float computedRadius, float borderWidth) {
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;
}
Expand Down Expand Up @@ -660,12 +664,12 @@ private void updatePath() {
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);
Expand Down
12 changes: 8 additions & 4 deletions ...eactAndroid/src/main/java/com/facebook/react/uimanager/drawable/InsetBoxShadowDrawable.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
19 changes: 15 additions & 4 deletions ...actAndroid/src/main/java/com/facebook/react/uimanager/drawable/OutsetBoxShadowDrawable.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import com.facebook.react.uimanager.FilterHelper
import com.facebook.react.uimanager.PixelUtil
import com.facebook.react.uimanager.style.BorderRadiusStyle
import com.facebook.react.uimanager.style.ComputedBorderRadius
import kotlin.math.ceil
import kotlin.math.roundToInt

private const val TAG = "OutsetBoxShadowDrawable"
Expand Down Expand Up @@ -54,7 +55,7 @@ internal class OutsetBoxShadowDrawable(

private val renderNode =
RenderNode(TAG).apply {
clipToBounds = false
clipToBounds = true
setRenderEffect(FilterHelper.createBlurEffect(blurRadius * BLUR_RADIUS_SIGMA_SCALE))
}

Expand All @@ -81,13 +82,21 @@ internal class OutsetBoxShadowDrawable(
}

val spreadExtent = PixelUtil.toPixelFromDIP(spread).roundToInt().coerceAtLeast(0)
val shadowOutset = ceil(PixelUtil.toPixelFromDIP(blurRadius))
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(
Expand Down Expand Up @@ -149,9 +158,11 @@ internal class OutsetBoxShadowDrawable(
offset(
PixelUtil.toPixelFromDIP(offsetX).roundToInt() - shadowShapeFrame.left,
PixelUtil.toPixelFromDIP(offsetY).roundToInt() - shadowShapeFrame.top)
inset(-shadowOutset.roundToInt(), -shadowOutset.roundToInt())
})

beginRecording().let { renderNodeCanvas ->
renderNodeCanvas.translate(shadowOutset, shadowOutset)
if (shadowBorderRadii?.hasRoundedBorders() == true) {
renderNodeCanvas.drawPath(shadowOuterPath, shadowPaint)
} else {
Expand Down
2 changes: 1 addition & 1 deletion ...act-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
5 changes: 3 additions & 2 deletions .../react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion ...ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1351,7 +1351,8 @@ public void setBorderRadius(float borderRadius, int position) {
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);
Expand Down
3 changes: 1 addition & 2 deletions ...droid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,7 @@ public void setBorderRadius(ReactHorizontalScrollView view, int index, float bor
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)) {
Expand Down
3 changes: 2 additions & 1 deletion ...ct-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1283,7 +1283,8 @@ public void setBorderRadius(float borderRadius, int position) {
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);
Expand Down
3 changes: 1 addition & 2 deletions ...ve/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,7 @@ public void setBorderRadius(ReactScrollView view, int index, float borderRadius)
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)) {
Expand Down
3 changes: 1 addition & 2 deletions .../ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextAnchorViewManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,7 @@ public void setBorderRadius(ReactTextView view, int index, float borderRadius) {
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)) {
Expand Down
3 changes: 2 additions & 1 deletion .../react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,8 @@ public void setBorderRadius(float borderRadius, int position) {
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);
Expand Down
3 changes: 2 additions & 1 deletion ...t-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,7 +1132,8 @@ public void setBorderRadius(float borderRadius, int position) {
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);
Expand Down
3 changes: 1 addition & 2 deletions .../ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,8 +962,7 @@ public void setBorderRadius(ReactEditText view, int index, float borderRadius) {
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)) {
Expand Down
21 changes: 13 additions & 8 deletions ...react-native/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,18 +994,23 @@ private void dispatchOverflowDraw(Canvas canvas) {
mPath = new Path();
}

float topLeftRadius = PixelUtil.toPixelFromDIP(borderRadius.getTopLeft());
float topRightRadius = PixelUtil.toPixelFromDIP(borderRadius.getTopRight());
float bottomLeftRadius = PixelUtil.toPixelFromDIP(borderRadius.getBottomLeft());
float bottomRightRadius = PixelUtil.toPixelFromDIP(borderRadius.getBottomRight());

mPath.rewind();
mPath.addRoundRect(
new RectF(left, top, right, bottom),
new float[] {
Math.max(borderRadius.getTopLeft() - borderWidth.left, 0),
Math.max(borderRadius.getTopLeft() - borderWidth.top, 0),
Math.max(borderRadius.getTopRight() - borderWidth.right, 0),
Math.max(borderRadius.getTopRight() - borderWidth.top, 0),
Math.max(borderRadius.getBottomRight() - borderWidth.right, 0),
Math.max(borderRadius.getBottomRight() - borderWidth.bottom, 0),
Math.max(borderRadius.getBottomLeft() - borderWidth.left, 0),
Math.max(borderRadius.getBottomLeft() - borderWidth.bottom, 0),
Math.max(topLeftRadius - borderWidth.left, 0),
Math.max(topLeftRadius - borderWidth.top, 0),
Math.max(topRightRadius - borderWidth.right, 0),
Math.max(topRightRadius - borderWidth.top, 0),
Math.max(bottomRightRadius - borderWidth.right, 0),
Math.max(bottomRightRadius - borderWidth.bottom, 0),
Math.max(bottomLeftRadius - borderWidth.left, 0),
Math.max(bottomLeftRadius - borderWidth.bottom, 0),
},
Path.Direction.CW);
canvas.clipPath(mPath);
Expand Down
2 changes: 1 addition & 1 deletion ...act-native/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public void setBorderRadius(ReactViewGroup view, int index, Dynamic rawBorderRad
// 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;
}

Expand Down