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

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

Closed
wants to merge 4 commits into from

Conversation

woehrl01
Copy link
Contributor

@woehrl01 woehrl01 commented Mar 12, 2017

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
Copy link
Contributor Author

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
Copy link

rigdern commented Mar 12, 2017

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?

@woehrl01
Copy link
Contributor Author

#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
Copy link
Contributor

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

facebook-github-bot pushed a commit that referenced this pull request Mar 15, 2017
Summary:
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.
Closes #477

Differential Revision: D4697833

Pulled By: emilsjolander

fbshipit-source-id: d081ec7ea559a5f2bd3271c3a4dc272960beddfa
@rigdern
Copy link

rigdern commented Apr 10, 2017

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

@woehrl01
Copy link
Contributor Author

@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
Copy link

rigdern commented Apr 10, 2017

@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
Copy link
Contributor Author

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

facebook-github-bot pushed a commit that referenced this pull request Apr 11, 2017
Summary:
If the root node has a position and we have a RTL layout, that position must be like LTR direction. See #477.
Closes #502

Differential Revision: D4867144

Pulled By: emilsjolander

fbshipit-source-id: b5ad3d87e7054090da12d7665a3d1abe8496a548
vincentriemer pushed a commit to vincentriemer/yoga-dom that referenced this pull request May 9, 2018
Summary:
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.
Closes facebook/yoga#477

Differential Revision: D4697833

Pulled By: emilsjolander

fbshipit-source-id: d081ec7ea559a5f2bd3271c3a4dc272960beddfa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants