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-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

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

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

commit 1f11909de37abd564714ee82ff123041af4583a8
Author: Max Alfonso-Ying <maxying@google.com>
Date: Thu Dec 28 17:57:06 2023

Fix typo in ListItem end padding

Bug:

b/305342674

Test: updated screenshots
Relnote: "Fixed ListItem end padding value to align with spec."
Change-Id: Ibd68bcdc69da576e47b83fa793c0d626874aa4be

M compose/material3/material3/src/commonMain/kotlin/androidx/compose/material3/ListItem.kt

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

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

Marked as fixed.

Thanks for pointing out the spec discrepancy! This has been fixed.

As for the minimumInteractiveComponentSize() issue, a workaround is to use Modifier.offset on the icon, but we will explore options to make this common use case more intuitive.

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