From a9d50174f11b62ce74bbd43c554d3dd45f5ef2da Mon Sep 17 00:00:00 2001 From: Jan Skrasek Date: Mon, 13 Feb 2023 19:07:31 +0100 Subject: [PATCH] rework Scaffold to use subcompose & content-padding only for action --- .../orbit/compose/ui/controls/Scaffold.kt | 116 +++++++++--------- .../orbit/compose/ui/controls/TextField.kt | 70 +---------- .../orbit/compose/ui/controls/ScaffoldTest.kt | 82 +++++++++++++ 3 files changed, 143 insertions(+), 125 deletions(-) create mode 100644 ui/src/test/kotlin/kiwi/orbit/compose/ui/controls/ScaffoldTest.kt diff --git a/ui/src/main/java/kiwi/orbit/compose/ui/controls/Scaffold.kt b/ui/src/main/java/kiwi/orbit/compose/ui/controls/Scaffold.kt index f8c45ddee..38ec0251e 100644 --- a/ui/src/main/java/kiwi/orbit/compose/ui/controls/Scaffold.kt +++ b/ui/src/main/java/kiwi/orbit/compose/ui/controls/Scaffold.kt @@ -1,38 +1,34 @@ package kiwi.orbit.compose.ui.controls import androidx.compose.foundation.background -import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.PaddingValues import androidx.compose.foundation.layout.WindowInsets +import androidx.compose.foundation.layout.asPaddingValues +import androidx.compose.foundation.layout.calculateEndPadding +import androidx.compose.foundation.layout.calculateStartPadding import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.ime import androidx.compose.foundation.layout.navigationBars +import androidx.compose.foundation.layout.systemBars import androidx.compose.foundation.layout.union import androidx.compose.foundation.layout.windowInsetsPadding import androidx.compose.runtime.Composable -import androidx.compose.runtime.CompositionLocalProvider -import androidx.compose.runtime.ProvidableCompositionLocal import androidx.compose.runtime.remember -import androidx.compose.runtime.staticCompositionLocalOf import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.Brush import androidx.compose.ui.graphics.Color import androidx.compose.ui.layout.Layout +import androidx.compose.ui.layout.SubcomposeLayout import androidx.compose.ui.platform.LocalDensity -import androidx.compose.ui.tooling.preview.Preview -import androidx.compose.ui.unit.LayoutDirection import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.offset import kiwi.orbit.compose.ui.OrbitTheme import kiwi.orbit.compose.ui.controls.internal.CustomPlaceholder -import kiwi.orbit.compose.ui.controls.internal.MutablePaddingValues +import kiwi.orbit.compose.ui.controls.internal.OrbitPreviews import kiwi.orbit.compose.ui.controls.internal.Preview import kiwi.orbit.compose.ui.foundation.contentColorFor -public val LocalScaffoldPadding: ProvidableCompositionLocal = - staticCompositionLocalOf { PaddingValues(0.dp) } - @Composable public fun Scaffold( modifier: Modifier = Modifier, @@ -43,6 +39,7 @@ public fun Scaffold( actionLayout: @Composable () -> Unit = { ScaffoldAction(backgroundColor, action) }, toastHostState: ToastHostState = remember { ToastHostState() }, toastHost: @Composable (ToastHostState) -> Unit = { ToastHost(it) }, + contentWindowInsets: WindowInsets = WindowInsets.systemBars, content: @Composable (contentPadding: PaddingValues) -> Unit, ) { Surface( @@ -55,6 +52,7 @@ public fun Scaffold( toast = { toastHost(toastHostState) }, action = actionLayout, content = content, + contentWindowInsets = contentWindowInsets, ) } } @@ -65,57 +63,48 @@ private fun ScaffoldLayout( toast: @Composable () -> Unit, action: @Composable () -> Unit, content: @Composable (contentPadding: PaddingValues) -> Unit, + contentWindowInsets: WindowInsets, ) { - val density = LocalDensity.current - val contentPadding = remember { MutablePaddingValues() } - val insets = WindowInsets.ime.union(WindowInsets.navigationBars) - - Layout( - content = { - Box { topBar() } - Box { toast() } - Box { action() } - Box { - CompositionLocalProvider( - LocalScaffoldPadding provides contentPadding, - ) { - content(contentPadding) - } - } - }, - ) { measurables, constraints -> + SubcomposeLayout { constraints -> val layoutWidth = constraints.maxWidth val layoutHeight = constraints.maxHeight val looseConstraints = constraints.copy(minWidth = 0, minHeight = 0) - val topBarPlaceable = measurables[0].measure(looseConstraints) - - val topBarHeight = topBarPlaceable.height - val mainConstraints = looseConstraints.copy(maxHeight = layoutHeight - topBarHeight) - val toastPlaceables = measurables[1].measure(mainConstraints) - - val actionPlaceable = measurables[2].measure(mainConstraints) - val actionHeight = actionPlaceable.height - - val contentPlaceable = measurables[3].measure(looseConstraints) + val topBarPlaceables = subcompose(SlotTopAppBar, topBar).map { it.measure(looseConstraints) } + val actionPlaceables = subcompose(SlotAction, action).map { it.measure(looseConstraints) } + val toastPlaceables = subcompose(SlotToast, toast).map { it.measure(looseConstraints) } - val topInset = topBarHeight.takeUnless { it == 0 } ?: insets.getTop(density) - val startInset = insets.getLeft(density, LayoutDirection.Ltr) - val endInset = insets.getRight(density, LayoutDirection.Ltr) - val bottomInset = actionHeight.takeUnless { it == 0 } ?: insets.getBottom(density) + val actionHeight = actionPlaceables.maxOfOrNull { it.height } ?: 0 + val contentTop = topBarPlaceables.maxOfOrNull { it.height } ?: 0 + val contentBottom = (actionHeight - ActionGradientHeight.roundToPx()).coerceAtLeast(0) + val contentConstraints = looseConstraints.copy( + maxHeight = layoutHeight - contentTop - contentBottom, + ) - contentPadding.updateFrom( - top = topInset.toDp(), - start = startInset.toDp(), - end = endInset.toDp(), - bottom = bottomInset.toDp(), + val insets = contentWindowInsets.asPaddingValues(this) + val innerPadding = PaddingValues( + top = if (topBarPlaceables.isEmpty()) { + insets.calculateTopPadding() + } else { + 0.dp + }, + bottom = if (actionPlaceables.isEmpty()) { + insets.calculateBottomPadding() + } else { + ActionGradientHeight + }, + start = insets.calculateStartPadding(layoutDirection), + end = insets.calculateEndPadding(layoutDirection), ) + val contentPlaceables = subcompose(SlotContent) { + content(innerPadding) + }.map { it.measure(contentConstraints) } layout(layoutWidth, layoutHeight) { - contentPlaceable.place(0, 0) - topBarPlaceable.place(0, 0) - actionPlaceable.place(0, layoutHeight - bottomInset) - toastPlaceables.place((layoutWidth - toastPlaceables.measuredWidth) / 2, topBarHeight) + contentPlaceables.forEach { it.placeRelative(0, contentTop) } + topBarPlaceables.forEach { it.placeRelative(0, 0) } + actionPlaceables.forEach { it.placeRelative(0, layoutHeight - actionHeight) } + toastPlaceables.forEach { it.placeRelative(0, contentTop) } } } } @@ -133,7 +122,7 @@ private fun ScaffoldAction( val brush = remember(density, backgroundColor) { Brush.verticalGradient( colors = listOf(Color.Transparent, backgroundColor), - endY = with(density) { 16.dp.toPx() }, + endY = with(density) { ActionGradientHeight.toPx() }, ) } Layout( @@ -142,20 +131,35 @@ private fun ScaffoldAction( .background(brush) .windowInsetsPadding(WindowInsets.ime.union(WindowInsets.navigationBars)), ) { measurables, constraints -> - val action = measurables.firstOrNull() ?: return@Layout layout(0, 0) {} + val action = measurables.firstOrNull() + ?: return@Layout layout(0, 0) {} + val top = ActionGradientHeight.roundToPx() val padding = 16.dp.roundToPx() val placeable = action.measure(constraints.offset(horizontal = -2 * padding)) val width = constraints.maxWidth - val height = placeable.height + 2 * padding + val height = top + placeable.height + padding layout(width, height) { - placeable.place(x = (width - placeable.width) / 2, y = padding) + placeable.place(x = (width - placeable.width) / 2, y = top) } } } -@Preview +private val SlotTopAppBar = 0 +private val SlotAction = 1 +private val SlotToast = 2 +private val SlotContent = 3 + +/** + * Action's top gradient currently decreased from 16.dp to minimize contentPadding + * (auto)scrolling issues. We need a new api for scroll to be able to account for + * this semi-transparent area. + * https://issuetracker.google.com/issues/221252680 + */ +private val ActionGradientHeight = 8.dp + +@OrbitPreviews @Composable internal fun ScaffoldPreview() { Preview { diff --git a/ui/src/main/java/kiwi/orbit/compose/ui/controls/TextField.kt b/ui/src/main/java/kiwi/orbit/compose/ui/controls/TextField.kt index c3fcbcc78..fec4f56a2 100644 --- a/ui/src/main/java/kiwi/orbit/compose/ui/controls/TextField.kt +++ b/ui/src/main/java/kiwi/orbit/compose/ui/controls/TextField.kt @@ -3,39 +3,28 @@ package kiwi.orbit.compose.ui.controls import androidx.compose.animation.animateColor import androidx.compose.animation.core.tween import androidx.compose.animation.core.updateTransition -import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.background import androidx.compose.foundation.border import androidx.compose.foundation.interaction.MutableInteractionSource import androidx.compose.foundation.interaction.collectIsFocusedAsState import androidx.compose.foundation.layout.fillMaxWidth -import androidx.compose.foundation.relocation.BringIntoViewRequester -import androidx.compose.foundation.relocation.bringIntoViewRequester import androidx.compose.foundation.text.BasicTextField import androidx.compose.foundation.text.KeyboardActions import androidx.compose.foundation.text.KeyboardOptions import androidx.compose.runtime.Composable -import androidx.compose.runtime.LaunchedEffect -import androidx.compose.runtime.State import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier -import androidx.compose.ui.focus.onFocusEvent -import androidx.compose.ui.geometry.toRect import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.SolidColor -import androidx.compose.ui.layout.LayoutCoordinates -import androidx.compose.ui.layout.onGloballyPositioned -import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.res.stringResource import androidx.compose.ui.semantics.error import androidx.compose.ui.semantics.semantics import androidx.compose.ui.text.input.TextFieldValue import androidx.compose.ui.text.input.VisualTransformation import androidx.compose.ui.unit.dp -import androidx.compose.ui.unit.toSize import kiwi.orbit.compose.icons.Icons import kiwi.orbit.compose.ui.OrbitTheme import kiwi.orbit.compose.ui.R @@ -47,10 +36,6 @@ import kiwi.orbit.compose.ui.controls.internal.OrbitPreviews import kiwi.orbit.compose.ui.controls.internal.Preview import kiwi.orbit.compose.ui.foundation.LocalTextStyle import kiwi.orbit.compose.ui.foundation.ProvideMergedTextStyle -import kotlinx.coroutines.FlowPreview -import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.collectLatest -import kotlinx.coroutines.flow.debounce @Composable public fun TextField( @@ -71,7 +56,6 @@ public fun TextField( keyboardActions: KeyboardActions = KeyboardActions.Default, singleLine: Boolean = true, maxLines: Int = Int.MAX_VALUE, - bringIntoView: Boolean = true, visualTransformation: VisualTransformation = VisualTransformation.None, interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }, ) { @@ -94,13 +78,11 @@ public fun TextField( keyboardActions = keyboardActions, singleLine = singleLine, maxLines = maxLines, - bringIntoView = bringIntoView, visualTransformation = visualTransformation, interactionSource = interactionSource, ) } -@OptIn(ExperimentalFoundationApi::class) @Composable internal fun TextField( value: String, @@ -121,29 +103,9 @@ internal fun TextField( keyboardActions: KeyboardActions = KeyboardActions.Default, singleLine: Boolean = true, maxLines: Int = Int.MAX_VALUE, - bringIntoView: Boolean = true, visualTransformation: VisualTransformation = VisualTransformation.None, interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }, ) { - val autoBringIntoViewSetupModifier: Modifier - val autoBringIntoViewFocusModifier: Modifier - - if (bringIntoView) { - val bringIntoViewRequester = remember { BringIntoViewRequester() } - val layoutCoordinates = remember { mutableStateOf(null) } - val focused = remember { mutableStateOf(false) } - - autoBringIntoViewSetupModifier = Modifier - .bringIntoViewRequester(bringIntoViewRequester) - .onGloballyPositioned { layoutCoordinates.value = it } - autoBringIntoViewFocusModifier = Modifier.onFocusEvent { focused.value = it.isFocused } - - BringIntoViewWhenFocused(focused, layoutCoordinates, bringIntoViewRequester) - } else { - autoBringIntoViewSetupModifier = Modifier - autoBringIntoViewFocusModifier = Modifier - } - val errorMessage = stringResource(R.string.orbit_field_default_error) ColumnWithMinConstraints( modifier @@ -151,8 +113,7 @@ internal fun TextField( if (error != null) { this.error(errorMessage) } - } - .then(autoBringIntoViewSetupModifier), + }, ) { ProvideMergedTextStyle(OrbitTheme.typography.bodyNormal) { var textFieldValueState by remember { mutableStateOf(TextFieldValue(text = value)) } @@ -199,7 +160,6 @@ internal fun TextField( } }, modifier = Modifier - .then(autoBringIntoViewFocusModifier) .border(1.dp, borderColor.value, OrbitTheme.shapes.normal) .background(OrbitTheme.colors.surface.normal, OrbitTheme.shapes.normal), enabled = enabled, @@ -238,34 +198,6 @@ internal fun TextField( } } -@OptIn(FlowPreview::class, ExperimentalFoundationApi::class) -@Composable -private fun BringIntoViewWhenFocused( - focused: State, - layoutCoordinates: State, - bringIntoViewRequester: BringIntoViewRequester, -) { - val density = LocalDensity.current - val scaffoldBottomPadding = remember { - MutableStateFlow(0f) - } - val height = with(density) { - LocalScaffoldPadding.current.calculateBottomPadding().toPx() - } - LaunchedEffect(height) { - scaffoldBottomPadding.emit(height) - } - LaunchedEffect(focused.value) { - if (focused.value) { - scaffoldBottomPadding.debounce(100).collectLatest { scaffoldBottomPadding -> - val size = layoutCoordinates.value?.size?.toSize() ?: return@collectLatest - val paddedSize = size.copy(height = size.height + scaffoldBottomPadding) - bringIntoViewRequester.bringIntoView(paddedSize.toRect()) - } - } - } -} - private enum class InputState { Normal, NormalError, diff --git a/ui/src/test/kotlin/kiwi/orbit/compose/ui/controls/ScaffoldTest.kt b/ui/src/test/kotlin/kiwi/orbit/compose/ui/controls/ScaffoldTest.kt new file mode 100644 index 000000000..7d0cf3de9 --- /dev/null +++ b/ui/src/test/kotlin/kiwi/orbit/compose/ui/controls/ScaffoldTest.kt @@ -0,0 +1,82 @@ +package kiwi.orbit.compose.ui.controls + +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.lazy.LazyColumn +import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.verticalScroll +import androidx.compose.ui.Modifier +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.compose.ui.test.performScrollTo +import androidx.compose.ui.test.performScrollToIndex +import androidx.compose.ui.unit.dp +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +internal class ScaffoldTest { + @get:Rule + val composeTestRule = createComposeRule() + + @Test + fun testScrollingLazyColumn() { + val clicked = mutableSetOf() + composeTestRule.setContent { + Scaffold( + modifier = Modifier.height(300.dp), + topBar = { TopAppBarLarge({ Text("Title") }) }, + action = { ButtonPrimary(onClick = {}) { Text("Action") } }, + ) { contentPadding -> + LazyColumn(Modifier.testTag("list"), contentPadding = contentPadding) { + items(50) { + ListChoice(onClick = { clicked.add(it) }) { + Text("Item $it") + } + } + } + } + } + composeTestRule.onNodeWithTag("list").performScrollToIndex(49) + composeTestRule.onNodeWithText("Item 49").performClick() + assert(clicked.contains(49)) + composeTestRule.onNodeWithTag("list").performScrollToIndex(0) + composeTestRule.onNodeWithText("Item 0").performClick() + assert(clicked.contains(0)) + } + + @Test + fun testScrollingColumn() { + val clicked = mutableSetOf() + composeTestRule.setContent { + Scaffold( + modifier = Modifier.height(300.dp), + topBar = { TopAppBarLarge({ Text("Title") }) }, + action = { ButtonPrimary(onClick = {}) { Text("Action") } }, + ) { contentPadding -> + Column( + Modifier + .testTag("list") + .verticalScroll(rememberScrollState()) + .padding(contentPadding), + ) { + repeat(50) { + ListChoice(onClick = { clicked.add(it) }) { + Text("Item $it") + } + } + } + } + } + composeTestRule.onNodeWithText("Item 49").performScrollTo().performClick() + assert(clicked.contains(49)) + composeTestRule.onNodeWithText("Item 0").performScrollTo().performClick() + assert(clicked.contains(0)) + } +}