Skip to content

Commit

Permalink
Do not implicitly convert parsed LengthPercentage to pixels (#45987)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #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
  • Loading branch information
NickGerleman authored and facebook-github-bot committed Aug 13, 2024
1 parent 3ee9417 commit 9e48976
Show file tree
Hide file tree
Showing 18 changed files with 69 additions and 43 deletions.
8 changes: 7 additions & 1 deletion packages/react-native/ReactAndroid/api/ReactAndroid.api
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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
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
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
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
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
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
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
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
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
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
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
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
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

0 comments on commit 9e48976

Please sign in to comment.