facebook / yoga

Yoga is an embeddable layout engine targeting web standards.
https://yogalayout.dev/
MIT License
17.25k stars 1.42k forks source link

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

Closed Omxjep3434 closed 3 months ago

Omxjep3434 commented 4 months ago

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 commented 4 months ago

FYI @joevilches for when you’re back from PTO

joevilches commented 4 months ago

@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 commented 4 months ago

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.

Omxjep3434 commented 4 months ago

Awesome, glad to help!