Fix wrong min-width percentage calculation for child #1015

2020-06-11T08:02:58-07:00

woehrl01 commented

2020-06-11 08:02:58 -07:00

(Migrated from github.com)

This PR fixes #872 by using the correct parent values instead of the grandparents.

Unfortunately that fix reveals a bug which causes two existing tests to fail, which are false negatives.

percentage_flex_basis_cross_min_height
percentage_flex_basis_main_min_width

The underlying bug is a misbehavior from the specs (https://www.w3.org/TR/css-flexbox-1/#algo-main-item).

When determining the flex base size, the item’s min and max main sizes are ignored (no clamping occurs).

Sadly the current algorithm takes the clamping into account. As the generated tests also have changed based on using the latest (83) chrome, it also looks that this has been a previous bug in chrome, too.

Despite the fact this PR is still valid, it just reveals another existing bug. But as Yoga does not apply to the web basis specs all together I suggest having this fix in place instead of using the wrong values for calculation. As the underlying bug (including min values for flex basis) is more predictable.

I also moved one of the tests to a different file to not get overridden by the generator.

@SidharthGuglani I highly appreciate your opinion on that one.
@emilsjolander I know it's a long time, maybe you have an idea here, too. Perhaps I also have an oversight here.

This PR fixes #872 by using the correct parent values instead of the grandparents. Unfortunately that fix reveals a bug which causes two existing tests to fail, which are false negatives. * `percentage_flex_basis_cross_min_height` * `percentage_flex_basis_main_min_width` The underlying bug is a misbehavior from the specs (https://www.w3.org/TR/css-flexbox-1/#algo-main-item). > When determining the flex base size, the item’s min and max main sizes are ignored (no clamping occurs). Sadly the current algorithm takes the clamping into account. As the generated tests also have changed based on using the latest (83) chrome, it also looks that this has been a previous bug in chrome, too. Despite the fact this PR is still valid, it just reveals another existing bug. But as Yoga does not apply to the web basis specs all together I suggest having this fix in place instead of using the wrong values for calculation. As the underlying bug (including min values for flex basis) is more predictable. I also moved one of the tests to a different file to not get overridden by the generator. @SidharthGuglani I highly appreciate your opinion on that one. @emilsjolander I know it's a long time, maybe you have an idea here, too. Perhaps I also have an oversight here.

devsnek commented

2021-05-21 14:43:17 -07:00

(Migrated from github.com)

Any chance of this being merged?

NickGerleman commented

2022-10-03 17:29:04 -07:00

(Migrated from github.com)

Ooh this explains the test failures I was seeing.

I do think we will want to fix this, but behind a flag. For React Native, we are planning on doing a push for layout conformance, where we know there will be enough changes to require apps to migrate themselves.

Ooh this explains the test failures I was seeing. I do think we will want to fix this, but behind a flag. For React Native, we are planning on doing a push for layout conformance, where we know there will be enough changes to require apps to migrate themselves.

This pull request has changes conflicting with the target branch.

gentest/fixtures/YGPercentageTest.html
tests/YGMeasureTest.cpp
tests/YGPercentageTest.cpp
yoga/Yoga.cpp

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin woehrl01/fix-child-percent:woehrl01/fix-child-percent

git checkout woehrl01/fix-child-percent

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1015