Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

absolute percent position of root element is not correct #1658

Closed
1 task done
robbert-ef opened this issue May 29, 2024 · 3 comments
Closed
1 task done

absolute percent position of root element is not correct #1658

robbert-ef opened this issue May 29, 2024 · 3 comments
Assignees

Comments

@robbert-ef
Copy link

Report

Issues and Steps to Reproduce

YGNodeRef root = YGNodeNew();
YGNodeStyleSetPositionType(root, YGPositionTypeAbsolute);
YGNodeStyleSetPositionPercent(root, YGEdgeLeft, 10);
YGNodeStyleSetPositionPercent(root, YGEdgeTop, 10);
YGNodeStyleSetWidthPercent(root, 80);
YGNodeStyleSetHeightPercent(root, 80);

YGNodeCalculateLayout(root, 300, 200, YGDirectionLTR);
float left = YGNodeLayoutGetLeft(root);
float top = YGNodeLayoutGetTop(root);
float width = YGNodeLayoutGetWidth(root);
float height = YGNodeLayoutGetHeight(root);

Expected Behavior

left = 30
top = 20
width = 240
height = 160

Actual Behavior

left = 20 (not ok, 10% of 300 is 30)
top = 30 (not ok, 10% of 200 is 20)
width = 240 (ok)
height = 160 (ok)

When using the following similar code in the playground, the result is correct:

<Layout>
  <Node style={{width: 300, height: 200 }}>
    <Node style={{ position: "absolute", width: "80%", height: "80%", left: "10%", top: "10%" }} />
  </Node>
</Layout>
@NickGerleman
Copy link
Contributor

NickGerleman commented May 29, 2024

The playground example is giving a root node with explicit dimensions, then calculating layout with infinite free space available, so it makes sense it could give a different answer if this bug is specific to behavior on root node.

Interesting that the result is as if width and height were swapped when calculating inset percentage reference. We’ve had bugs before with getting confused on flex direction, which could maybe cause this.

@joevilches going to assign your way, since you worked on the absolute positioning code recently.

@joevilches
Copy link
Contributor

joevilches commented Jun 5, 2024

@robbert-ef thanks for the report! I was able to verify the problem and it seems to be a bit more general than you described. We get this issue when we have the following:

  • Parent had different flex direction than child
  • Child uses percents for some position (left, right, top, etc)

Neither the position type nor the fact that this is the root matters (actually, when we are not working on the root, absolute position will be correct). Your second example works because the nodes have the same flex direction. The "flex direction" of the root's owner is undefined, but the relevant code seems to assume "row" while the default flex-direction is column. Ill put up a fix here soon

joevilches added a commit to joevilches/react-native that referenced this issue Jun 5, 2024
Summary:
Fixing facebook/yoga#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
joevilches added a commit to joevilches/yoga that referenced this issue Jun 5, 2024
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
joevilches added a commit to joevilches/react-native that referenced this issue Jun 5, 2024
Summary:
X-link: facebook/yoga#1663

Fixing facebook/yoga#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
joevilches added a commit to joevilches/yoga that referenced this issue Jun 5, 2024
…ook#1663)

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
facebook-github-bot pushed a commit to facebook/litho that referenced this issue Jun 11, 2024
Summary:
X-link: facebook/react-native#44792

X-link: facebook/yoga#1663

Fixing facebook/yoga#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.

Changelog: [Internal]

Reviewed By: NickGerleman

Differential Revision: D58172416

fbshipit-source-id: eafd8069e03493fc56c41a76879d1ad9b7e9236d
facebook-github-bot pushed a commit that referenced this issue Jun 11, 2024
Summary:
X-link: facebook/react-native#44792

Pull Request resolved: #1663

Fixing #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.

Changelog: [Internal]

Reviewed By: NickGerleman

Differential Revision: D58172416

fbshipit-source-id: eafd8069e03493fc56c41a76879d1ad9b7e9236d
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 11, 2024
Summary:
Pull Request resolved: #44792

X-link: facebook/yoga#1663

Fixing facebook/yoga#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.

Changelog: [Internal]

Reviewed By: NickGerleman

Differential Revision: D58172416

fbshipit-source-id: eafd8069e03493fc56c41a76879d1ad9b7e9236d
@joevilches
Copy link
Contributor

The attached commit should fix the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants