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
16.32k stars 1.18k forks source link

Add support for includeFontPadding #2477

Closed litrik closed 1 year ago

litrik commented 2 years ago

According to this blog post, support to configure includeFontPadding was added to PlatformTextStyle in Compose 1.2.0 on Android.

This is currently not supported in Compose Desktop. Are there any plans to support this? Or is there any other way to get rid of the padding included in fonts?

eymar commented 2 years ago

https://android-review.googlesource.com/c/platform/frameworks/support/+/2028663/27/compose/ui/ui-text/src/androidMain/kotlin/androidx/compose/ui/text/AndroidTextStyle.android.kt#116 - includeFontPadding was added on Android for compatibility reasons and it will be removed.

Have you encountered a use case where includeFontPadding would help on desktop? (A use case would help to understand the issue better) I think we will not add includeFontPadding config on desktop, since it's going to be removed for android anyway. Instead we would do the same fixes which were done for android:

To fix all tall fonts clipping issues that could happen from turning includeFontPadding off in Compose, we apply additional padding when required for only the first and last line, and the max line height is calculated based on the font(s) that is used for all the text included in a given line (instead of the FontMetrics of the first font in the font fallback).

litrik commented 2 years ago

You are right that includeFontPadding is going to be removed in the future on Android but, according to the blog post I have mentioned in the original description, the default value will be changed from true to false, This will effectively result in font padding to be ignored, making it easier to align text.

The use case I'm working on involves the auto-sizing of text to fit a specific width.

This is the composable I have written for this:

@Composable
fun AutoFitTextHorizontally(
    text: String,
    style: TextStyle,
    modifier: Modifier = Modifier,
) {
    BoxWithConstraints(modifier = modifier.fillMaxWidth()) {
        var shrunkFontSize = 200.sp
        val calculateIntrinsics = @Composable {
            ParagraphIntrinsics(
                text = text,
                style = style.copy(fontSize = shrunkFontSize),
                density = LocalDensity.current,
                fontFamilyResolver = LocalFontFamilyResolver.current
            )
        }

        var intrinsics = calculateIntrinsics()
        with(LocalDensity.current) {
            while (intrinsics.maxIntrinsicWidth > maxWidth.toPx()) {
                shrunkFontSize *= 0.9f
                intrinsics = calculateIntrinsics()
            }
        }

        BasicText(
            text = text,
            modifier = Modifier.fillMaxWidth().background(Color.Gray),
            style = style.copy(fontSize = shrunkFontSize),
            maxLines = 1,
        )
    }
}

The fonts include padding so the output looks like this:

image

The 4 w's come very close to the edge, but there is plenty of room for additional i's if font padding would be ignored. Ideally all text would touch the left and right sides of their gray background.

eymar commented 1 year ago

Thank you @litrik. We usually aim to align compose-jb with Jetpack Compose behaviour. So we'll look at when we can implement this.

dimsuz commented 1 year ago

Here's another usecase. I want to perfectly center a "+" symbol vertically in a small Box. Here I have a blue Box and a Text inside it, which uses the Jetbrains Mono font.

image As you can see glyph is not centered vertically. That's because it has extra padding on top and on bottom and top > bottom. Here's the Text with its background highlighted: image

It also occupies unnecessary vertical space in layout. Ideally I want to have exactly bounding box of + char here and nothing more, just like it has in horizontal axis.

So with the current API it's impossible for me to achieve this (it seems).

francisconoriega commented 1 year ago

Since includeFontPadding is basically a workaround for a very old decision/bug in android's viewsystem, I don't think that adding is necessary, but I would just prefer if jetpack for Desktop just plain didn't add any padding by default at all.

Compose for desktop is pretty new still, so I don't think there would be lots of people having issues if the font padding is removed, and the fix would be easy, to manually set your own lineheight to your textstyles.

I guess includeFontPadding could be included there anyways as an opt out, for people who want the legacy behavior

GaborPeto commented 1 year ago

I came across the same issue where my font has extra padding on the top and bottom and I would like to get rid of it but not able to do so as I cannot use includeFontPadding in Compose Desktop.

m-sasha commented 1 year ago

From https://developer.android.com/jetpack/androidx/releases/compose-ui#1.5.0-beta01:

PlatformTextStyle.includeFontPadding is undeprecated. Our original intent was to remove the field, however the feedback shows that developers need this configuration option. Therefore removing deprecation from the field

MitjaHenner commented 1 year ago

Would be great, I'm trying to have a Text fill the full height of a Box and more than a third of the Text is padding.

mgroth0 commented 1 year ago

I am trying to make a button with a tight style and this blocked me.

syt0r commented 1 year ago

I too have an issues with centering text inside without this property

mgroth0 commented 1 year ago

There is a StackOverflow question Drawing text in Jetpack Compose with precision in which the top answer says to set includeFontPadding = false. Due to this issue, that answer is not helpful for Compose for Desktop.

m-sasha commented 1 year ago

Compose for desktop doesn't add font padding, so it effectively always has includeFontPadding=false

mgroth0 commented 1 year ago

@m-sasha, say I have a Cavas in Compose for Desktop, and I use drawRect with a sepcific size. Next, I want to drawText to draw a single character (say, 'X') so that it is exactly the same height as the rectangle such that the 4 points of the X are touching the top and bottom of the rectangle. Is it possible, and if so how?

m-sasha commented 1 year ago

I don't think you can without a special purpose font, but you won't be able to in Android with includeFontPadding=false either.

mgroth0 commented 1 year ago

So to clarify then, this issue is about adding support for a feature that would allow enabling font padding, which at present is not possible to include?

m-sasha commented 1 year ago

includeFontPadding=true is the default in Jetpack Compose up to and including version 1.5, and the request from developers is usually to allow setting it to false. Compose for Desktop, however, has never implemented this feature (so it's effectively false), and because the default will change to includeFontPadding=false in Jetpack Compose 1.6, we've decided to leave it unimplemented in Compose for Desktop.

MatkovIvan commented 1 year ago

Thanks for everyone's input and participation in this issue.

This has been a feature request for the includeFontPadding functionality in Compose Multiplatform. The necessity for this property arises primarily from a unique behavior of Android platform where system level text layouter introduces extra padding by default. To manage this, the includeFontPadding flag was introduced in Jetpack Compose for Android. It's interesting to note that this flag was always a compatibility API and had varied in its presence across versions: first introduced and deprecated in 1.2.0, undeprecated in 1.5.0, and finally, set to false by default in the upcoming 1.6.0 release.

However, Compose Multiplatform does not add this extra padding in the first place and hence there isn't a need to introduce a flag to suppress something that isn't added.

Concerning centering text in Compose Multiplatform, the primary approach is to use the wrapContentHeight modifier like so:

Text(
    "Your text here",
    modifier = Modifier
        .fillMaxSize()
        .wrapContentHeight(
            align = Alignment.CenterVertically, // aligns to the center vertically (default value)
            unbounded = true // Makes sense if the container size less than text's height
        )
)

Note: The simple combination of Modifier.fillMaxSize() along with textAlign = TextAlign.Center will not center align the text vertically. The wrapContentHeight modifier should be used to achieve that.

An alternative method for centering can be using the Box composable:

Box(contentAlignment = Alignment.Center) {
    Text("Your text here")
}

Also, we are working on further improvements for text centering in Compose Multiplatform - the upcoming 1.6.0 version will include additional fixes related to text centering such as support of LineHeightStyle.Trim parameter and aligning default behavior of it with Android.

It's also crucial to note that the positioning of a specific glyph is controlled by the font being used, and the default fonts may vary across different systems. This factor can result in slight differences in the exact visual appearance of a character.

Given all the points above, we are closing this issue as "Not Planned". Please don't hesitate to open new issues if you encounter other challenges or have suggestions for improvements. We appreciate your contributions, thank you for your understanding and continued support.

okushnikov commented 2 months ago

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