Skip to content

Commit

Permalink
Fix issue with alternating flex direction and percent postions
Browse files Browse the repository at this point in the history
Summary:
Fixing facebook#1658. We had a problem where if a child had a different flex direction than its parent, and it also set a position as a percent, it would look at the wrong axis to evaluate the percent. What was happening was we were passing in the container's mainAxis size and crossAxis size to use to evaluate the position size if it was a percent. However, we matched these sizes with the main/cross axis of the child - which is wrong if the flex direction is different. 

I changed it so that the function just takes in ownerWidth and ownerHeight then calls isRow to determine which one to use for the main/cross axis position. This reduces the ambiguity quite a bit imo.

Differential Revision: D58172416
  • Loading branch information
joevilches authored and facebook-github-bot committed Jun 5, 2024
1 parent 7c45e58 commit e79507c
Show file tree
Hide file tree
Showing 7 changed files with 156 additions and 22 deletions.
5 changes: 5 additions & 0 deletions gentest/fixtures/YGFlexDirectionTest.html
Original file line number Diff line number Diff line change
Expand Up @@ -400,3 +400,8 @@
<div style="width: 10px;"></div>
</div>
</div>

<div id="flex_direction_alternating_with_percent" style="height: 300px; width: 200px; flex-direction: column;">
<div style="height: 50%; width: 50%; left: 10%; top: 10%; flex-direction: row;">
</div>
</div>
45 changes: 44 additions & 1 deletion java/tests/com/facebook/yoga/YGFlexDirectionTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<4007f83eb3e84f3ee3fcf46d6d7be3bc>>
* @generated SignedSource<<5bb2b698f40c9c95737a77fff84f7700>>
* generated by gentest/gentest-driver.ts from gentest/fixtures/YGFlexDirectionTest.html
*/

Expand Down Expand Up @@ -4242,6 +4242,49 @@ public void test_flex_direction_row_reverse_inner_padding_end() {
assertEquals(100f, root_child0_child2.getLayoutHeight(), 0.0f);
}

@Test
public void test_flex_direction_alternating_with_percent() {
YogaConfig config = YogaConfigFactory.create();

final YogaNode root = createNode(config);
root.setPositionType(YogaPositionType.ABSOLUTE);
root.setWidth(200f);
root.setHeight(300f);

final YogaNode root_child0 = createNode(config);
root_child0.setFlexDirection(YogaFlexDirection.ROW);
root_child0.setPositionPercent(YogaEdge.LEFT, 10f);
root_child0.setPositionPercent(YogaEdge.TOP, 10f);
root_child0.setWidthPercent(50f);
root_child0.setHeightPercent(50f);
root.addChildAt(root_child0, 0);
root.setDirection(YogaDirection.LTR);
root.calculateLayout(YogaConstants.UNDEFINED, YogaConstants.UNDEFINED);

assertEquals(0f, root.getLayoutX(), 0.0f);
assertEquals(0f, root.getLayoutY(), 0.0f);
assertEquals(200f, root.getLayoutWidth(), 0.0f);
assertEquals(300f, root.getLayoutHeight(), 0.0f);

assertEquals(20f, root_child0.getLayoutX(), 0.0f);
assertEquals(30f, root_child0.getLayoutY(), 0.0f);
assertEquals(100f, root_child0.getLayoutWidth(), 0.0f);
assertEquals(150f, root_child0.getLayoutHeight(), 0.0f);

root.setDirection(YogaDirection.RTL);
root.calculateLayout(YogaConstants.UNDEFINED, YogaConstants.UNDEFINED);

assertEquals(0f, root.getLayoutX(), 0.0f);
assertEquals(0f, root.getLayoutY(), 0.0f);
assertEquals(200f, root.getLayoutWidth(), 0.0f);
assertEquals(300f, root.getLayoutHeight(), 0.0f);

assertEquals(120f, root_child0.getLayoutX(), 0.0f);
assertEquals(30f, root_child0.getLayoutY(), 0.0f);
assertEquals(100f, root_child0.getLayoutWidth(), 0.0f);
assertEquals(150f, root_child0.getLayoutHeight(), 0.0f);
}

private YogaNode createNode(YogaConfig config) {
return mNodeFactory.create(config);
}
Expand Down
50 changes: 49 additions & 1 deletion javascript/tests/generated/YGFlexDirectionTest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<61e2e5c148d45c0bbb6bc886991bf3b9>>
* @generated SignedSource<<5aa80de7d8424e35b5b1b205ff44f4cc>>
* generated by gentest/gentest-driver.ts from gentest/fixtures/YGFlexDirectionTest.html
*/

Expand Down Expand Up @@ -4510,3 +4510,51 @@ test('flex_direction_row_reverse_inner_padding_end', () => {
config.free();
}
});
test('flex_direction_alternating_with_percent', () => {
const config = Yoga.Config.create();
let root;

try {
root = Yoga.Node.create(config);
root.setPositionType(PositionType.Absolute);
root.setWidth(200);
root.setHeight(300);

const root_child0 = Yoga.Node.create(config);
root_child0.setFlexDirection(FlexDirection.Row);
root_child0.setPosition(Edge.Left, "10%");
root_child0.setPosition(Edge.Top, "10%");
root_child0.setWidth("50%");
root_child0.setHeight("50%");
root.insertChild(root_child0, 0);
root.calculateLayout(undefined, undefined, Direction.LTR);

expect(root.getComputedLeft()).toBe(0);
expect(root.getComputedTop()).toBe(0);
expect(root.getComputedWidth()).toBe(200);
expect(root.getComputedHeight()).toBe(300);

expect(root_child0.getComputedLeft()).toBe(20);
expect(root_child0.getComputedTop()).toBe(30);
expect(root_child0.getComputedWidth()).toBe(100);
expect(root_child0.getComputedHeight()).toBe(150);

root.calculateLayout(undefined, undefined, Direction.RTL);

expect(root.getComputedLeft()).toBe(0);
expect(root.getComputedTop()).toBe(0);
expect(root.getComputedWidth()).toBe(200);
expect(root.getComputedHeight()).toBe(300);

expect(root_child0.getComputedLeft()).toBe(120);
expect(root_child0.getComputedTop()).toBe(30);
expect(root_child0.getComputedWidth()).toBe(100);
expect(root_child0.getComputedHeight()).toBe(150);
} finally {
if (typeof root !== 'undefined') {
root.freeRecursive();
}

config.free();
}
});
46 changes: 45 additions & 1 deletion tests/generated/YGFlexDirectionTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*
* clang-format off
* @generated SignedSource<<4a6a69c4e9bfda6dc73198791f553e13>>
* @generated SignedSource<<552f1533812daf0793244bbc8c465e17>>
* generated by gentest/gentest-driver.ts from gentest/fixtures/YGFlexDirectionTest.html
*/

Expand Down Expand Up @@ -4283,3 +4283,47 @@ TEST(YogaTest, flex_direction_row_reverse_inner_padding_end) {

YGConfigFree(config);
}

TEST(YogaTest, flex_direction_alternating_with_percent) {
const YGConfigRef config = YGConfigNew();

const YGNodeRef root = YGNodeNewWithConfig(config);
YGNodeStyleSetPositionType(root, YGPositionTypeAbsolute);
YGNodeStyleSetWidth(root, 200);
YGNodeStyleSetHeight(root, 300);

const YGNodeRef root_child0 = YGNodeNewWithConfig(config);
YGNodeStyleSetFlexDirection(root_child0, YGFlexDirectionRow);
YGNodeStyleSetPositionPercent(root_child0, YGEdgeLeft, 10);
YGNodeStyleSetPositionPercent(root_child0, YGEdgeTop, 10);
YGNodeStyleSetWidthPercent(root_child0, 50);
YGNodeStyleSetHeightPercent(root_child0, 50);
YGNodeInsertChild(root, root_child0, 0);
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);

ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root));
ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root));
ASSERT_FLOAT_EQ(200, YGNodeLayoutGetWidth(root));
ASSERT_FLOAT_EQ(300, YGNodeLayoutGetHeight(root));

ASSERT_FLOAT_EQ(20, YGNodeLayoutGetLeft(root_child0));
ASSERT_FLOAT_EQ(30, YGNodeLayoutGetTop(root_child0));
ASSERT_FLOAT_EQ(100, YGNodeLayoutGetWidth(root_child0));
ASSERT_FLOAT_EQ(150, YGNodeLayoutGetHeight(root_child0));

YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionRTL);

ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root));
ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root));
ASSERT_FLOAT_EQ(200, YGNodeLayoutGetWidth(root));
ASSERT_FLOAT_EQ(300, YGNodeLayoutGetHeight(root));

ASSERT_FLOAT_EQ(120, YGNodeLayoutGetLeft(root_child0));
ASSERT_FLOAT_EQ(30, YGNodeLayoutGetTop(root_child0));
ASSERT_FLOAT_EQ(100, YGNodeLayoutGetWidth(root_child0));
ASSERT_FLOAT_EQ(150, YGNodeLayoutGetHeight(root_child0));

YGNodeFreeRecursive(root);

YGConfigFree(config);
}
9 changes: 2 additions & 7 deletions yoga/algorithm/CalculateLayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -545,12 +545,8 @@ static float computeFlexBasisForChildren(
if (performLayout) {
// Set the initial position (relative to the owner).
const Direction childDirection = child->resolveDirection(direction);
const float mainDim =
isRow(mainAxis) ? availableInnerWidth : availableInnerHeight;
const float crossDim =
isRow(mainAxis) ? availableInnerHeight : availableInnerWidth;
child->setPosition(
childDirection, mainDim, crossDim, availableInnerWidth);
childDirection, availableInnerWidth, availableInnerHeight);
}

if (child->style().positionType() == PositionType::Absolute) {
Expand Down Expand Up @@ -2388,8 +2384,7 @@ void calculateLayout(
markerData,
0, // tree root
gCurrentGenerationCount.load(std::memory_order_relaxed))) {
node->setPosition(
node->getLayout().direction(), ownerWidth, ownerHeight, ownerWidth);
node->setPosition(node->getLayout().direction(), ownerWidth, ownerHeight);
roundLayoutResultsToPixelGrid(node, 0.0f, 0.0f);
}

Expand Down
17 changes: 10 additions & 7 deletions yoga/node/Node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,8 @@ float Node::relativePosition(

void Node::setPosition(
const Direction direction,
const float mainSize,
const float crossSize,
const float ownerWidth) {
const float ownerWidth,
const float ownerHeight) {
/* Root nodes should be always layouted as LTR, so we don't return negative
* values. */
const Direction directionRespectingRoot =
Expand All @@ -244,10 +243,14 @@ void Node::setPosition(

// In the case of position static these are just 0. See:
// https://www.w3.org/TR/css-position-3/#valdef-position-static
const float relativePositionMain =
relativePosition(mainAxis, directionRespectingRoot, mainSize);
const float relativePositionCross =
relativePosition(crossAxis, directionRespectingRoot, crossSize);
const float relativePositionMain = relativePosition(
mainAxis,
directionRespectingRoot,
isRow(mainAxis) ? ownerWidth : ownerHeight);
const float relativePositionCross = relativePosition(
crossAxis,
directionRespectingRoot,
isRow(mainAxis) ? ownerHeight : ownerWidth);

const auto mainAxisLeadingEdge = inlineStartEdge(mainAxis, direction);
const auto mainAxisTrailingEdge = inlineEndEdge(mainAxis, direction);
Expand Down
6 changes: 1 addition & 5 deletions yoga/node/Node.h
Original file line number Diff line number Diff line change
Expand Up @@ -229,11 +229,7 @@ class YG_EXPORT Node : public ::YGNode {
void setLayoutBorder(float border, PhysicalEdge edge);
void setLayoutPadding(float padding, PhysicalEdge edge);
void setLayoutPosition(float position, PhysicalEdge edge);
void setPosition(
Direction direction,
float mainSize,
float crossSize,
float ownerWidth);
void setPosition(Direction direction, float ownerWidth, float ownerHeight);

// Other methods
Style::Length resolveFlexBasisPtr() const;
Expand Down

0 comments on commit e79507c

Please sign in to comment.