Skip to content
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.

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

feat: radial gradient android changes #50269

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,22 @@ import com.facebook.react.bridge.ReadableMap

public class BackgroundImageLayer(gradientMap: ReadableMap?, context: Context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we make this internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're using this in BackgroundStyleApplicator so need to keep it public! 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite following. E.g. BackgroundStyleApplicator as the public API relies on private internals like OutsetBoxShadowDrawable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the issue is that it is being exposed as an argument to a public function. So when making it internal it results in below.

private val gradient: Gradient? =
if (gradientMap != null) {
try {
Gradient(gradientMap, context)
} catch (e: IllegalArgumentException) {
// Gracefully reject invalid styles
null
if (gradientMap != null) {
val typeString = gradientMap.getString("type")
try {
when (typeString) {
"linearGradient" -> LinearGradient(gradientMap, context)
"radialGradient" -> RadialGradient(gradientMap, context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment on the JS PR, but we might want to change these for consistency

Suggested change
"linearGradient" -> LinearGradient(gradientMap, context)
"radialGradient" -> RadialGradient(gradientMap, context)
"linear-gradient" -> LinearGradient(gradientMap, context)
"radial-gradient" -> RadialGradient(gradientMap, context)

else -> null
}
} else {
} catch (e: IllegalArgumentException) {
// Gracefully reject invalid styles
null
}
} else {
null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is a little tricky to read. Consider

  1. Early return null on null (or, only accept non-null? Seems like caller would need to check that anyways)
  2. Explicitly check data types and return null instead of swallowing exception. That could help avoid over-swallowing logic errors too

}

public fun getShader(bounds: Rect): Shader? = gradient?.getShader(bounds)
public fun getShader(bounds: Rect): Shader? =
gradient?.getShader(bounds.width().toFloat(), bounds.height().toFloat())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backing up a bit, wondering what the possible null state means here? I.e. why can we have a no-op BackgroundImageLayer? Wondering how this will also eventually fit into the work @jorge-cab will be taking on more in the future about image support for background-image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If parsing any gradient fails we return null failing silently and do not draw the gradient like web. So this won't return a shader in that case!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of allowing constructing a BackgroundImageLayer which is in an invalid state, could we pull parsing out of the constructor? E.g. so that some static parse function returns null if passed invalid data, but then, if we can correctly parse a BackgroundImageLayer, and returns one, it must be valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added parse function here. i had to make the constructor private because of access modifier issues (can't keep the constructor public since Gradient interface is internal). So BackgroundImageLayer will stay public because of this but it's constructor will stay private.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import androidx.core.graphics.ColorUtils
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 kotlin.math.ln

public data class ColorStop(var color: Int? = null, val position: LengthPercentage? = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep all of these internal for now, to avoid potential breaking changes in the future if we need to change them? Later we could decide what to be public, that libraries should be able to call with a stable API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we should prefer class over data class where we don't need equality or hashing bc they can lead to binary bloat. Some folks have been looking to remove data class usages from RN to save a bit on binary.


public data class ProcessedColorStop(var color: Int? = null, val position: Float? = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we allow each part to be nullable?

Could we document what these types mean, and how they are used?


public object ColorStopUtils {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is pulled out of old code, but now this seems pretty unit testable. Could we maybe add some tests for these? BorderRadiusStyleTest is a test for something in the same structure that could be matched I think.

public fun getFixedColorStops(
colorStops: ArrayList<ColorStop>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is this dependent on type?

Suggested change
colorStops: ArrayList<ColorStop>,
colorStops: List<ColorStop>,

gradientLineLength: Float
): List<ProcessedColorStop> {
val fixedColorStops = Array<ProcessedColorStop>(colorStops.size) { ProcessedColorStop() }
var hasNullPositions = false
var maxPositionSoFar =
resolveColorStopPosition(colorStops[0].position, gradientLineLength) ?: 0f

for (i in colorStops.indices) {
val colorStop = colorStops[i]
var newPosition = resolveColorStopPosition(colorStop.position, gradientLineLength)

// Step 1:
// If the first color stop does not have a position,
// set its position to 0%. If the last color stop does not have a position,
// set its position to 100%.
newPosition =
newPosition
?: when (i) {
0 -> 0f
colorStops.size - 1 -> 1f
else -> null
}

// Step 2:
// If a color stop or transition hint has a position
// that is less than the specified position of any color stop or transition hint
// before it in the list, set its position to be equal to the
// largest specified position of any color stop or transition hint before it.
if (newPosition != null) {
newPosition = maxOf(newPosition, maxPositionSoFar)
fixedColorStops[i] = ProcessedColorStop(colorStop.color, newPosition)
maxPositionSoFar = newPosition
} else {
hasNullPositions = true
}
}

// Step 3:
// If any color stop still does not have a position,
// then, for each run of adjacent color stops without positions,
// set their positions so that they are evenly spaced between the preceding and
// following color stops with positions.
if (hasNullPositions) {
var lastDefinedIndex = 0
for (i in 1 until fixedColorStops.size) {
val endPosition = fixedColorStops[i].position
if (endPosition != null) {
val unpositionedStops = i - lastDefinedIndex - 1
if (unpositionedStops > 0) {
val startPosition = fixedColorStops[lastDefinedIndex].position
if (startPosition != null) {
val increment = (endPosition - startPosition) / (unpositionedStops + 1)
for (j in 1..unpositionedStops) {
fixedColorStops[lastDefinedIndex + j] =
ProcessedColorStop(
colorStops[lastDefinedIndex + j].color, startPosition + increment * j
)
}
}
}
lastDefinedIndex = i
}
}
}

return processColorTransitionHints(fixedColorStops)
}

// Spec: https://drafts.csswg.org/css-images-4/#coloring-gradient-line (Refer transition hint section)
// Browsers add 9 intermediate color stops when a transition hint is present
// Algorithm is referred from Blink engine
// [source](https://github.com/chromium/chromium/blob/a296b1bad6dc1ed9d751b7528f7ca2134227b828/third_party/blink/renderer/core/css/css_gradient_value.cc#L240).
private fun processColorTransitionHints(
originalStops: Array<ProcessedColorStop>
): List<ProcessedColorStop> {
val colorStops = originalStops.toMutableList()
var indexOffset = 0

for (i in 1 until originalStops.size - 1) {
// Skip if not a color hint
if (originalStops[i].color != null) {
continue
}

val x = i + indexOffset
if (x < 1) {
continue
}

val offsetLeft = colorStops[x - 1].position
val offsetRight = colorStops[x + 1].position
val offset = colorStops[x].position
if (offsetLeft == null || offsetRight == null || offset == null) {
continue
}
val leftDist = offset - offsetLeft
val rightDist = offsetRight - offset
val totalDist = offsetRight - offsetLeft
val leftColor = colorStops[x - 1].color
val rightColor = colorStops[x + 1].color

if (FloatUtil.floatsEqual(leftDist, rightDist)) {
colorStops.removeAt(x)
--indexOffset
continue
}

if (FloatUtil.floatsEqual(leftDist, 0f)) {
colorStops[x].color = rightColor
continue
}

if (FloatUtil.floatsEqual(rightDist, 0f)) {
colorStops[x].color = leftColor
continue
}

val newStops = ArrayList<ProcessedColorStop>(9)

// Position the new color stops
if (leftDist > rightDist) {
for (y in 0..6) {
newStops.add(ProcessedColorStop(null, offsetLeft + leftDist * ((7f + y) / 13f)))
}
newStops.add(ProcessedColorStop(null, offset + rightDist * (1f / 3f)))
newStops.add(ProcessedColorStop(null, offset + rightDist * (2f / 3f)))
} else {
newStops.add(ProcessedColorStop(null, offsetLeft + leftDist * (1f / 3f)))
newStops.add(ProcessedColorStop(null, offsetLeft + leftDist * (2f / 3f)))
for (y in 0..6) {
newStops.add(ProcessedColorStop(null, offset + rightDist * (y / 13f)))
}
}

// Calculate colors for the new stops
val hintRelativeOffset = leftDist / totalDist
val logRatio = ln(0.5) / ln(hintRelativeOffset)

for (newStop in newStops) {
if (newStop.position == null) {
continue
}
val pointRelativeOffset = (newStop.position - offsetLeft) / totalDist
val weighting = Math.pow(pointRelativeOffset.toDouble(), logRatio).toFloat()

if (!weighting.isFinite() || weighting.isNaN()) {
continue
}

// Interpolate color using the calculated weighting
leftColor?.let { left ->
rightColor?.let { right -> newStop.color = ColorUtils.blendARGB(left, right, weighting) }
}
}

// Replace the color hint with new color stops
colorStops.removeAt(x)
colorStops.addAll(x, newStops)
indexOffset += 8
}

return colorStops
}

private fun resolveColorStopPosition(
position: LengthPercentage?,
gradientLineLength: Float
): Float? {
if (position == null) return null

return when (position.type) {
LengthPercentageType.POINT ->
PixelUtil.toPixelFromDIP(position.resolve(0f)) / gradientLineLength

LengthPercentageType.PERCENT -> position.resolve(1f)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,44 +7,8 @@

package com.facebook.react.uimanager.style

import android.content.Context
import android.graphics.Rect
import android.graphics.Shader
import com.facebook.react.bridge.ReadableMap

internal class Gradient(gradient: ReadableMap?, context: Context) {
private enum class GradientType {
LINEAR_GRADIENT
}

private val type: GradientType
private val linearGradient: LinearGradient

init {
gradient ?: throw IllegalArgumentException("Gradient cannot be null")

val typeString = gradient.getString("type")
type =
when (typeString) {
"linearGradient" -> GradientType.LINEAR_GRADIENT
else -> throw IllegalArgumentException("Unsupported gradient type: $typeString")
}

val directionMap =
gradient.getMap("direction")
?: throw IllegalArgumentException("Gradient must have direction")

val colorStops =
gradient.getArray("colorStops")
?: throw IllegalArgumentException("Invalid colorStops array")

linearGradient = LinearGradient(directionMap, colorStops, context)
}

fun getShader(bounds: Rect): Shader? {
return when (type) {
GradientType.LINEAR_GRADIENT ->
linearGradient.getShader(bounds.width().toFloat(), bounds.height().toFloat())
}
}
public interface Gradient {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional we moved this from internal to public?

Suggested change
public interface Gradient {
internal interface Gradient {

public fun getShader(width: Float, height: Float): Shader?
}
Loading
Loading