From 0975e96d53546ac05b2154352fe56e5d82e2a1f8 Mon Sep 17 00:00:00 2001 From: Xin Chen Date: Tue, 1 Mar 2022 14:33:04 -0800 Subject: [PATCH] Fix transform when calculate overflowInset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This diff fixes overflowInset calculation when a shadow node has transform matrix from a transfrom prop in JS. Specifically, this fixed the use case when transform directly used on a view component. When using Animated.View, it will create an invisible wrapper which will behave correctly with existing logic. This diff bring both use cases to work properly. When a shadow node has transform on it, it will affect the overflowInset values for its parent nodes, but won't affect its own or any of its child nodes overflowInset values. This is obvious for translateX/Y case, but not for scale case. Take a look at the following case: ``` ┌────────────────┐ ┌────────────────┐ ┌────────────────┐ │Original Layout │ │ Translate AB │ │ Scale AB │ └────────────────┘ └────────────────┘ └────────────────┘ ─────▶ ◀───── ─────▶ ┌ ─ ─ ─ ┬──────────┐─ ─ ─ ─ ┐ ┌ ─ ─ ─ ┬──────────┐─ ─ ─ ─ ─ ┐ ┌ ─ ─ ─ ─ ─ ┬──────────┐─ ─ ─ ─ ─ ┐ │ A │ │ A │ │ A │ │ │ │ │ │ │ │ │ ├ ─ ─ ─ ─ ─ ┼ ─ ─┌─────┤─ ─ ─ ─ ─ ┤ ─ ─ ─ ─│─ ─ ─┌───┐┼ ─ ─ ─ ─ │ │ ◀─ ─ ─ │ │AB │ ─ ─ ─▶ │ │ │AB ││ │ │ ┌ ─ ─ ┼ ─ ─ ─ ┬──┴┬ ─ ─ ─ ─ ┤ │ │ │ │ │ └─────┤ ├┘ └───────┤AB │ └────┤ │ │ │┌──┴─────────┤ │ │ │ │ │ │ │ │ ┌───┴──────────┤ ││ABC │ │┌──┴─────────┐ │ │ABC │ │ │└──┬─────────┤ │ │ │ ││ABC │ │ │ │ │ │ │ ┌───ABD───────┴─┐ │ │ │└──┬─────────┘ │ ▼ │ └───┬──────────┘ ├─────────────┬─┘ │ │ │ │ ├───ABD───────┴─┐ │ │ │ ├────────────────┴──┐ │ │ ─ ─ ─ ─ ─ ─ ─└───┘─ ─ ─ ─ ─ ▼ └─────────────┬─┘ │ ▼ │ ABD │ │ └ ┴ ─ ─ ─ ─ ─ ─ ┴───┴ ─ ─ ─ ─ ┘ ├────────────────┬──┘ │ │ ─ ─ ─ ─ ─ ─ ─ ─ ┴─────┴ ─ ─ ─ ─ ─ ``` For the translate case, only view A has change on the overflowInset values for `right` and `bottom`. Note that the `left` and `top` are not changed as we union before and after transform is applied. For the scale case, similar things are happening for view A, and both `left`, `right`, and `bottom` values are increased. However, for View AB or any of its children, they only *appear* to be increased, but that is purely cosmetic as it's caused by transform. The actual values are not changed, which will later be converted during render phase to actual pixels on screen. In summary, overflowInset is affected from child nodes transform matrix to the current node (bottom up), but not from transform matrix on the current node to child nodes (top down). So the correct way to apply transform is to make it only affect calculating `contentFrame` during layout, which collects child nodes layout information and their transforms. The `contentFrame` is then used to decide the overflowInset values for the parent node. The current transform matrix on parent node is never used as it's not affecting overflowInset for the current node or its child nodes. This diff reflects the context above with added unit test to cover the scale and translate transform matrix. Changelog: [Android/IOS][Fixed] - Fixed how we calculate overflowInset with transform matrix Reviewed By: sammy-SC Differential Revision: D34433404 fbshipit-source-id: 0e48e4af4cfd5a6dd32a30e7667686e8ef1a7004 --- .../view/YogaLayoutableShadowNode.cpp | 33 ++- .../components/view/tests/LayoutTest.cpp | 215 ++++++++++++++++-- .../react/renderer/graphics/Transform.cpp | 8 + .../react/renderer/graphics/Transform.h | 6 + 4 files changed, 232 insertions(+), 30 deletions(-) diff --git a/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp b/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp index 5ba750e4832887..90d6a808ea5701 100644 --- a/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp +++ b/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp @@ -479,7 +479,6 @@ void YogaLayoutableShadowNode::layout(LayoutContext layoutContext) { react_native_assert(!yogaNode_.isDirty()); auto contentFrame = Rect{}; - for (auto childYogaNode : yogaNode_.getChildren()) { auto &childNode = *static_cast(childYogaNode->getContext()); @@ -521,24 +520,36 @@ void YogaLayoutableShadowNode::layout(LayoutContext layoutContext) { auto layoutMetricsWithOverflowInset = childNode.getLayoutMetrics(); if (layoutMetricsWithOverflowInset.displayType != DisplayType::None) { + // The contentFrame should always union with existing child node layout + + // overflowInset. The transform may in a deferred animation and not + // applied yet. contentFrame.unionInPlace(insetBy( layoutMetricsWithOverflowInset.frame, layoutMetricsWithOverflowInset.overflowInset)); + + auto childTransform = childNode.getTransform(); + if (childTransform != Transform::Identity()) { + // The child node's transform matrix will affect the parent node's + // contentFrame. We need to union with child node's after transform + // layout here. + contentFrame.unionInPlace(insetBy( + layoutMetricsWithOverflowInset.frame * childTransform, + layoutMetricsWithOverflowInset.overflowInset * childTransform)); + } } } if (yogaNode_.getStyle().overflow() == YGOverflowVisible) { - auto transform = getTransform(); - auto transformedContentFrame = contentFrame; - if (Transform::Identity() != transform) { - // When animation uses native driver, Yoga has no knowledge of the - // animation. In case the content goes out from current container, we need - // to union the content frame with its transformed frame. - transformedContentFrame = contentFrame * getTransform(); - transformedContentFrame.unionInPlace(contentFrame); - } + // Note that the parent node's overflow layout is NOT affected by its + // transform matrix. That transform matrix is applied on the parent node as + // well as all of its child nodes, which won't cause changes on the + // overflowInset values. A special note on the scale transform -- the scaled + // layout may look like it's causing overflowInset changes, but it's purely + // cosmetic and will be handled by pixel density conversion logic later when + // render the view. The actual overflowInset value is not changed as if the + // transform is not happening here. layoutMetrics_.overflowInset = - calculateOverflowInset(layoutMetrics_.frame, transformedContentFrame); + calculateOverflowInset(layoutMetrics_.frame, contentFrame); } else { layoutMetrics_.overflowInset = {}; } diff --git a/ReactCommon/react/renderer/components/view/tests/LayoutTest.cpp b/ReactCommon/react/renderer/components/view/tests/LayoutTest.cpp index 6d907a3dcb0dd8..691fb5b6bec922 100644 --- a/ReactCommon/react/renderer/components/view/tests/LayoutTest.cpp +++ b/ReactCommon/react/renderer/components/view/tests/LayoutTest.cpp @@ -17,6 +17,9 @@ namespace facebook { namespace react { +// Note: the (x, y) origin is always relative to the parent node. You may use +// P482342650 to re-create this test case in playground. + // *******************************************************┌─ABCD:────┐**** // *******************************************************│ {70,-50} │**** // *******************************************************│ {30,60} │**** @@ -43,6 +46,8 @@ namespace react { // ***********************│ │************************************ // ***********************└──────────┘************************************ +enum TestCase { AS_IS, CLIPPING, TRANSFORM_SCALE, TRANSFORM_TRANSLATE }; + class LayoutTest : public ::testing::Test { protected: ComponentBuilder builder_; @@ -55,7 +60,7 @@ class LayoutTest : public ::testing::Test { LayoutTest() : builder_(simpleComponentBuilder()) {} - void initialize(bool enforceClippingForABC) { + void initialize(TestCase testCase) { // clang-format off auto element = Element() @@ -73,6 +78,7 @@ class LayoutTest : public ::testing::Test { .children({ Element() .reference(viewShadowNodeA_) + .tag(2) .props([] { auto sharedProps = std::make_shared(); auto &props = *sharedProps; @@ -85,7 +91,8 @@ class LayoutTest : public ::testing::Test { .children({ Element() .reference(viewShadowNodeAB_) - .props([] { + .tag(3) + .props([=] { auto sharedProps = std::make_shared(); auto &props = *sharedProps; auto &yogaStyle = props.yogaStyle; @@ -94,17 +101,26 @@ class LayoutTest : public ::testing::Test { yogaStyle.position()[YGEdgeTop] = YGValue{10, YGUnitPoint}; yogaStyle.dimensions()[YGDimensionWidth] = YGValue{30, YGUnitPoint}; yogaStyle.dimensions()[YGDimensionHeight] = YGValue{90, YGUnitPoint}; + + if (testCase == TRANSFORM_SCALE) { + props.transform = props.transform * Transform::Scale(2, 2, 1); + } + + if (testCase == TRANSFORM_TRANSLATE) { + props.transform = props.transform * Transform::Translate(10, 10, 0); + } return sharedProps; }) .children({ Element() .reference(viewShadowNodeABC_) + .tag(4) .props([=] { auto sharedProps = std::make_shared(); auto &props = *sharedProps; auto &yogaStyle = props.yogaStyle; - if (enforceClippingForABC) { + if (testCase == CLIPPING) { yogaStyle.overflow() = YGOverflowHidden; } @@ -118,6 +134,7 @@ class LayoutTest : public ::testing::Test { .children({ Element() .reference(viewShadowNodeABCD_) + .tag(5) .props([] { auto sharedProps = std::make_shared(); auto &props = *sharedProps; @@ -132,6 +149,7 @@ class LayoutTest : public ::testing::Test { }), Element() .reference(viewShadowNodeABE_) + .tag(6) .props([] { auto sharedProps = std::make_shared(); auto &props = *sharedProps; @@ -154,32 +172,191 @@ class LayoutTest : public ::testing::Test { } }; +// Test the layout as described above with no extra changes TEST_F(LayoutTest, overflowInsetTest) { - initialize(false); + initialize(AS_IS); + + auto layoutMetricsA = viewShadowNodeA_->getLayoutMetrics(); - auto layoutMetrics = viewShadowNodeA_->getLayoutMetrics(); + EXPECT_EQ(layoutMetricsA.frame.size.width, 50); + EXPECT_EQ(layoutMetricsA.frame.size.height, 50); - EXPECT_EQ(layoutMetrics.frame.size.width, 50); - EXPECT_EQ(layoutMetrics.frame.size.height, 50); + EXPECT_EQ(layoutMetricsA.overflowInset.left, -50); + EXPECT_EQ(layoutMetricsA.overflowInset.top, -30); + EXPECT_EQ(layoutMetricsA.overflowInset.right, -80); + EXPECT_EQ(layoutMetricsA.overflowInset.bottom, -50); - EXPECT_EQ(layoutMetrics.overflowInset.left, -50); - EXPECT_EQ(layoutMetrics.overflowInset.top, -30); - EXPECT_EQ(layoutMetrics.overflowInset.right, -80); - EXPECT_EQ(layoutMetrics.overflowInset.bottom, -50); + auto layoutMetricsABC = viewShadowNodeABC_->getLayoutMetrics(); + + EXPECT_EQ(layoutMetricsABC.frame.size.width, 110); + EXPECT_EQ(layoutMetricsABC.frame.size.height, 20); + + EXPECT_EQ(layoutMetricsABC.overflowInset.left, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.top, -50); + EXPECT_EQ(layoutMetricsABC.overflowInset.right, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.bottom, 0); } +// Test when box ABC has clipping (aka overflow hidden) TEST_F(LayoutTest, overflowInsetWithClippingTest) { - initialize(true); + initialize(CLIPPING); + + auto layoutMetricsA = viewShadowNodeA_->getLayoutMetrics(); + + EXPECT_EQ(layoutMetricsA.frame.size.width, 50); + EXPECT_EQ(layoutMetricsA.frame.size.height, 50); + + EXPECT_EQ(layoutMetricsA.overflowInset.left, -50); + EXPECT_EQ(layoutMetricsA.overflowInset.top, 0); + EXPECT_EQ(layoutMetricsA.overflowInset.right, -80); + EXPECT_EQ(layoutMetricsA.overflowInset.bottom, -50); + + auto layoutMetricsABC = viewShadowNodeABC_->getLayoutMetrics(); + + EXPECT_EQ(layoutMetricsABC.frame.size.width, 110); + EXPECT_EQ(layoutMetricsABC.frame.size.height, 20); + + EXPECT_EQ(layoutMetricsABC.overflowInset.left, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.top, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.right, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.bottom, 0); +} + +// Test when box AB translate (10, 10, 0) in transform. The parent node's +// overflowInset will be affected, but the transformed node and its child nodes +// are not affected. Here is an example: +// +// ┌────────────────┐ ┌────────────────┐ +// │Original Layout │ │ Translate AB │ +// └────────────────┘ └────────────────┘ +// ─────▶ +// ┌ ─ ─ ─ ┬──────────┐─ ─ ─ ─ ┐ ┌ ─ ─ ─ ┬──────────┐─ ─ ─ ─ ─ ┐ +// │ A │ │ A │ +// │ │ │ │ │ │ │ │ +// ─ ─ ─ ─│─ ─ ─┌───┐┼ ─ ─ ─ ─ │ │ +// │ │ │AB ││ │ │ ┌ ─ ─ ┼ ─ ─ ─ ┬──┴┬ ─ ─ ─ ─ ┤ +// └─────┤ ├┘ └───────┤AB │ +// │ │┌──┴─────────┤ │ │ │ │ │ +// ││ABC │ │┌──┴─────────┐ +// │ │└──┬─────────┤ │ │ │ ││ABC │ +// ┌───ABD───────┴─┐ │ │ │└──┬─────────┘ +// ├─────────────┬─┘ │ │ │ │ ├───ABD───────┴─┐ │ │ +// ─ ─ ─ ─ ─ ─ ─└───┘─ ─ ─ ─ ─ ▼ └─────────────┬─┘ │ +// └ ┴ ─ ─ ─ ─ ─ ─ ┴───┴ ─ ─ ─ ─ ┘ + +TEST_F(LayoutTest, overflowInsetTransformTranslateTest) { + initialize(TRANSFORM_TRANSLATE); + + auto layoutMetricsA = viewShadowNodeA_->getLayoutMetrics(); + + EXPECT_EQ(layoutMetricsA.frame.size.width, 50); + EXPECT_EQ(layoutMetricsA.frame.size.height, 50); + + // Change on parent node + // The top/left values are NOT changing as overflowInset is union of before + // and after transform layout. In this case, we move to the right and bottom, + // so the left and top is not changing, while right and bottom values are + // increased. + EXPECT_EQ(layoutMetricsA.overflowInset.left, -50); + EXPECT_EQ(layoutMetricsA.overflowInset.top, -30); + EXPECT_EQ(layoutMetricsA.overflowInset.right, -90); + EXPECT_EQ(layoutMetricsA.overflowInset.bottom, -60); + + auto layoutMetricsAB = viewShadowNodeAB_->getLayoutMetrics(); + + EXPECT_EQ(layoutMetricsAB.frame.size.width, 30); + EXPECT_EQ(layoutMetricsAB.frame.size.height, 90); + + // No change on self node with translate transform + EXPECT_EQ(layoutMetricsAB.overflowInset.left, -60); + EXPECT_EQ(layoutMetricsAB.overflowInset.top, -40); + EXPECT_EQ(layoutMetricsAB.overflowInset.right, -90); + EXPECT_EQ(layoutMetricsAB.overflowInset.bottom, 0); + + auto layoutMetricsABC = viewShadowNodeABC_->getLayoutMetrics(); + + EXPECT_EQ(layoutMetricsABC.frame.size.width, 110); + EXPECT_EQ(layoutMetricsABC.frame.size.height, 20); + + // No change on child node + EXPECT_EQ(layoutMetricsABC.overflowInset.left, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.top, -50); + EXPECT_EQ(layoutMetricsABC.overflowInset.right, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.bottom, 0); +} + +// Test when box AB scaled 2X in transform. The parent node's overflowInset will +// be affected. However, the transformed node and its child nodes only appears +// to be affected (dashed arrow). Since all transform is cosmetic only, the +// actual values are NOT changed. It will be converted later when mapping the +// values to pixels during rendering. Here is an example: +// +// ┌────────────────┐ ┌────────────────┐ +// │Original Layout │ │ Scale AB │ +// └────────────────┘ └────────────────┘ +// ─────▶ +// ┌ ─ ─ ─ ┬──────────┐─ ─ ─ ─ ┐ ┌ ─ ─ ─ ─ ─ ┬──────────┐─ ─ ─ ─ ─ ┐ +// │ A │ │ A │ +// │ │ │ │ ├ ─ ─ ─ ─ ─ ┼ ─ ─┌─────┤─ ─ ─ ─ ─ ┤ +// ─ ─ ─ ─│─ ─ ─┌───┐┼ ─ ─ ─ ─ │ │AB │ ─ ─ ─▶ +// │ │ │AB ││ │ │ │ │ │ │ +// └─────┤ ├┘ └────┤ │ +// │ │┌──┴─────────┤ │ │ ┌───┴──────────┤ +// ││ABC │ │ │ABC │ +// │ │└──┬─────────┤ │ │ │ │ │ +// ┌───ABD───────┴─┐ │ │ │ └───┬──────────┘ +// ├─────────────┬─┘ │ │ │ ├────────────────┴──┐ │ │ +// ─ ─ ─ ─ ─ ─ ─└───┘─ ─ ─ ─ ─ ▼ │ ABD │ │ +// ├────────────────┬──┘ │ │ +// ─ ─ ─ ─ ─ ─ ─ ─ ┴─────┴ ─ ─ ─ ─ ─ + +TEST_F(LayoutTest, overflowInsetTransformScaleTest) { + initialize(TRANSFORM_SCALE); + + auto layoutMetricsA = viewShadowNodeA_->getLayoutMetrics(); + + EXPECT_EQ(layoutMetricsA.frame.size.width, 50); + EXPECT_EQ(layoutMetricsA.frame.size.height, 50); + + // Change on parent node when a child view scale up + // Note that AB scale up from its center point. The numbers are calculated + // assuming AB's center point is not moving. + EXPECT_EQ(layoutMetricsA.overflowInset.left, -125); + EXPECT_EQ(layoutMetricsA.overflowInset.top, -115); + EXPECT_EQ(layoutMetricsA.overflowInset.right, -185); + EXPECT_EQ(layoutMetricsA.overflowInset.bottom, -95); + + auto layoutMetricsAB = viewShadowNodeAB_->getLayoutMetrics(); + + // The frame of box AB won't actually scale up. The transform matrix is + // purely cosmetic and should apply later in mounting phase. + EXPECT_EQ(layoutMetricsAB.frame.size.width, 30); + EXPECT_EQ(layoutMetricsAB.frame.size.height, 90); + + // No change on self node with scale transform. This may sound a bit + // surprising, but the overflowInset values will be scaled up via pixel + // density ratio along with width/height of the view. When we do hit-testing, + // the overflowInset value will appears to be doubled as expected. + EXPECT_EQ(layoutMetricsAB.overflowInset.left, -60); + EXPECT_EQ(layoutMetricsAB.overflowInset.top, -40); + EXPECT_EQ(layoutMetricsAB.overflowInset.right, -90); + EXPECT_EQ(layoutMetricsAB.overflowInset.bottom, 0); - auto layoutMetrics = viewShadowNodeA_->getLayoutMetrics(); + auto layoutMetricsABC = viewShadowNodeABC_->getLayoutMetrics(); - EXPECT_EQ(layoutMetrics.frame.size.width, 50); - EXPECT_EQ(layoutMetrics.frame.size.height, 50); + // The frame of box ABC won't actually scale up. The transform matrix is + // purely cosmatic and should apply later in mounting phase. + EXPECT_EQ(layoutMetricsABC.frame.size.width, 110); + EXPECT_EQ(layoutMetricsABC.frame.size.height, 20); - EXPECT_EQ(layoutMetrics.overflowInset.left, -50); - EXPECT_EQ(layoutMetrics.overflowInset.top, 0); - EXPECT_EQ(layoutMetrics.overflowInset.right, -80); - EXPECT_EQ(layoutMetrics.overflowInset.bottom, -50); + // The overflowInset of ABC won't change either. This may sound a bit + // surprising, but the overflowInset values will be scaled up via pixel + // density ratio along with width/height of the view. When we do hit-testing, + // the overflowInset value will appears to be doubled as expected. + EXPECT_EQ(layoutMetricsABC.overflowInset.left, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.top, -50); + EXPECT_EQ(layoutMetricsABC.overflowInset.right, 0); + EXPECT_EQ(layoutMetricsABC.overflowInset.bottom, 0); } } // namespace react diff --git a/ReactCommon/react/renderer/graphics/Transform.cpp b/ReactCommon/react/renderer/graphics/Transform.cpp index b150fb8ca8a995..d34246fa17ebc3 100644 --- a/ReactCommon/react/renderer/graphics/Transform.cpp +++ b/ReactCommon/react/renderer/graphics/Transform.cpp @@ -360,6 +360,14 @@ Rect operator*(Rect const &rect, Transform const &transform) { transformedA, transformedB, transformedC, transformedD); } +EdgeInsets operator*(EdgeInsets const &edgeInsets, Transform const &transform) { + return EdgeInsets{ + edgeInsets.left * transform.matrix[0], + edgeInsets.top * transform.matrix[5], + edgeInsets.right * transform.matrix[0], + edgeInsets.bottom * transform.matrix[5]}; +} + Vector operator*(Transform const &transform, Vector const &vector) { return { vector.x * transform.at(0, 0) + vector.y * transform.at(1, 0) + diff --git a/ReactCommon/react/renderer/graphics/Transform.h b/ReactCommon/react/renderer/graphics/Transform.h index 16baac06d2c6da..61522d4041419f 100644 --- a/ReactCommon/react/renderer/graphics/Transform.h +++ b/ReactCommon/react/renderer/graphics/Transform.h @@ -180,6 +180,12 @@ Size operator*(Size const &size, Transform const &transform); */ Rect operator*(Rect const &rect, Transform const &transform); +/* + * Applies tranformation to the given EdgeInsets. + * ONLY SUPPORTS scale transformation. + */ +EdgeInsets operator*(EdgeInsets const &edgeInsets, Transform const &transform); + Vector operator*(Transform const &transform, Vector const &vector); } // namespace react