TextField's cursor is small with empty value and includeFontPadding=false [236615813]

Fixed

Bug

Status Update

No update yet.

Description

sk...@gmail.com

created issue #1

Jun 21, 2022 10:12AM

Jetpack Compose version: 1.2.0-rc01
Jetpack Compose component used: BasicTextField, Font & FontFamily
Android Studio Build: Chipmunk 2021.2.1 Patch 1 (#AI-212.5712.43.2112.8609683)
Kotlin version: 1.6.21
Devices/Android versions reproduced on: Pixel 3
Keyboard: Gboard

Steps to reproduce:

Use non default font like Circular Std (download here: https://befonts.com/circular-std-font.html)
Use includeFontPadding=false & lineHeightStyle=center, none
Use BasicTextField with empty value ("")

var value by remember { mutableStateOf("") }
BasicTextField(
    modifier = Modifier.border(1.dp, Color.Red).padding(4.dp),
    value = value,
    onValueChange = { value = it },
    textStyle = TextStyle(
        fontFamily = FontFamily(
            Font(R.font.circular_std_medium_500, FontWeight.Normal),
        ),
        fontSize = 16.sp,
        lineHeight = 20.sp,
        lineHeightStyle = LineHeightStyle(
            LineHeightStyle.Alignment.Center,
            LineHeightStyle.Trim.None
        ),
        platformStyle = PlatformTextStyle(includeFontPadding = false),
    ),
)

Current result:

cursor is smaller than the one after entering some text, see screenshot

Expected result:

cursor is the same size all the time

textfield-empty.png

619 B

View

Download

textfield-filled.png

2 KB

View

Download

Comments

si...@google.com <si...@google.com> Jul 6, 2022 06:57PM

Reassigned to si...@google.com.

si...@google.com <si...@google.com> #2Jul 27, 2022 11:41PM

removed the dup; this is possibly about how android spans work on empty strings.

the calculation in the composable level seemed correct but the layout was not
it happens for a new line as well
one problem with both includeFontPadding true/false is that the cursor height is possibly can be visually improved to match the text ascent/descent rather than line top/bottom (might be challenging)

si...@google.com <si...@google.com> Jul 29, 2022 07:23PM

Accepted by si...@google.com.

sk...@gmail.com <sk...@gmail.com> #3Jul 29, 2022 08:51PM

I do not fully understand your points, though I was wondering if a similar "trick" to measure "HHHHHHHHHHH" cannot be used here, because if I'm not mistaken the cursor gets drawn by text layout measuring on empty string.

si...@google.com <si...@google.com> #4Jul 29, 2022 09:09PM

StaticLayout does not call LineHeightSpan's for empty sstring (or the empty final line at the end of the string - a string that ends with "\n")

ap...@google.com <ap...@google.com> #5Aug 2, 2022 04:13PM

Project: platform/frameworks/support
Branch: androidx-main

commit b7a67dc25a34b254adbaffd02495b5c6d26ee18c
Author: siyamed <siyamed@google.com>
Date: Thu Jul 28 08:06:35 2022

Fix TextField empty line lineHeight (2)

When a line is empty (including empty text) StaticLayout
does not apply the LineHeightSpans.

This causes the line height to be different for an empty
line compared to a non empty line. The issue becomes more
visible within TextField .

This CL stores adjustment values to report the height and the
last line metrics correctly.

For the other solution option please see aosp/2170504

Performance

There are no performance differences for cases where text
is not empty or does not end with "\n"

The below numbers are for the cases where text is empty or ends with "\n"

+--------+-----------+-----------+------+
| | Before | After | Diff |
+--------+-----------+-----------+------+
| length | ns | ns | ns |
| 0 | 115,835 | 180,161 | 56% |
| 16 | 395,679 | 484,748 | 23% |
| 32 | 513,235 | 590,783 | 15% |
| 64 | 807,133 | 901,896 | 12% |
| 128 | 1,330,733 | 1,421,371 | 7% |
| 256 | 2,338,534 | 2,464,810 | 5% |
+--------+-----------+-----------+------+

Test: Added test
Test: ./gradlew text:text:test
Test: ./gradlew text:text:cAT
Test: ./gradlew compose:ui:ui-text:test
Test: ./gradlew compose:ui:ui-text:cAT
Test: Treehugger

Bug: 236615813

Change-Id: I57f8b6632f569ad06e547cf6ec83df1bc42845c9

M compose/ui/ui-text/lint-baseline.xml
M compose/ui/ui-text/benchmark/src/androidTest/java/androidx/compose/ui/text/benchmark/input/EditProcessorBenchmark.kt
M compose/ui/ui-text/benchmark/src/androidTest/java/androidx/compose/ui/text/benchmark/ParagraphBenchmark.kt
M compose/ui/ui-text/benchmark/src/main/java/androidx/compose/ui/text/benchmark/TextBenchmarkHelper.kt
M compose/ui/ui-text/src/androidAndroidTest/kotlin/androidx/compose/ui/text/ParagraphIntegrationLineHeightStyleTest.kt
M text/text/src/main/java/androidx/compose/ui/text/android/style/LineHeightStyleSpan.kt
A compose/ui/ui-text/benchmark/src/androidTest/java/androidx/compose/ui/text/benchmark/ParagraphWithLineHeightBenchmark.kt
M text/text/src/main/java/androidx/compose/ui/text/android/TextLayout.kt
M compose/ui/ui-text/benchmark/src/androidTest/java/androidx/compose/ui/text/benchmark/TextMeasurerBenchmark.kt
M compose/ui/ui-text/src/androidMain/kotlin/androidx/compose/ui/text/platform/extensions/SpannableExtensions.android.kt

https://android-review.googlesource.com/2168642

si...@google.com <si...@google.com> Aug 2, 2022 04:15PM

Marked as fixed.

sk...@gmail.com <sk...@gmail.com> #6Aug 2, 2022 05:13PM

Thank you for fixing this. Will be this cherry-picked to 1.2.x release?

si...@google.com <si...@google.com> #7Aug 2, 2022 09:52PM

I think this will be in a 1.3 alpha/beta release (https://developer.android.com/jetpack/androidx/releases/compose-ui#1.3.0-alpha02)

unfortunately we cant cherry pick it into an older release.