JetBrains / compose-multiplatform

Compose Multiplatform, a modern UI framework for Kotlin that makes building performant and beautiful user interfaces easy and enjoyable.
https://jetbrains.com/lp/compose-multiplatform
Apache License 2.0
15.88k stars 1.15k forks source link

SVGPainter returns the wrong default size when percentage units are used in the SVG file #3732

Open alexfacciorusso opened 11 months ago

alexfacciorusso commented 11 months ago

Describe the bug

The androidx.compose.ui.res.SVGPainter class returns a defaultSize (and in turn, sets the intrinsic sizes in the Image composable) even when a size in concrete units is not specified in the SVG file.

Affected platforms

Desktop (reproduced), and potentially any platform using the SVGPainter class.

Versions

To Reproduce

Create a painter loaded from an SVG resource with width and height set to 100% (<svg ... width="100%" height="100%">:

val painterResource = painterResource("svg_logo.svg")

Where svg_logo.svg is this one (By W3C SVG Logo, CC BY-SA 4.0)

Inspect the painterResource's intrinsicSize property. It will be Size(100.0, 100.0).

Expected behavior

Assigning 100 pixels to be the intrinsic size of an SVG that sets a percentage as its size is not sensible in that context.

Possibly, it should not be set at all, or perhaps the viewBox boundaries in the SVG should be taken into consideration.

Screenshots

image

Additional context

By debugging and digging in the source code, the problem happens in the DesktopSvgResources.desktop.kt file, where this code calculates the default size in px of the painter:

private val defaultSizePx: Size = run {
    val width = root?.width?.withUnit(SVGLengthUnit.PX)?.value ?: 0f // <-- here
    val height = root?.height?.withUnit(SVGLengthUnit.PX)?.value ?: 0f // <-- and here
    if (width == 0f && height == 0f) {
        Size.Unspecified
    } else {
        Size(width, height)
    }
}

Maybe a check can be put into place to ignore that size if the original unit is PERCENTAGE.

eymar commented 11 months ago

withUnit doesn't really do any conversion of the value, it just creates a new SVGLength with the same value. So, as I understand this code completely ignores the units from an .svg file, which is likely a bug (I'll check with the team).


I think the easiest way for us is to treat it as Size.Unspecified. What is your expectation for image behaviour when its size unit is PERCENTAGE?

cc: @igordmn Maybe we can convert other units to pixels. But not sure for now how to correctly handle PERCENTAGE unit.

igordmn commented 11 months ago

I think the easiest way for us is to treat it as Size.Unspecified. What is your expectation for image behaviour when its size unit is PERCENTAGE?](cc: @igordmn Maybe we can convert other units to pixels. But not sure for now how to correctly handle PERCENTAGE unit.)

I would treat percentage as unspecified, and convert other units to px (that was the idea, but it seems we have a bug)

okushnikov commented 1 month ago

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.