Skip to content

Commit

Permalink
In placeAutoMirrored for RTL, calculate the placement position using …
Browse files Browse the repository at this point in the history
…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 <oleksandr.karpovich@jetbrains.com>
  • Loading branch information
eymar and Oleksandr Karpovich authored Aug 9, 2022
1 parent 5036fca commit b70a3b0
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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))
Expand Down Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,30 @@
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
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
Expand All @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down

0 comments on commit b70a3b0

Please sign in to comment.