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

Image displayed at placeholder aspect ratio #1505

Open
CurtJRees opened this issue Oct 11, 2022 · 5 comments
Open

Image displayed at placeholder aspect ratio #1505

CurtJRees opened this issue Oct 11, 2022 · 5 comments
Labels
help wanted Issues that are up for grabs + are good candidates for community PRs

Comments

@CurtJRees
Copy link

CurtJRees commented Oct 11, 2022

Describe the bug
It appears that using a combination of Placeholder, Crossfade and ContentScale.FillWidth will cause an image to be displayed at the placeholder aspect ratio and not the final image.

To Reproduce
The test_image drawable in this case is a 16:9 image

@Composable
fun Sample() {
    Column(Modifier.fillMaxSize().verticalScroll(rememberScrollState())) {
        //Correct aspect ratio
        AsyncImage(
            modifier = Modifier.fillMaxWidth(),
            model = ImageRequest.Builder(LocalContext.current)
                .data("https://s3.amazonaws.com/www-inside-design/uploads/2020/10/aspect-ratios-blogpost-1x1-1.png")
                .placeholder(R.drawable.test_image)
                .crossfade(false)
                .build(),
            contentDescription = null,
            contentScale = ContentScale.FillWidth
        )

        Spacer(Modifier.height(16.dp))

        //Incorrect aspect ratio
        AsyncImage(
            modifier = Modifier.fillMaxWidth(),
            model = ImageRequest.Builder(LocalContext.current)
                .data("https://s3.amazonaws.com/www-inside-design/uploads/2020/10/aspect-ratios-blogpost-1x1-1.png")
                .placeholder(R.drawable.test_image)
                .crossfade(true)
                .build(),
            contentDescription = null,
            contentScale = ContentScale.FillWidth
        )
    }
}

Logs/Screenshots
Screenshot_20221011-165830

Version
Reproduced on both 2.0.0 and 2.2.2, using a Pixel 6 Pro on Android 13

@colinrtwhite
Copy link
Member

Which version of Compose are you using?

@colinrtwhite colinrtwhite added the help wanted Issues that are up for grabs + are good candidates for community PRs label Oct 12, 2022
@CurtJRees
Copy link
Author

Which version of Compose are you using?

I've managed to reproduce using Compose 1.1.1, 1.2.0 and 1.3.0-rc01

@digitalheir
Copy link

Ran into the same issue trying to workaround #1568. Coil version 2.2.2. Perhaps the solution could be fixed together?

@digitalheir
Copy link

digitalheir commented Dec 17, 2022

The bug seems specific for ContentScale.FillWidth.

Constraints for my container are: Constraints(minWidth = 0, maxWidth = 828, minHeight = 0, maxHeight = 1260)

In modifyConstraints, this causes hasBoundedSize && (hasFixedWidth || hasFixedHeight) to be false, causing the else block to run:

dstWidth = ... constraints.constrainWidth(intrinsicWidth)
dstHeight = ... constraints.constrainHeight(intrinsicHeight)

But this is unexpected to me. Why should it clamp to the intrinsic width? It should clamp to the max width of the constraints!

Something like this is better. Perhaps a bit messy. I'm not acquainted enough with the source code to determine whether this if-else spaghetti is what you really want.

val dstWidth: Float
val dstHeight: Float
if(contentScale == ContentScale.FillWidth && constraints.hasBoundedWidth && intrinsicWidth.isFinite() && intrinsicHeight.isFinite()) {
    val intrinsicRatio = intrinsicWidth / intrinsicHeight 
    dstWidth = constraints.maxWidth
    dstHeight = constraints.maxWidth / intrinsicRatio 
} else if (hasBoundedSize && (hasFixedWidth || hasFixedHeight)) {
   ... etc ...
} else {
   ... etc ...
}

Insn't it better?

Equivalently, my issue is worked-around by passing a fixed width, because then hasFixedWidth || hasFixedHeight will be true.

@dessalines
Copy link

I can verify that this is still a bug. When using placeholder + ContentScale.FillWidth, the image sizing gets messed up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that are up for grabs + are good candidates for community PRs
Projects
None yet
Development

No branches or pull requests

4 participants