[Insets, Material] Allow usage of insets for Material window components (Scaffold, bottom navigation, app bar) [217776202]

Fixed

Bug

adexe s nau

Status Update

No update yet.

Description

va...@google.com

created issue #1

Feb 3, 2022 11:02PM

Right now, Scaffold, bottom navigation, and app bars for material and material3 don't nicely work with insets.

This is because insets want to apply padding around the content of these items, while still including that padding in the drawing of the surface.

Taking BottomNavigation as the simplest example, the current implementation looks like this:

@Composable
fun BottomNavigation(
    modifier: Modifier = Modifier,
    backgroundColor: Color = MaterialTheme.colors.primarySurface,
    contentColor: Color = contentColorFor(backgroundColor),
    elevation: Dp = BottomNavigationDefaults.Elevation,
    content: @Composable RowScope.() -> Unit
) {
    Surface(
        color = backgroundColor,
        contentColor = contentColor,
        elevation = elevation,
        modifier = modifier
    ) {
        Row(
            Modifier // <-- insets want to apply padding here, inside of the Surface
                .fillMaxWidth()
                .height(BottomNavigationHeight)
                .selectableGroup(),
            horizontalArrangement = Arrangement.SpaceBetween,
            content = content
        )
    }
}

Per discussion, the most straightforward approach forward is to allow developers to independently use the surface and the content, independently. Thus, the proposed new implementation would look like this (also preserving backwards-compatability):

@Composable
fun BottomNavigation(
    modifier: Modifier = Modifier,
    backgroundColor: Color = MaterialTheme.colors.primarySurface,
    contentColor: Color = contentColorFor(backgroundColor),
    elevation: Dp = BottomNavigationDefaults.Elevation,
    content: @Composable RowScope.() -> Unit,
) {
    BottomNavigationSurface(modifier, backgroundColor, contentColor, elevation) {
        BottomNavigationContent {
            content()
        }
    }
}

@Composable
fun BottomNavigationSurface(
    modifier: Modifier = Modifier,
    backgroundColor: Color = MaterialTheme.colors.primarySurface,
    contentColor: Color = contentColorFor(backgroundColor),
    elevation: Dp = BottomNavigationDefaults.Elevation,
    content: @Composable () -> Unit
) {
    Surface(
        color = backgroundColor,
        contentColor = contentColor,
        elevation = elevation,
        modifier = modifier,
    ) {
        content()
    }
}

@Composable
fun BottomNavigationContent(
    modifier: Modifier = Modifier,
    content: @Composable RowScope.() -> Unit,
) {
    Row(
        modifier = modifier
            .fillMaxWidth()
            .height(BottomNavigationHeight)
            .selectableGroup(),
        horizontalArrangement = Arrangement.SpaceBetween,
        content = content,
    )
}

This opens up the ability for insets usage like this:

BottomNavigationSurface(
    backgroundColor = MaterialTheme.colors.surface.copy(alpha = 0.95f),
) {
    BottomNavigationContent(
        modifier = Modifier.navigationBarsPadding()
    ) {
        BottomNavigationItem(/* ... */)
        BottomNavigationItem(/* ... */)
        BottomNavigationItem(/* ... */)
        BottomNavigationItem(/* ... */)
    }
}

The resolution of this issue would allow for the deprecation of accompanist/insets-ui.

Comments

lp...@google.com <lp...@google.com> #2Feb 3, 2022 11:12PM

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

va...@google.com <va...@google.com> #3Feb 3, 2022 11:34PM

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

[Deleted User] <[Deleted User]> #4Mar 5, 2022 12:20AM

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

ap...@google.com <ap...@google.com> #5Apr 5, 2022 06:46PM

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

commit a8a8ac12b599a64ff9895d2e348689f2ad263462
Author: Alex Vanyo <vanyo@google.com>
Date: Fri Feb 18 22:46:07 2022

Apply top bar height as inner padding in material3 Scaffold

Relnote: "Move the top bar height of material3 Scaffold into the padding
passed to content, allowing the content to render underneath the top app
bar. If the PaddingValues are ignored, then the content might be
obscured by the top bar."

Bug: 217776202
Test: Updated tests

Change-Id: I83cbc17a8068a04ea8b18609f12bd9e103630229

M compose/desktop/desktop/samples/src/jvmMain/kotlin/androidx/compose/desktop/examples/example1/Main.jvm.kt
M compose/material3/material3/src/androidAndroidTest/kotlin/androidx/compose/material3/ScaffoldTest.kt
M compose/material3/material3/src/commonMain/kotlin/androidx/compose/material3/Scaffold.kt

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

ma...@google.com <ma...@google.com> #6Oct 3, 2023 10:44AM

Marked as fixed, reassigned to ma...@google.com.

Outdated bug. Support for material3 component's insets has been added in march 2022, support for material og components has been added in 2023 July. Closing as fixed

Issue 217776202