From b70a3b0791b82e4f70f5040a5b3408e84844b904 Mon Sep 17 00:00:00 2001 From: Oleksandr Karpovich Date: Tue, 9 Aug 2022 10:24:34 +0200 Subject: [PATCH] In placeAutoMirrored for RTL, calculate the placement position using the placeable width respecting the constraints (#267) It used to calculate the position using `measuredSize.width` which doesn't respect the constraints, and therefore the position could end up outside of the parent's bounds. Test: run ui and foundation-layout tests, added an additional demo to RtlDemo Change-Id: I946079669ef6ad783708d9f958dd4231188451c5 Co-authored-by: Oleksandr Karpovich --- .../foundation/layout/demos/RtlDemo.kt | 27 +++++ .../compose/ui/layout/RtlLayoutTest.kt | 98 +++++++++++++++++++ .../androidx/compose/ui/layout/Placeable.kt | 2 +- 3 files changed, 126 insertions(+), 1 deletion(-) diff --git a/compose/foundation/foundation-layout/integration-tests/layout-demos/src/main/java/androidx/compose/foundation/layout/demos/RtlDemo.kt b/compose/foundation/foundation-layout/integration-tests/layout-demos/src/main/java/androidx/compose/foundation/layout/demos/RtlDemo.kt index adb9dbe17e557..2b42b7742d290 100644 --- a/compose/foundation/foundation-layout/integration-tests/layout-demos/src/main/java/androidx/compose/foundation/layout/demos/RtlDemo.kt +++ b/compose/foundation/foundation-layout/integration-tests/layout-demos/src/main/java/androidx/compose/foundation/layout/demos/RtlDemo.kt @@ -24,10 +24,15 @@ import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.wrapContentHeight import androidx.compose.foundation.layout.wrapContentSize +import androidx.compose.material.Icon +import androidx.compose.material.IconButton import androidx.compose.material.Text +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.Info import androidx.compose.runtime.Composable import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.ui.Alignment @@ -42,6 +47,14 @@ import androidx.compose.ui.unit.dp @Composable fun RtlDemo() { Column(verticalArrangement = Arrangement.SpaceEvenly) { + CompositionLocalProvider(LocalLayoutDirection provides LayoutDirection.Rtl) { + Text("RTL") + TestPlacementInLimitedSpace() + } + CompositionLocalProvider(LocalLayoutDirection provides LayoutDirection.Ltr) { + Text("LTR") + TestPlacementInLimitedSpace() + } Text("TEXT", Modifier.align(Alignment.CenterHorizontally)) TestText() Text("ROW", Modifier.align(Alignment.CenterHorizontally)) @@ -73,6 +86,20 @@ fun RtlDemo() { } } +@Composable +private fun TestPlacementInLimitedSpace() { + Row(modifier = Modifier.fillMaxWidth()) { + Box(modifier = Modifier.padding(12.dp).size(56.dp).background(Color.Yellow)) { + IconButton( // adds minimumTouchTargetSize + modifier = Modifier.padding(horizontal = 12.dp), + onClick = { } + ) { + Icon(Icons.Filled.Info, null) + } + } + } +} + @Composable fun StackExample() { Box(Modifier.fillMaxSize().background(Color.LightGray)) { diff --git a/compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/layout/RtlLayoutTest.kt b/compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/layout/RtlLayoutTest.kt index 197be276ac082..038a11189e509 100644 --- a/compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/layout/RtlLayoutTest.kt +++ b/compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/layout/RtlLayoutTest.kt @@ -17,8 +17,12 @@ package androidx.compose.ui.layout import androidx.activity.compose.setContent +import androidx.compose.foundation.background import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.IntrinsicSize +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width import androidx.compose.runtime.Composable import androidx.compose.runtime.CompositionLocalProvider @@ -26,13 +30,17 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.ui.FixedSize import androidx.compose.ui.Modifier import androidx.compose.ui.geometry.Offset +import androidx.compose.ui.graphics.Color import androidx.compose.ui.node.Ref +import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.platform.LocalLayoutDirection import androidx.compose.ui.runOnUiThreadIR import androidx.compose.ui.test.TestActivity import androidx.compose.ui.unit.Constraints import androidx.compose.ui.unit.Density +import androidx.compose.ui.unit.DpSize import androidx.compose.ui.unit.LayoutDirection +import androidx.compose.ui.unit.dp import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.filters.SmallTest import org.junit.Assert @@ -44,6 +52,8 @@ import org.junit.Test import org.junit.runner.RunWith import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit +import kotlin.math.abs +import kotlin.math.roundToInt @SmallTest @RunWith(AndroidJUnit4::class) @@ -297,6 +307,94 @@ class RtlLayoutTest { assertEquals(LayoutDirection.Ltr, resultLayoutDirection.value) } + @Test + fun testChildGetsPlacedWithinContainerWithPaddingAndMinimumTouchTarget() { + // copy-pasted from TouchTarget.kt (internal in material module) + class MinimumTouchTargetModifier(val size: DpSize = DpSize(48.dp, 48.dp)) : LayoutModifier { + override fun MeasureScope.measure( + measurable: Measurable, + constraints: Constraints + ): MeasureResult { + val placeable = measurable.measure(constraints) + val width = maxOf(placeable.width, size.width.roundToPx()) + val height = maxOf(placeable.height, size.height.roundToPx()) + return layout(width, height) { + val centerX = ((width - placeable.width) / 2f).roundToInt() + val centerY = ((height - placeable.height) / 2f).roundToInt() + placeable.place(centerX, centerY) + } + } + + override fun equals(other: Any?): Boolean { + val otherModifier = other as? MinimumTouchTargetModifier ?: return false + return size == otherModifier.size + } + + override fun hashCode(): Int = size.hashCode() + } + + val latch = CountDownLatch(2) + var outerLC: LayoutCoordinates? = null + var innerLC: LayoutCoordinates? = null + var density: Density? = null + + val rowWidth = 200.dp + val outerBoxWidth = 56.dp + val padding = 16.dp + + activityTestRule.runOnUiThread { + activity.setContent { + density = LocalDensity.current + CompositionLocalProvider(LocalLayoutDirection provides LayoutDirection.Rtl) { + Row(modifier = Modifier.width(rowWidth)) { + Box( + modifier = Modifier + .onGloballyPositioned { + outerLC = it + latch.countDown() + } + .size(outerBoxWidth) + .background(color = Color.Red) + .padding(horizontal = padding) + .then(MinimumTouchTargetModifier()) + ) { + Box( + modifier = Modifier + .onGloballyPositioned { + innerLC = it + latch.countDown() + } + .size(30.dp) + .background(color = Color.Gray) + ) + } + } + } + } + } + + assertTrue(latch.await(1, TimeUnit.SECONDS)) + val (innerOffset, innerWidth) = with(innerLC!!) { + localToWindow(Offset.Zero) to size.width + } + val (outerOffset, outerWidth) = with(outerLC!!) { + localToWindow(Offset.Zero) to size.width + } + assertTrue(innerWidth < outerWidth) + assertTrue(innerOffset.x > outerOffset.x) + assertTrue(innerWidth + innerOffset.x < outerWidth + outerOffset.x) + + with(density!!) { + assertEquals(outerOffset.x.roundToInt(), rowWidth.roundToPx() - outerWidth) + val paddingPx = padding.roundToPx() + // OuterBoxLeftEdge_padding-16dp_InnerBoxLeftEdge + assertTrue(abs(outerOffset.x + paddingPx - innerOffset.x) <= 1.0) + // InnerBoxRightEdge_padding-16dp_OuterRightEdge + val outerRightEdge = outerOffset.x + outerWidth + assertTrue(abs(outerRightEdge - paddingPx - (innerOffset.x + innerWidth)) <= 1) + } + } + @Composable private fun CustomLayout( absolutePositioning: Boolean, diff --git a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/layout/Placeable.kt b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/layout/Placeable.kt index cf4e911a1904b..dbb870b1725fd 100644 --- a/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/layout/Placeable.kt +++ b/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/layout/Placeable.kt @@ -299,7 +299,7 @@ abstract class Placeable : Measured { placeApparentToRealOffset(position, zIndex, layerBlock) } else { placeApparentToRealOffset( - IntOffset(parentWidth - measuredSize.width - position.x, position.y), + IntOffset((parentWidth - width - position.x), position.y), zIndex, layerBlock )