Issues with end padding in ListItem from material3 [305342674]

Fixed

Bug

Status Update

No update yet.

Description

sp...@gmail.com

created issue #1

Oct 14, 2023 03:18PM

Jetpack Compose version: 1.6.0-alpha07
Jetpack Compose material3 version: 1.2.0-alpha09
Jetpack Compose component used: ListItem
Android Studio Build: Android Studio Iguana | 2023.2.1 Canary 7
Kotlin version: 1.9.10

I was trying to use the ListItem composable from material3 and noticed a couple of issues with trailingContent leaving more space than expected at the end.

First issue

After checking the specs, I believe that ListItemEndPadding is set to the wrong value. It should be 16.dp not 24.dp.

Second issue

If, for example, I try to recreate the "One-line lists" and use a CheckBox in the trailing content lambda, ListItem will add a 16.dp** padding to the end. Unfortunately, a clickable CheckBox uses minimumInteractiveComponentSize(), and the reserved space will increase from 24.dp (CheckBox size) to 48.dp, adding 12.dp to the already existing 16.dp from ListItem.

Right now I think it's not possible to recreate the examples from the specs that have a "Secondary Action area" like a CheckBox, IconButton, or anything small with a minimum touch target size added. Text "100+" examples are different since they don't need to be clickable.

** Assuming First issue is fixed at some point and the end padding applied is 16.dp and not 24.dp.

Steps to reproduce

Run a composable with this content:

// Result with markers in the "output_with_8dp_markers.png" file attached
Column {
    ListItem(
        headlineContent = { Text(text = "Text") },
        trailingContent = { Text(text = "100+") },
    )
    HorizontalDivider()
    ListItem(
        headlineContent = { Text(text = "Checkbox") },
        trailingContent = { Checkbox(checked = true, onCheckedChange = { }) },
    )
    HorizontalDivider()
    ListItem(
        headlineContent = { Text(text = "Checkbox (null onChange)") },
        trailingContent = { Checkbox(checked = true, onCheckedChange = null) },
    )
    HorizontalDivider()
    ListItem(
        headlineContent = { Text(text = "Icon") },
        trailingContent = {
            IconButton(onClick = {}) {
                Icon(Icons.AutoMirrored.Default.Send, contentDescription = "Send this")
            }
        },
    )
}

Instead of 16.dp, we get:

The "Text" example leaves 24.dp between the "100+" and the end.
The "Checkbox" example leaves 36.dp.
The "Checkbox [null onChange]" example (if we don't want to use the Checkbox as a "Secondary Action") leaves 24.dp.
The "Icon" example leaves 36.dp.

Apologies if this is working as expected. All the ListItem examples use Text as the trailingContent and I think an example with CheckBox/IconButton could be very helpful, because I don't see what the correct approach is when using other types of content. Thank you.

output_with_8dp_markers.png

20 KB

View

Download

Comments

sg...@google.com <sg...@google.com> Oct 16, 2023 09:25PM

Reassigned to ma...@google.com.

al...@gmail.com <al...@gmail.com> #2Nov 30, 2023 11:56PM

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

commit a330c0d3bcdd41326f37968a60e6084ad4a2e32c
Author: Chet Haase <chet@google.com>
Date: Wed Jul 05 07:26:46 2023

Convert APIs using PointF to use Float instead

PointF is a convenient mechanism for passing around x.y values
representing 2D points. But there are downsides, including:
- Converting to PointF: You may not have the data in PointF form
to begin with, so using an API which takes PointF requires converting
the data to that form (including allocating a PointF object every time)
- Mutability: Point structures can be mutated internally, causing
unpredictability in what that mutation means. Should the library
react to those changes? Ignore them? Do defensive copies (requiring
even more allocations)? Using primitive types like Float make the
behavior more obvious (by making the data inherently immutable).
- Allocations: Whenever we use object types, there are necessarily
allocations on the Java heap for them. This puts pressure on the GC
at both allocation and collection time. Given the amount of points
being passed around (especially at morph creation time, when curves
are being split and created), this causes a lot of PointF objects to
be allocated (even temporarily). Using Float avoids that problem.

Also fixed bug with unclosed paths causing discontinuity at the
start/end point.

Bug: 276466399
Bug: 290254314
Test: integration and unit tests pass
Relnote: PointF parameters changed to Float pairs
Change-Id: Id4705d27c7be31b26ade8186b99fffe2e2f8450e

M graphics/graphics-shapes/api/current.txt
M graphics/graphics-shapes/api/restricted_current.txt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicShapeTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonMeasureTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/RoundedPolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/ShapesTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/TestUtils.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Cubic.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/CubicShape.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FeatureMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FloatMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Morph.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/PolygonMeasure.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/RoundedPolygon.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Shapes.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Utils.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/DebugDraw.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/ShapeEditor.kt
M graphics/integration-tests/testapp/src/main/java/androidx/graphics/shapes/test/MaterialShapes.kt

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

ap...@google.com <ap...@google.com> #3Jan 2, 2024 05:58PM

PointF was the main (possibly only) mutability issue, marking this as fixed

ma...@google.com <ma...@google.com> #4Jan 2, 2024 06:00PM

Marked as fixed.

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.graphics:graphics-shapes:1.0.0-alpha04

pr...@google.com <pr...@google.com> #5Jan 16, 2024 06:24PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.compose.material3:material3:1.2.0-beta02
androidx.compose.material3:material3-android:1.2.0-beta02
androidx.compose.material3:material3-desktop:1.2.0-beta02

od...@gmail.com <od...@gmail.com> #6Feb 5, 2024 04:11PM

Can we ask why this was closed? We are finding that the trailing padding has issues even after the applied fix and the workaround posted here. Note the following reproducible code and screenshots.

@Preview(widthDp = 403)
@Composable
private fun ListItemTrailingPaddingIssue() {
    ListItem(
        overlineContent = {
            Text("Overline content")
        },
        headlineContent = {
            Text("Headline content reaches to trailing content")
        },
        supportingContent = {
            Text("Supporting content")
        },
        trailingContent = {
            IconButton(
                // We've applied the modifier "workaround". However, note the large padding left between the leading
                // and trailing content because the offset doesn't change or move the actual padding of
                // the trailing content
                // https://issuetracker.google.com/issues/305342674#comment4
                modifier = Modifier.offset(
                    y = (-12).dp,
                    x = 12.dp,
                ),
                onClick = {},
            ) {
                Icon(
                    imageVector = Icons.Default.Delete,
                    contentDescription = "Delete button",
                )
            }
        },
    )
}

@Preview(widthDp = 403)
@Composable
private fun ListItemWithoutTrailingPaddingIssue() {
    ListItem(
        overlineContent = {
            Text("Overline content")
        },
        headlineContent = {
            Text("Headline content reaches to trailing content")
        },
        supportingContent = {
            Text("Supporting content")
        },
        trailingContent = {
            Icon(
                imageVector = Icons.Default.Delete,
                contentDescription = "Delete button",
            )
        },
    )
}

TrailingPaddingIssue.png

1002 KB