[NavigationSuiteScaffold] Intended use of containerColor and contentColor unclear [331993720]

Fixed

Customer Issue

Status Update

No update yet.

Description

ia...@google.com

created issue #1

Mar 30, 2024 01:09AM

NavigationSuiteScaffold draws a Surface using

Surface(modifier = modifier, color = containerColor, contentColor = contentColor) { ... }

This covers the whole scaffold, including the content lambda. It's a bit unclear what the intended usage is. For example, contentColor will override the color of text in the content lambda but not the nav items (since they have their own colors set separately). It seems a bit odd to have the scaffold affect the content in that way, but I'm not sure if there was a specific intent behind this.

The default Compose templates uses this layout:

setContent {
    AppTheme {
        Surface() {
            // app stuff
        }
    }
}

So maybe the intent is to replace the default surface with the NavigationSuiteScaffold?

Comments

ia...@google.com <ia...@google.com> #2Apr 1, 2024 11:23PM

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

le...@google.com <le...@google.com> #3Apr 2, 2024 06:06PM

This is the current kdoc of the different colors params of the NSS:

@param navigationSuiteColors [NavigationSuiteColors] that will be used to determine the container
(background) color of the navigation component and the preferred color for content inside the
navigation component

@param containerColor the color used for the background of the navigation suite scaffold. Use
[Color.Transparent] to have no color

@param contentColor the preferred color for content inside the navigation suite scaffold.
Defaults to either the matching content color for [containerColor], or to the current
[LocalContentColor] if [containerColor] is not a color from the theme

so do you want to add something to the contentColor param part? It already mentions it affects the content. Could you help me clarify what is needed?

ia...@google.com <ia...@google.com> #4Apr 4, 2024 11:51PM

I think it's both containerColor and contentColor that aren't fully clear, but part of that is just that the main content is technically inside of the NSS but might not be in the developer or designer's mental model (especially because of the trailing lambda syntax.

Maybe something like:

@param containerColor the color used for the background of the navigation suite scaffold, including the passed content composable. Use [Color.Transparent] to have no color

The contentColor is harder because 'content' in that context means the Material3 concept (sort of, the 'ink' that's used for UI on the Surface) but it basically just applies to the content lambda since the navigation UI components have theirs set separately. AFAICT, this isn't talked about on the mio site and M3 doesn't seem to have a page for Surface like M2 did (best I could find was "Surface – A role used for backgrounds and large, low-emphasis areas of the screen."), so I'm not sure what the best language is.

Here's what I've got so far:

@param contentColor the preferred color to be used for typography and iconography within the content lambda. This defaults to the 'on' color of the [containerColor] or to the current [LocalContentColor] if [containerColor] is not a color from the theme

le...@google.com <le...@google.com> #5Apr 10, 2024 05:22PM

Marked as fixed.

Thank you! resolved in https://android-review.git.corp.google.com/c/platform/frameworks/support/+/3034074

na...@google.com <na...@google.com> #6Apr 17, 2024 05:17PM

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

androidx.compose.material3:material3-adaptive-navigation-suite:1.0.0-alpha06
androidx.compose.material3:material3-adaptive-navigation-suite-android:1.0.0-alpha06
androidx.compose.material3:material3-adaptive-navigation-suite-desktop:1.0.0-alpha06

Issue 331993720