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.

Sign up for GitHub

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

Jump to bottom

Support TextStyle.lineHeightStyle #2602

Closed
AiHMin opened this issue Jan 6, 2023 · 11 comments · Fixed by JetBrains/compose-multiplatform-core#1569
Closed

Support TextStyle.lineHeightStyle #2602

AiHMin opened this issue Jan 6, 2023 · 11 comments · Fixed by JetBrains/compose-multiplatform-core#1569
Assignees
@MatkovIvan
Labels
p:high High priority skia text

Comments

@AiHMin
Copy link

AiHMin commented Jan 6, 2023 •
edited
Loading

I'm trying to set lineHeight and lineHeightStyle to TextStyle but I found that lineHeightStyle doesn't work

minimum reproduced code:

Text(
     "Hello World!",
      style = TextStyle(
          lineHeight = 36.sp,
          lineHeightStyle = LineHeightStyle(
              LineHeightStyle.Alignment.Center, // or any kind of Alignment
              LineHeightStyle.Trim.None
           )
       
)

version properties:

kotlin.version=1.7.20
agp.version=7.3.1
# also test 1.2.2
compose.version=1.3.0-beta04-dev903

Actual:
image

Expect (use html and css😂):
image
@willflier
Copy link

You can try this LineHeightStyle.Trim.Both

@AiHMin
Copy link
Author

AiHMin commented Jan 9, 2023

You can try this LineHeightStyle.Trim.Both

I tried but nothing changed

@m-sasha
Copy link
Member

m-sasha commented Jan 16, 2023

Maybe you can use TextStyle(baselineShift) as a workaround.

@m-sasha
Copy link
Member

m-sasha commented Jan 16, 2023

The problem seems to be that in SkiaParagraph.skiko.kt, the line height style alignment is never added to SkTextStyle.baselineShift. Only BaselineShift.multiplier is taken into account.

@AiHMin
Copy link
Author

AiHMin commented Jan 17, 2023 •
edited
Loading

Maybe you can use TextStyle(baselineShift) as a workaround.

It worked but seems different line height need multiplier to be recalculated.
Hope line height style alignment is added to SkTextStyle.baselineShift soon.

@m-sasha
Copy link
Member

m-sasha commented Jan 17, 2023

@igordmn We would need to decide how to merge BaselineShift and LineHeightStyle if both are specified.

@alexzhirkevich alexzhirkevich mentioned this issue Aug 7, 2023
Closed
@jershell
Copy link

Hello. Any news?

@dima-avdeev-jb
Copy link
Contributor

dima-avdeev-jb commented Oct 27, 2023 •
edited
Loading

@jershell Hello! Sorry, not yet

@MatkovIvan MatkovIvan mentioned this issue Nov 8, 2023
Merged
MatkovIvan added a commit to JetBrains/compose-multiplatform-core that referenced this issue Nov 13, 2023
@MatkovIvan
Add support LineHeightStyle.Trim (#897)
62e994b 
## Proposed Changes

- Set `ParagraphStyle.heightMode` based on `LineHeightStyle.Trim` value
- Align default behavior with Android
- Avoid using `StrutStyle` - it doesn't allow the line height trimming.
Set `height` via `TextStyle` instead
- Cache and post-process `lineMetrics`
- Port tests from an Android source set

## Behaviour change

In case of larger `lineHeight`, both paddings are trimmed by default to
match the Android behaviour.

## Testing

Test: run tests from `DesktopParagraphIntegrationLineHeightStyleTest`

```kt
Row(horizontalArrangement = Arrangement.spacedBy(5.dp)) {
    for (lineHeightStyle in listOf(
        null,
        LineHeightStyle(LineHeightStyle.Alignment.Proportional, LineHeightStyle.Trim.Both),
        LineHeightStyle(LineHeightStyle.Alignment.Proportional, LineHeightStyle.Trim.FirstLineTop),
        LineHeightStyle(LineHeightStyle.Alignment.Proportional, LineHeightStyle.Trim.LastLineBottom),
        LineHeightStyle(LineHeightStyle.Alignment.Proportional, LineHeightStyle.Trim.None),
    )) {
        Text("Line 1\nLine 2",
            modifier = Modifier.background(Color.Gray),
            style = TextStyle(
                fontSize = 18.sp,
                lineHeight = 50.sp,
                lineHeightStyle = lineHeightStyle
            )
        )
    }
}
```

Before | After
--- | ---
<img width="285" alt="Screenshot 2023-11-08 at 13 38 08"
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/2c7fbddc-dac9-408e-ad53-cedc361fe777">
| <img width="285" alt="Screenshot 2023-11-08 at 13 37 45"
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/4fe4d7c9-a414-4338-885f-6701e6daecf1">



## Issues Fixed

Fixes (partially)
JetBrains/compose-multiplatform#2602
Fixes JetBrains/compose-multiplatform#3866
mazunin-v-jb pushed a commit to JetBrains/compose-multiplatform-core that referenced this issue Dec 7, 2023
@MatkovIvan @mazunin-v-jb
Add support LineHeightStyle.Trim (#897)
f683a75 
## Proposed Changes

- Set `ParagraphStyle.heightMode` based on `LineHeightStyle.Trim` value
- Align default behavior with Android
- Avoid using `StrutStyle` - it doesn't allow the line height trimming.
Set `height` via `TextStyle` instead
- Cache and post-process `lineMetrics`
- Port tests from an Android source set

## Behaviour change

In case of larger `lineHeight`, both paddings are trimmed by default to
match the Android behaviour.

## Testing

Test: run tests from `DesktopParagraphIntegrationLineHeightStyleTest`

```kt
Row(horizontalArrangement = Arrangement.spacedBy(5.dp)) {
    for (lineHeightStyle in listOf(
        null,
        LineHeightStyle(LineHeightStyle.Alignment.Proportional, LineHeightStyle.Trim.Both),
        LineHeightStyle(LineHeightStyle.Alignment.Proportional, LineHeightStyle.Trim.FirstLineTop),
        LineHeightStyle(LineHeightStyle.Alignment.Proportional, LineHeightStyle.Trim.LastLineBottom),
        LineHeightStyle(LineHeightStyle.Alignment.Proportional, LineHeightStyle.Trim.None),
    )) {
        Text("Line 1\nLine 2",
            modifier = Modifier.background(Color.Gray),
            style = TextStyle(
                fontSize = 18.sp,
                lineHeight = 50.sp,
                lineHeightStyle = lineHeightStyle
            )
        )
    }
}
```

Before | After
--- | ---
<img width="285" alt="Screenshot 2023-11-08 at 13 38 08"
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/2c7fbddc-dac9-408e-ad53-cedc361fe777">
| <img width="285" alt="Screenshot 2023-11-08 at 13 37 45"
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/4fe4d7c9-a414-4338-885f-6701e6daecf1">



## Issues Fixed

Fixes (partially)
JetBrains/compose-multiplatform#2602
Fixes JetBrains/compose-multiplatform#3866
@PatrickVvB
Copy link

PatrickVvB commented Dec 11, 2023 •
edited
Loading

Try something like this:

TextStyle(
    lineHeight = 36.sp,
    lineHeightStyle = LineHeightStyle(
        alignment = LineHeightStyle.Alignment.Center,
        trim = LineHeightStyle.Trim.None
    ),
    platformStyle = PlatformTextStyle(includeFontPadding = false)
)

https://developer.android.com/jetpack/compose/text/style-paragraph#adjust-line-height

@MatkovIvan MatkovIvan assigned MatkovIvan and unassigned igordmn May 1, 2024
@MatkovIvan MatkovIvan added the text label May 1, 2024
@igordmn igordmn added the p:high High priority label May 1, 2024
@igordmn
Copy link
Collaborator

igordmn commented May 1, 2024 •
edited
Loading

As LineHeightStyle.Alignment.Center became the default of MaterialTheme, text looks different on Android now in case when fontSize != lineHeight. Increasing the priority

@MatkovIvan MatkovIvan mentioned this issue May 17, 2024
Closed
@MatkovIvan MatkovIvan added the skia label Jun 7, 2024
@okushnikov
Copy link
Collaborator

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

@MatkovIvan MatkovIvan mentioned this issue Sep 18, 2024
Merged
@MatkovIvan MatkovIvan changed the title TextStyle lineHeightStyle not working Support TextStyle.lineHeightStyle Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MatkovIvan MatkovIvan

Labels
p:high High priority skia text
Projects
None yet
Milestone
No milestone
9 participants
@jershell @MatkovIvan @m-sasha @igordmn @willflier @AiHMin @PatrickVvB @dima-avdeev-jb @okushnikov