Fix a bug with Modifier.flex() nested in a row and column (#2362)

* Fix a bug with Modifier.flex() nested in a row and column This is a bug in Android only caused by not explicitly handling MeasureSpec.UNSPECIFIED. * New snapshots for the test.
cashapp · Oct 4, 2024 · 1d5d711 · 1d5d711
1 parent 56118f2
commit 1d5d711
Show file tree

Hide file tree

Showing 18 changed files with 118 additions and 0 deletions.
diff --git a/...rc/androidUnitTest/kotlin/app/cash/redwood/layout/composeui/ComposeUiFlexContainerTest.kt b/...rc/androidUnitTest/kotlin/app/cash/redwood/layout/composeui/ComposeUiFlexContainerTest.kt
@@ -24,6 +24,7 @@ import app.cash.paparazzi.Paparazzi
 import app.cash.redwood.layout.AbstractFlexContainerTest
 import app.cash.redwood.layout.TestFlexContainer
 import app.cash.redwood.layout.widget.FlexContainer
+import app.cash.redwood.layout.widget.Spacer
 import app.cash.redwood.snapshot.testing.ComposeSnapshotter
 import app.cash.redwood.snapshot.testing.ComposeUiTestWidgetFactory
 import app.cash.redwood.ui.Px
@@ -62,6 +63,11 @@ class ComposeUiFlexContainerTest(
 
   override fun column() = flexContainer(FlexDirection.Column)
 
+  override fun spacer(backgroundColor: Int): Spacer<@Composable () -> Unit> {
+    // TODO: honor backgroundColor.
+    return ComposeUiRedwoodLayoutWidgetFactory().Spacer()
+  }
+
   override fun snapshotter(widget: @Composable () -> Unit) = ComposeSnapshotter(paparazzi, widget)
 
   class ComposeTestFlexContainer private constructor(

diff --git a/...poseUiFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png b/...poseUiFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png
diff --git a/...poseUiFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png b/...poseUiFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png
diff --git a/...ut-shared-test/src/commonMain/kotlin/app/cash/redwood/layout/AbstractFlexContainerTest.kt b/...ut-shared-test/src/commonMain/kotlin/app/cash/redwood/layout/AbstractFlexContainerTest.kt
@@ -22,6 +22,7 @@ import app.cash.redwood.layout.api.MainAxisAlignment
 import app.cash.redwood.layout.api.Overflow
 import app.cash.redwood.layout.widget.Column
 import app.cash.redwood.layout.widget.Row
+import app.cash.redwood.layout.widget.Spacer
 import app.cash.redwood.snapshot.testing.Blue
 import app.cash.redwood.snapshot.testing.Green
 import app.cash.redwood.snapshot.testing.Red
@@ -53,6 +54,10 @@ abstract class AbstractFlexContainerTest<T : Any> {
   /** Returns a non-lazy flex container column, even if the test is for a LazyList. */
   abstract fun column(): Column<T>
 
+  abstract fun spacer(
+    backgroundColor: Int = argb(17, 0, 0, 0),
+  ): Spacer<T>
+
   abstract fun snapshotter(widget: T): Snapshotter
 
   @Test fun testEmptyLayout_Column() {
@@ -829,6 +834,43 @@ abstract class AbstractFlexContainerTest<T : Any> {
 
     snapshotter(fullWidthParent.value).snapshot()
   }
+
+  /**
+   * We were incorrectly collapsing the dimensions of the widget.
+   * https://github.com/cashapp/redwood/issues/2018
+   */
+  @Test fun testWidgetWithFlexModifierNestedInRowAndColumn() {
+    val root = flexContainer(FlexDirection.Column).apply {
+      width(Constraint.Fill)
+      height(Constraint.Fill)
+      margin(Margin(top = 24.dp))
+      modifier = Modifier
+    }
+
+    val rootChild0 = row().apply {
+      width(Constraint.Fill)
+      horizontalAlignment(MainAxisAlignment.Center)
+      root.children.insert(0, this)
+    }
+
+    val rootChild0Child0 = column().apply {
+      width(Constraint.Fill)
+      height(Constraint.Fill)
+      horizontalAlignment(CrossAxisAlignment.Stretch)
+      modifier = Modifier
+        .then(MarginImpl(Margin(start = 24.dp, end = 24.dp)))
+        .then(FlexImpl(1.0))
+      rootChild0.children.insert(0, this)
+    }
+
+    val rootChild0Child0Child0 = spacer().apply {
+      width(48.dp)
+      height(48.dp)
+      rootChild0Child0.children.insert(0, this)
+    }
+
+    snapshotter(root.value).snapshot()
+  }
 }
 
 interface TestFlexContainer<T : Any> :

diff --git a/...IViewFlexContainerTestHost/testWidgetWithFlexModifierNestedInRowAndColumn.1.png b/...IViewFlexContainerTestHost/testWidgetWithFlexModifierNestedInRowAndColumn.1.png
diff --git a/...ut-uiview/src/commonTest/kotlin/app/cash/redwood/layout/uiview/UIViewFlexContainerTest.kt b/...ut-uiview/src/commonTest/kotlin/app/cash/redwood/layout/uiview/UIViewFlexContainerTest.kt
@@ -21,6 +21,7 @@ import app.cash.redwood.layout.TestFlexContainer
 import app.cash.redwood.layout.api.Constraint
 import app.cash.redwood.layout.api.CrossAxisAlignment
 import app.cash.redwood.layout.widget.FlexContainer
+import app.cash.redwood.layout.widget.Spacer
 import app.cash.redwood.snapshot.testing.UIViewSnapshotCallback
 import app.cash.redwood.snapshot.testing.UIViewSnapshotter
 import app.cash.redwood.snapshot.testing.UIViewTestWidgetFactory
@@ -68,6 +69,13 @@ class UIViewFlexContainerTest(
 
   override fun column() = flexContainer(FlexDirection.Column)
 
+  override fun spacer(backgroundColor: Int): Spacer<UIView> {
+    return UIViewRedwoodLayoutWidgetFactory().Spacer()
+      .apply {
+        value.backgroundColor = backgroundColor.toUIColor()
+      }
+  }
+
   class UIViewTestFlexContainer internal constructor(
     private val delegate: UIViewFlexContainer,
   ) : TestFlexContainer<UIView>,

diff --git a/redwood-layout-view/src/main/kotlin/app/cash/redwood/layout/view/YogaLayout.kt b/redwood-layout-view/src/main/kotlin/app/cash/redwood/layout/view/YogaLayout.kt
@@ -73,11 +73,13 @@ internal class YogaLayout(context: Context) : ViewGroup(context) {
     calculateLayout(
       requestedWidth = when {
         widthMode == MeasureSpec.EXACTLY -> widthSize.toFloat()
+        widthMode == MeasureSpec.UNSPECIFIED -> Size.UNDEFINED
         widthConstraint == Constraint.Fill -> widthSize.toFloat()
         else -> Size.UNDEFINED
       },
       requestedHeight = when {
         heightMode == MeasureSpec.EXACTLY -> heightSize.toFloat()
+        heightMode == MeasureSpec.UNSPECIFIED -> Size.UNDEFINED
         heightConstraint == Constraint.Fill -> heightSize.toFloat()
         else -> Size.UNDEFINED
       },

diff --git a/redwood-layout-view/src/test/kotlin/app/cash/redwood/layout/view/ViewFlexContainerTest.kt b/redwood-layout-view/src/test/kotlin/app/cash/redwood/layout/view/ViewFlexContainerTest.kt
@@ -21,6 +21,7 @@ import app.cash.paparazzi.Paparazzi
 import app.cash.redwood.layout.AbstractFlexContainerTest
 import app.cash.redwood.layout.TestFlexContainer
 import app.cash.redwood.layout.widget.FlexContainer
+import app.cash.redwood.layout.widget.Spacer
 import app.cash.redwood.snapshot.testing.ViewSnapshotter
 import app.cash.redwood.snapshot.testing.ViewTestWidgetFactory
 import app.cash.redwood.ui.Px
@@ -63,6 +64,13 @@ class ViewFlexContainerTest(
 
   override fun column() = flexContainer(FlexDirection.Column)
 
+  override fun spacer(backgroundColor: Int): Spacer<View> {
+    return ViewSpacer(paparazzi.context)
+      .apply {
+        setBackgroundColor(backgroundColor)
+      }
+  }
+
   override fun snapshotter(widget: View) = ViewSnapshotter(paparazzi, widget)
 
   class ViewTestFlexContainer internal constructor(

diff --git a/...w_ViewFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png b/...w_ViewFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png
diff --git a/...w_ViewFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png b/...w_ViewFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png
diff --git a/...eui/src/androidUnitTest/kotlin/app/cash/redwood/layout/composeui/ComposeUiLazyListTest.kt b/...eui/src/androidUnitTest/kotlin/app/cash/redwood/layout/composeui/ComposeUiLazyListTest.kt
@@ -27,6 +27,7 @@ import app.cash.redwood.layout.api.MainAxisAlignment
 import app.cash.redwood.layout.api.Overflow
 import app.cash.redwood.layout.widget.Column
 import app.cash.redwood.layout.widget.Row
+import app.cash.redwood.layout.widget.Spacer
 import app.cash.redwood.lazylayout.composeui.ComposeUiLazyList
 import app.cash.redwood.lazylayout.widget.LazyList
 import app.cash.redwood.snapshot.testing.ComposeSnapshotter
@@ -70,6 +71,11 @@ class ComposeUiLazyListTest(
     return ComposeUiRedwoodLayoutWidgetFactory().Column()
   }
 
+  override fun spacer(backgroundColor: Int): Spacer<@Composable () -> Unit> {
+    // TODO: honor backgroundColor.
+    return ComposeUiRedwoodLayoutWidgetFactory().Spacer()
+  }
+
   override fun snapshotter(widget: @Composable () -> Unit) = ComposeSnapshotter(paparazzi, widget)
 
   class ComposeTestFlexContainer private constructor(

diff --git a/...i_ComposeUiLazyListTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png b/...i_ComposeUiLazyListTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png
diff --git a/...i_ComposeUiLazyListTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png b/...i_ComposeUiLazyListTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png
diff --git a/...istAsFlexContainerTestHost/testWidgetWithFlexModifierNestedInRowAndColumn.1.png b/...istAsFlexContainerTestHost/testWidgetWithFlexModifierNestedInRowAndColumn.1.png
diff --git a/...commonTest/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyListAsFlexContainerTest.kt b/...commonTest/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyListAsFlexContainerTest.kt
@@ -20,6 +20,7 @@ import app.cash.redwood.layout.TestFlexContainer
 import app.cash.redwood.layout.api.MainAxisAlignment
 import app.cash.redwood.layout.api.Overflow
 import app.cash.redwood.layout.uiview.UIViewRedwoodLayoutWidgetFactory
+import app.cash.redwood.layout.widget.Spacer
 import app.cash.redwood.lazylayout.toUIColor
 import app.cash.redwood.lazylayout.widget.LazyList
 import app.cash.redwood.snapshot.testing.UIViewSnapshotCallback
@@ -47,6 +48,13 @@ class UIViewLazyListAsFlexContainerTest(
 
   override fun column() = UIViewRedwoodLayoutWidgetFactory().Column()
 
+  override fun spacer(backgroundColor: Int): Spacer<UIView> {
+    return UIViewRedwoodLayoutWidgetFactory().Spacer()
+      .apply {
+        value.backgroundColor = backgroundColor.toUIColor()
+      }
+  }
+
   override fun snapshotter(widget: UIView) = UIViewSnapshotter.framed(callback, widget)
 
   class ViewTestFlexContainer private constructor(

diff --git a/...-view/src/test/kotlin/app/cash/redwood/lazylayout/view/ViewLazyListAsFlexContainerTest.kt b/...-view/src/test/kotlin/app/cash/redwood/lazylayout/view/ViewLazyListAsFlexContainerTest.kt
@@ -26,6 +26,7 @@ import app.cash.redwood.layout.api.Overflow
 import app.cash.redwood.layout.view.ViewRedwoodLayoutWidgetFactory
 import app.cash.redwood.layout.widget.Column
 import app.cash.redwood.layout.widget.Row
+import app.cash.redwood.layout.widget.Spacer
 import app.cash.redwood.lazylayout.widget.LazyList
 import app.cash.redwood.snapshot.testing.ViewSnapshotter
 import app.cash.redwood.snapshot.testing.ViewTestWidgetFactory
@@ -69,6 +70,13 @@ class ViewLazyListAsFlexContainerTest(
     return ViewRedwoodLayoutWidgetFactory(paparazzi.context).Column()
   }
 
+  override fun spacer(backgroundColor: Int): Spacer<View> {
+    return ViewRedwoodLayoutWidgetFactory(paparazzi.context).Spacer()
+      .apply {
+        value.setBackgroundColor(backgroundColor)
+      }
+  }
+
   override fun snapshotter(widget: View) = ViewSnapshotter(paparazzi, widget)
 
   class ViewTestFlexContainer private constructor(

diff --git a/...ListAsFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png b/...ListAsFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png
diff --git a/...ListAsFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png b/...ListAsFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png