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

[Yoga 3] Potential RTL paddingStart/paddingEnd bug #1657

Closed
Omxjep3434 opened this issue May 25, 2024 · 4 comments
Closed

[Yoga 3] Potential RTL paddingStart/paddingEnd bug #1657

Omxjep3434 opened this issue May 25, 2024 · 4 comments
Assignees

Comments

@Omxjep3434
Copy link

Omxjep3434 commented May 25, 2024

Report

I see a potential bug related to the RTL layout direction, and paddingStart / paddingEnd values when computing the % width of child nodes. Please see the code snippet below.

I am testing with the Javascript npm package version 3.0.4, but I think this bug may have been introduced in Yoga 3.0.2.

Also, while my code snippet demonstrates an issue with padding values, I found that the same issue exists for border width values.

Issues and Steps to Reproduce

function rtlPercentWidthIssue(yoga) {
    function printNode(name, node) {
      console.log(`${name} - width: ${node.getComputedWidth()}, height: ${node.getComputedHeight()}, paddingLeft: ${node.getComputedPadding(Edge.Left)}, paddingRight: ${node.getComputedPadding(Edge.Right)}`);
    }

    const config = yoga.Config.create();
    config.setPointScaleFactor(0);

    const root = yoga.Node.createWithConfig(config);
    root.setWidth(10);
    root.setHeight(10);
    root.setPadding(Edge.Left, 2);
    root.setPadding(Edge.Start, 5);
    root.setPadding(Edge.End, NaN);

    const child = yoga.Node.createWithConfig(config);
    child.setWidth("100%");
    child.setHeight(1);

    root.insertChild(child, 0);

    // In LTR - paddingLeft = 2, paddingStart = 5. paddingStart should override paddingLeft.
    root.calculateLayout(20, 20, Direction.LTR);
    printNode("root", root);
    printNode("child", child);
    // Everything is correct here.
    // root - width: 10, height: 10, paddingLeft: 5, paddingRight: 0
    // child - width: 5, height: 1, paddingLeft: 0, paddingRight: 0

    // In RTL - paddingLeft = 2, paddingEnd = 5. paddingEnd should now override paddingLeft.
    root.setPadding(Edge.Start, NaN);
    root.setPadding(Edge.End, 5);
    root.calculateLayout(20, 20, Direction.RTL);
    printNode("root", root);
    printNode("child", child);
    // The computed padding values are correct, but notice the child width is no longer correct. It seems the % size calculation is treating paddingEnd as paddingRight, even though the layout direction is RTL.
    // root - width: 10, height: 10, paddingLeft: 5, paddingRight: 0
    // child - width: 3, height: 1, paddingLeft: 0, paddingRight: 0
  }

function execute() {
  const yoga3 = await loadYoga();
  rtlPercentWidthIssue(yoga3);
}

Expected Behavior

The child width should be '5' in both the LTR and RTL scenarios. See comments in the code snippet above. I have commented the console output and put a description of the problem.

My guess at the issue

It seems there is a bug when computing the width of a child in regard to the parent's paddingStart/paddingEnd values, in combination with the RTL layout direction. In reference to my code snippet, it seems the layout engine is treating paddingEnd as paddingRight when RTL (which is incorrect) when computing the % width of a child node. However, this bug doesn't appear in the actual computed padding values, which are correct.

@NickGerleman
Copy link
Contributor

FYI @joevilches for when you’re back from PTO

@joevilches
Copy link
Contributor

@Omxjep3434 thanks for the report! I was able to repro and am able to see all the things you described in the report. It looks like the padding is properly applied but the width is the part that is incorrect and is taking both paddings into account, likely like you said in your guess where we interpret the end as the right side for that part. I will take a look and see if I can fix it

joevilches added a commit to joevilches/yoga that referenced this issue Jun 5, 2024
…ng defined on parent

Summary:
This should fix facebook#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
joevilches added a commit to joevilches/react-native that referenced this issue Jun 5, 2024
…ng defined on parent

Summary:
This should fix facebook/yoga#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
joevilches added a commit to joevilches/react-native that referenced this issue Jun 5, 2024
…ng defined on parent

Summary:
X-link: facebook/yoga#1662

This should fix facebook/yoga#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
joevilches added a commit to joevilches/yoga that referenced this issue Jun 5, 2024
…ng defined on parent (facebook#1662)

Summary:

This should fix facebook#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
@joevilches
Copy link
Contributor

Ok that attached commit should fix the problem! We may wanna surround in errata, but this seems to be a super specific case where we need RTL, % child, and physical + relative edge defined for this to show up, so could go without it. We will see in code review.

joevilches added a commit to joevilches/react-native that referenced this issue Jun 5, 2024
…ng defined on parent (facebook#44791)

Summary:

X-link: facebook/yoga#1662

This should fix facebook/yoga#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
joevilches added a commit to joevilches/yoga that referenced this issue Jun 5, 2024
…ng defined on parent (facebook#1662)

Summary:
X-link: facebook/react-native#44791


This should fix facebook#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
joevilches added a commit to joevilches/react-native that referenced this issue Jun 5, 2024
…tive padding defined on parent (facebook#44791)

Summary:

X-link: facebook/yoga#1662

This should fix facebook/yoga#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
joevilches added a commit to joevilches/yoga that referenced this issue Jun 5, 2024
…ng defined on parent (facebook#1662)

Summary:
X-link: facebook/react-native#44791


This should fix facebook#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
@Omxjep3434
Copy link
Author

Awesome, glad to help!

joevilches added a commit to joevilches/react-native that referenced this issue Jun 7, 2024
…ng defined on parent (facebook#44791)

Summary:
Pull Request resolved: facebook#44791

X-link: facebook/yoga#1662

This should fix facebook/yoga#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
joevilches added a commit to joevilches/yoga that referenced this issue Jun 7, 2024
…ng defined on parent (facebook#1662)

Summary:
X-link: facebook/react-native#44791

Pull Request resolved: facebook#1662

This should fix facebook#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
facebook-github-bot pushed a commit that referenced this issue Jun 11, 2024
…ng defined on parent (#1662)

Summary:
X-link: facebook/react-native#44791

Pull Request resolved: #1662

This should fix #1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Changelog: [Internal]

Reviewed By: NickGerleman

Differential Revision: D58169843

fbshipit-source-id: 5b4854dddc019285076bd06955557edf73ef7ec5
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 11, 2024
…ng defined on parent (#44791)

Summary:
Pull Request resolved: #44791

X-link: facebook/yoga#1662

This should fix facebook/yoga#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Changelog: [Internal]

Reviewed By: NickGerleman

Differential Revision: D58169843

fbshipit-source-id: 5b4854dddc019285076bd06955557edf73ef7ec5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment