Fix align-content: center, flex-end alignment with margin #477

2017-03-11T16:13:38-08:00

woehrl01 commented

2017-03-11 16:13:38 -08:00

(Migrated from github.com)

This fixes align-content: center and align-content: flex-end when the child exceeds the parents size. See #476. It also fixes those layouts if the child has margin: auto set.

This fixes ```align-content: center``` and ```align-content: flex-end``` when the child exceeds the parents size. See #476. It also fixes those layouts if the child has ```margin: auto``` set.

woehrl01 commented

2017-03-12 09:48:59 -07:00

(Migrated from github.com)

I am not 100% certain with this change, as there isn't any failing test after removing those lines. According to git those lines have been added by @rigdern . Do you have an example layout where this line are required?

After looking deeper into this, and trying to add failing tests, I found that it fails even without margin set, and also has an addional bug there with margin: auto. I'm now a lot more certain that this is a valid fix.

> I am not 100% certain with this change, as there isn't any failing test after removing those lines. According to git those lines have been added by @rigdern . Do you have an example layout where this line are required? After looking deeper into this, and trying to add failing tests, I found that it fails even without margin set, and also has an addional bug there with ```margin: auto```. I'm now a lot more certain that this is a valid fix.

rigdern commented

2017-03-12 13:13:49 -07:00

(Migrated from github.com)

I am not 100% certain with this change, as there isn't any failing test after removing those lines. According to git those lines have been added by @rigdern . Do you have an example layout where this line are required?

@woehrl01 When you say "removing those lines", I believe you are referring to:

if (measureModeCrossDim == YGMeasureModeAtMost) {
  containerCrossAxis = fminf(containerCrossAxis, availableInnerCrossDim);
}

I don't have a specific example available. It looks like those lines were added in #185 which involved significant changes to the layout engine in order to make it better match the W3C flexbox spec.

#476 didn't repro in the version of the layout engine that is included in React Native 0.32.0-rc.0. So it seems those lines didn't break this scenario back then.

If you delete those lines, what prevents the calculated cross size from exceeding the available cross size when running in the at-most measure mode?

> I am not 100% certain with this change, as there isn't any failing test after removing those lines. According to git those lines have been added by @rigdern . Do you have an example layout where this line are required? @woehrl01 When you say "removing those lines", I believe you are referring to: ``` if (measureModeCrossDim == YGMeasureModeAtMost) { containerCrossAxis = fminf(containerCrossAxis, availableInnerCrossDim); } ``` I don't have a specific example available. It looks like those lines were added in #185 which involved significant changes to the layout engine in order to make it better match the [W3C flexbox spec](https://www.w3.org/TR/css-flexbox-1/). #476 didn't repro in the version of the layout engine that is included in React Native 0.32.0-rc.0. So it seems those lines didn't break this scenario back then. If you delete those lines, what prevents the calculated cross size from exceeding the available cross size when running in the at-most measure mode?

woehrl01 commented

2017-03-12 14:25:08 -07:00

(Migrated from github.com)

#476 didn't repro in the version of the layout engine that is included in React Native 0.32.0-rc.0. So it seems those lines didn't break this scenario back then.

That's strange.

If you delete those lines, what prevents the calculated cross size from exceeding the available cross size when running in the at-most measure mode?

Nothing prevents it. But as the resulting value is only used for the alignItem/margin: auto step, this value shouldn't be clipped, as the child is still bigger then AT_MOST wants it.

I just want to share my findings, as I'm still not that convinced. But I also don't understand why that value should be clipped either.

> #476 didn't repro in the version of the layout engine that is included in React Native 0.32.0-rc.0. So it seems those lines didn't break this scenario back then. That's strange. > If you delete those lines, what prevents the calculated cross size from exceeding the available cross size when running in the at-most measure mode? Nothing prevents it. But as the resulting value is only used for the ```alignItem```/```margin: auto``` step, this value shouldn't be clipped, as the child is still bigger then ```AT_MOST``` wants it. I just want to share my findings, as I'm still not that convinced. But I also don't understand why that value should be clipped either.

facebook-github-bot commented

2017-03-13 06:01:31 -07:00

(Migrated from github.com)

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D4697833).

rigdern commented

2017-04-10 13:39:46 -07:00

(Migrated from github.com)

@woehrl01 The test case I listed in #476 is still laid out incorrectly in master.

woehrl01 commented

2017-04-10 13:48:14 -07:00

(Migrated from github.com)

@rigdern that's strange, as I added a test case with exactly your values, which doesn't fail in the master. Could you please verify if I correctly adapted your failing layout?

@rigdern that's strange, as I added [a test case with exactly your values](https://github.com/facebook/yoga/blob/master/gentest/fixtures/YGAlignItemsTest.html#L147-L151), which doesn't fail in the master. Could you please verify if I correctly adapted your failing layout?

rigdern commented

2017-04-10 13:56:14 -07:00

(Migrated from github.com)

@woehrl01 The test case I reported used a position: absolute element on the root with a non-zero value for left. Here's the diff: https://www.diffchecker.com/BD9BgS4m

@woehrl01 The test case I reported used a `position: absolute` element on the root with a non-zero value for `left`. Here's the diff: https://www.diffchecker.com/BD9BgS4m

woehrl01 commented

2017-04-10 14:20:45 -07:00

(Migrated from github.com)

@rigdern Oh, thanks, I see! It's not the root node which isn't laid out correctly, I'll have a look. 🔨

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#477