From 1841aed4ac70832382cce83394cf90b29d5bde41 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Mon, 30 Sep 2024 09:30:11 -0400 Subject: [PATCH] Fix Constraint.Wrap layouts when a view is larger than its parent (#2337) * Fix Constraint.Wrap layouts when a view is larger than its parent These were incorrectly overflowing the parent container because we were applying the target size at the wrong layer. This fixes an ugly inconsistency between iOS and Android, and makes the iOS layouts less surprising. https://github.com/cashapp/redwood/issues/2128 * CHANGELOG --- CHANGELOG.md | 1 + ...stChildIsConstrainedToParentWidth[LTR].png | 3 ++ ...stChildIsConstrainedToParentWidth[RTL].png | 3 ++ .../layout/AbstractFlexContainerTest.kt | 35 ++++++++++++++ .../testChildIsConstrainedToParentWidth.1.png | 3 ++ .../testColumnThenRow.1.png | 4 +- .../layout/uiview/UIViewFlexContainer.kt | 4 +- .../cash/redwood/layout/uiview/YogaUIView.kt | 48 +++++++++++-------- ...stChildIsConstrainedToParentWidth[LTR].png | 3 ++ ...stChildIsConstrainedToParentWidth[RTL].png | 3 ++ ...stChildIsConstrainedToParentWidth[LTR].png | 3 ++ ...stChildIsConstrainedToParentWidth[RTL].png | 3 ++ ...stChildIsConstrainedToParentWidth[LTR].png | 3 ++ ...stChildIsConstrainedToParentWidth[RTL].png | 3 ++ 14 files changed, 96 insertions(+), 23 deletions(-) create mode 100644 redwood-layout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png create mode 100644 redwood-layout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png create mode 100644 redwood-layout-uiview/RedwoodLayoutUIViewTests/__Snapshots__/UIViewFlexContainerTestHost/testChildIsConstrainedToParentWidth.1.png create mode 100644 redwood-layout-view/src/test/snapshots/images/app.cash.redwood.layout.view_ViewFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png create mode 100644 redwood-layout-view/src/test/snapshots/images/app.cash.redwood.layout.view_ViewFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png create mode 100644 redwood-lazylayout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiLazyListTest_testChildIsConstrainedToParentWidth[LTR].png create mode 100644 redwood-lazylayout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiLazyListTest_testChildIsConstrainedToParentWidth[RTL].png create mode 100644 redwood-lazylayout-view/src/test/snapshots/images/app.cash.redwood.lazylayout.view_ViewLazyListAsFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png create mode 100644 redwood-lazylayout-view/src/test/snapshots/images/app.cash.redwood.lazylayout.view_ViewLazyListAsFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png diff --git a/CHANGELOG.md b/CHANGELOG.md index 865f2babbd..0b0704613a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ Fixed: - Breaking the last remaining retain cycle in `UIViewLazyList`. - Don't leak the `DisplayLink` when a `TreehouseApp` is stopped on iOS. - Correctly handle dynamic size changes for child widgets of `Box`, `Column`, and `Row`. +- Don't clip elements of `Column` and `Row` layouts whose unbounded size exceeds the container size. - Correctly implement margins for `Box` on iOS. - Correctly handle dynamic updates to modifiers on `Column` and `Row`. diff --git a/redwood-layout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png b/redwood-layout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png new file mode 100644 index 0000000000..6fb81d1ea9 --- /dev/null +++ b/redwood-layout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:81a9963a0b5e6092ed757f885315f581fc3a96bceea8cffccd4a5b1bcd9bdb35 +size 8669 diff --git a/redwood-layout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png b/redwood-layout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png new file mode 100644 index 0000000000..5e70082ef5 --- /dev/null +++ b/redwood-layout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:599f8910469cc82b82e1e48c002ca292c71c5ca37cab70179305f45c7f35fdca +size 8772 diff --git a/redwood-layout-shared-test/src/commonMain/kotlin/app/cash/redwood/layout/AbstractFlexContainerTest.kt b/redwood-layout-shared-test/src/commonMain/kotlin/app/cash/redwood/layout/AbstractFlexContainerTest.kt index 59bd8cb5b1..99095c2b46 100644 --- a/redwood-layout-shared-test/src/commonMain/kotlin/app/cash/redwood/layout/AbstractFlexContainerTest.kt +++ b/redwood-layout-shared-test/src/commonMain/kotlin/app/cash/redwood/layout/AbstractFlexContainerTest.kt @@ -782,6 +782,41 @@ abstract class AbstractFlexContainerTest { rowA2.text("A-TWO ".repeat(5)) snapshotter.snapshot("v2") } + + /** + * When a child widget's intrinsic size won't fit in the available space, what happens? We can + * either let it have its requested size anyway (and overrun the available space) or we confine it + * to the space available. + */ + @Test fun testChildIsConstrainedToParentWidth() { + // Wrap in a parent column to let us configure an exact width for our subject flex container. + // Otherwise we're relying on the platform-specific snapshot library's unspecified frame width. + val fullWidthParent = column().apply { + width(Constraint.Fill) + height(Constraint.Fill) + } + + val alignStart = HorizontalAlignmentImpl(CrossAxisAlignment.Start) + flexContainer(FlexDirection.Column) + .apply { + modifier = WidthImpl(25.dp) + add(widgetFactory.text("ok", alignStart)) // This is under 25.dp in width. + add(widgetFactory.text("1 2 3 4", alignStart)) // Each character is under 25.dp in width. + onEndChanges() + } + .also { fullWidthParent.children.insert(0, it) } + + flexContainer(FlexDirection.Column) + .apply { + modifier = WidthImpl(25.dp) + add(widgetFactory.text("overflows parent", alignStart)) // This is over 25.dp in width. + add(widgetFactory.text("1 2 3 4", alignStart)) // Each character is under 25.dp in width. + onEndChanges() + } + .also { fullWidthParent.children.insert(1, it) } + + snapshotter(fullWidthParent.value).snapshot() + } } interface TestFlexContainer : diff --git a/redwood-layout-uiview/RedwoodLayoutUIViewTests/__Snapshots__/UIViewFlexContainerTestHost/testChildIsConstrainedToParentWidth.1.png b/redwood-layout-uiview/RedwoodLayoutUIViewTests/__Snapshots__/UIViewFlexContainerTestHost/testChildIsConstrainedToParentWidth.1.png new file mode 100644 index 0000000000..917dabee44 --- /dev/null +++ b/redwood-layout-uiview/RedwoodLayoutUIViewTests/__Snapshots__/UIViewFlexContainerTestHost/testChildIsConstrainedToParentWidth.1.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:0a79c0d38522567fa6530cde363d3c0902f02b8a3dce2c7417183e745065a404 +size 82103 diff --git a/redwood-layout-uiview/RedwoodLayoutUIViewTests/__Snapshots__/UIViewFlexContainerTestHost/testColumnThenRow.1.png b/redwood-layout-uiview/RedwoodLayoutUIViewTests/__Snapshots__/UIViewFlexContainerTestHost/testColumnThenRow.1.png index 3d53e193d1..ab96e0965f 100644 --- a/redwood-layout-uiview/RedwoodLayoutUIViewTests/__Snapshots__/UIViewFlexContainerTestHost/testColumnThenRow.1.png +++ b/redwood-layout-uiview/RedwoodLayoutUIViewTests/__Snapshots__/UIViewFlexContainerTestHost/testColumnThenRow.1.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:ef515d673afc46ae662ca0dc227efd17a8903434ede8fa7567f22bec6930db8c -size 98527 +oid sha256:a6a7f711ad8a208ce6b6a0c7dac78d2dce3b55866fd062beaa4ec5a66f99d66e +size 98586 diff --git a/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/UIViewFlexContainer.kt b/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/UIViewFlexContainer.kt index 1a29915fbb..a1aff937b0 100644 --- a/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/UIViewFlexContainer.kt +++ b/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/UIViewFlexContainer.kt @@ -76,11 +76,11 @@ internal class UIViewFlexContainer( } override fun width(width: Constraint) { - yogaView.width = width + yogaView.widthConstraint = width } override fun height(height: Constraint) { - yogaView.height = height + yogaView.heightConstraint = height } override fun overflow(overflow: Overflow) { diff --git a/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/YogaUIView.kt b/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/YogaUIView.kt index 781cf24486..ea8f891a44 100644 --- a/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/YogaUIView.kt +++ b/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/YogaUIView.kt @@ -30,8 +30,8 @@ internal class YogaUIView( ) : UIScrollView(cValue { CGRectZero }), UIScrollViewDelegateProtocol { val rootNode = Node() - var width = Constraint.Wrap - var height = Constraint.Wrap + var widthConstraint = Constraint.Wrap + var heightConstraint = Constraint.Wrap var onScroll: ((Px) -> Unit)? = null @@ -44,12 +44,11 @@ internal class YogaUIView( } override fun intrinsicContentSize(): CValue { - return calculateLayoutWithSize(CGSizeMake(Size.UNDEFINED.toDouble(), Size.UNDEFINED.toDouble())) + return calculateLayoutWithSize(Size.UNDEFINED, Size.UNDEFINED) } override fun sizeThatFits(size: CValue): CValue { - val constrainedSize = size.useContents { sizeForConstraints(this) } - return calculateLayoutWithSize(constrainedSize) + return calculateLayoutWithSize(size) } override fun layoutSubviews() { @@ -58,7 +57,7 @@ internal class YogaUIView( // Based on the constraints of Fill or Wrap, we // calculate a size that the container should fit in. val bounds = bounds.useContents { - sizeForConstraints(size) + CGSizeMake(size.width, size.height) } if (scrollEnabled) { @@ -96,6 +95,24 @@ internal class YogaUIView( } private fun calculateLayoutWithSize(size: CValue): CValue { + return size.useContents { + calculateLayoutWithSize(width.toYoga(), height.toYoga()) + } + } + + private fun calculateLayoutWithSize(width: Float, height: Float): CValue { + rootNode.requestedWidth = when (widthConstraint) { + Constraint.Fill -> width + else -> Size.UNDEFINED + } + rootNode.requestedMaxWidth = width + + rootNode.requestedHeight = when (heightConstraint) { + Constraint.Fill -> height + else -> Size.UNDEFINED + } + rootNode.requestedMaxHeight = height + for ((index, node) in rootNode.children.withIndex()) { applyModifier(node, index) } @@ -104,23 +121,16 @@ internal class YogaUIView( rootNode.markEverythingDirty() } - size.useContents { rootNode.measureOnly(width.toFloat(), height.toFloat()) } + rootNode.measureOnly(width, height) return CGSizeMake(rootNode.width.toDouble(), rootNode.height.toDouble()) } - private fun sizeForConstraints(size: CGSize): CValue { - return CGSizeMake( - width = sizeForConstraintsDimension(width, size.width), - height = sizeForConstraintsDimension(height, size.height), - ) - } - - private fun sizeForConstraintsDimension(constraint: Constraint, dimension: Double): Double { - if (constraint == Constraint.Wrap || dimension == UIViewNoIntrinsicMetric) { - return Size.UNDEFINED.toDouble() - } else { - return dimension + /** Convert a UIView dimension (a double) to a Yoga dimension (a float). */ + private fun Double.toYoga(): Float { + return when (this) { + UIViewNoIntrinsicMetric -> Size.UNDEFINED + else -> this.toFloat() } } diff --git a/redwood-layout-view/src/test/snapshots/images/app.cash.redwood.layout.view_ViewFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png b/redwood-layout-view/src/test/snapshots/images/app.cash.redwood.layout.view_ViewFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png new file mode 100644 index 0000000000..bd5e721338 --- /dev/null +++ b/redwood-layout-view/src/test/snapshots/images/app.cash.redwood.layout.view_ViewFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:7a9605a301e6b96a8d41b8a3f7fb237106a443cf06f3139aa55fd675dc01fe21 +size 8853 diff --git a/redwood-layout-view/src/test/snapshots/images/app.cash.redwood.layout.view_ViewFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png b/redwood-layout-view/src/test/snapshots/images/app.cash.redwood.layout.view_ViewFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png new file mode 100644 index 0000000000..6e74f501f9 --- /dev/null +++ b/redwood-layout-view/src/test/snapshots/images/app.cash.redwood.layout.view_ViewFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d1fe8b4c8923cf667d62c544c05b39665b906c3b70a253964edbc8a365842da5 +size 8844 diff --git a/redwood-lazylayout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiLazyListTest_testChildIsConstrainedToParentWidth[LTR].png b/redwood-lazylayout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiLazyListTest_testChildIsConstrainedToParentWidth[LTR].png new file mode 100644 index 0000000000..4073a34479 --- /dev/null +++ b/redwood-lazylayout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiLazyListTest_testChildIsConstrainedToParentWidth[LTR].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:ad0191c6e03eafe37ff8b80a6fc85087f88862741da713c5b7f5d1e79fa758be +size 8995 diff --git a/redwood-lazylayout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiLazyListTest_testChildIsConstrainedToParentWidth[RTL].png b/redwood-lazylayout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiLazyListTest_testChildIsConstrainedToParentWidth[RTL].png new file mode 100644 index 0000000000..5bc0e7002f --- /dev/null +++ b/redwood-lazylayout-composeui/src/test/snapshots/images/app.cash.redwood.layout.composeui_ComposeUiLazyListTest_testChildIsConstrainedToParentWidth[RTL].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:81b35a1e916dadda6045b4471e6f15385e03184fff94341cc3b2f2e24cea1f16 +size 8830 diff --git a/redwood-lazylayout-view/src/test/snapshots/images/app.cash.redwood.lazylayout.view_ViewLazyListAsFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png b/redwood-lazylayout-view/src/test/snapshots/images/app.cash.redwood.lazylayout.view_ViewLazyListAsFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png new file mode 100644 index 0000000000..b845bcba31 --- /dev/null +++ b/redwood-lazylayout-view/src/test/snapshots/images/app.cash.redwood.lazylayout.view_ViewLazyListAsFlexContainerTest_testChildIsConstrainedToParentWidth[LTR].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:1de287706d5558908aa28cdacb115d20a8aec05dff7981095406b619a6f3166c +size 8851 diff --git a/redwood-lazylayout-view/src/test/snapshots/images/app.cash.redwood.lazylayout.view_ViewLazyListAsFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png b/redwood-lazylayout-view/src/test/snapshots/images/app.cash.redwood.lazylayout.view_ViewLazyListAsFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png new file mode 100644 index 0000000000..4928982350 --- /dev/null +++ b/redwood-lazylayout-view/src/test/snapshots/images/app.cash.redwood.lazylayout.view_ViewLazyListAsFlexContainerTest_testChildIsConstrainedToParentWidth[RTL].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:c1184906d0c1edae1f05a06c4c6e3007c727be9c0ce13d8afb6a7a249cae6669 +size 8848