Status Update
Comments
lp...@google.com <lp...@google.com> #2
Thanks for filing. Agreed that this is an issue, I'm not convinced that adding more overloads / top level API here is a great solution, it will just double our API surface and make the developer experience when browsing docs / autocomplete etc a lot worse, since you will see a lot of similarly named things.
I think some other options we should consider are:
-
Exposing content padding / a similarly named thing for navigational / all components, to allow for easier integration.
-
Adding built in support to our components, such that top app bar could know about status bar padding, and do it automatically - with either a direct parameter / composition local / modifier local for configuring whether this behaviour should be enabled or not.
For the latter, assume top app bar, I see two main cases:
- You want the app bar to draw under the status bar, so you use status bar padding
- Your app isn't drawing under the status bar, so you have an explicit status bar color and you don't want to use status bar padding
Are there any other cases here we should think about? Are there other types of padding you might want to use for the content, except for the status bar? In general it seems reasonable to me that our components would care about this out of the box, but at the same time I am worried that if we only support one type, then we still will have the problem in the future where there is no escape hatch.
va...@google.com <va...@google.com> #3
Exposing content padding
One issue about exposing this in a non-Modifier way is that the insets implementation relies on ModifierLocal
s to propagate consumption, so it's a lot more awkward to participate in that chain with a separate contentPadding
value. Additionally, in the case where these values are animating, there might be performance considerations if the contentPadding
has to be determined at composition time instead of measure/layout time.
Are there any other cases here we should think about?
I think those are the only cases for the TopAppBar that I can think of, and if a developer wants something very different they can probably roll their own with a Surface
+ content.
I think the Scaffold
change would be minor enough to consider regardless:
Right now, the Scaffold
assumes that the top app bar is opaque, and it places the content below the topAppBarHeight
with contentPadding.top = 0
. It should be non-breaking change (I hope) to instead place the content at 0, 0
and pass down contentPadding.top = topAppBarHeight
.
ap...@google.com <ap...@google.com> #5
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
ma...@google.com <ma...@google.com> #6
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
Description
Right now,
Scaffold
, bottom navigation, and app bars formaterial
andmaterial3
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: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):
This opens up the ability for insets usage like this:
The resolution of this issue would allow for the deprecation of accompanist/insets-ui.