fix wraping calculation if min-size constraint exists

[woehrl01]

Fixes #261

[facebook-github-bot]

@woehrl01 updated the pull request - view changes

[facebook-github-bot]

@woehrl01 updated the pull request - view changes

[woehrl01]

this also fixes, that you have to take padding and border into account on space distribution.

[facebook-github-bot]

@woehrl01 updated the pull request - view changes

[emilsjolander]

Looks good. I'll merge it this week. Thanks!

[facebook-github-bot]

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

[facebook-github-bot]

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

[emilsjolander]

When importing this I noticed it broke some internal product tests. So I investigated and boiled it down to the following tests which breaks when applying this diff.

<div style="width: 200px; height: 200px; justify-content: flex-end;">
  <div style="width: 100px; height: 100px; padding: 20px;"></div>
</div>

I'll add this test to the test suite

[woehrl01]

Thanks! I will have a look!

[facebook-github-bot]

@woehrl01 updated the pull request - view changes - changes since last import

[woehrl01]

puh 😄 finally got the logic right.
It seams there is a difference if the container is flex-wrap: wrap or not. So I added a bunch of test cases. But all in all they are running fine now.

Do you have additional test cases in your internel test suite which are failing with these changes?

[facebook-github-bot]

@woehrl01 updated the pull request - view changes - changes since last import

[emilsjolander]

I've re-imported and rebased this pull request. All looks good as far as tests go. I'm not quite understanding the change though so I would love it if you could explain what is going on here. Why are we only including the padding of a child in its size calculation if flex is set to wrap?

[woehrl01]

@emilsjolander I'm not sure why this is. I couldn't find something like this in the spec. My theory is, that under chrome box-sizing: border-box is not correctly applied if flex wrapping is enabled. I changed test-template.html to use box-sizing: content-box and the same expected values got generated, which should not, shouldn't it? I'm torn back and forth with this change, it is required to make the tests pass against the chrome implementation, but I don't fully understand the switch on flex-wrap either. I think it is required to be confident about the output (if you use chrome as reference) and to make some gentests pass, which wouldn't otherwise. I haven't checked the output with other browsers yet. Should we hide this behind a feature flag, how about tests then?

Please don't get me wrong, I'm not the one who likes to say "there must be a bug in Xyz", but I currently have no other explanation at hand.

[emilsjolander]

@woehrl01 lol css has a bunch of unexplainable things, just wondering your thoughts behind this. Thanks! It would be awesome if you could check the output in other browsers. I've previously had issues with flex-wrap being implemented slightly differently in different browsers causing different behavior in all major browsers (yuck). Regarding feature flag and tests I recently added support for feature flags in generated tests 👍 Not saying we should do this but in case we do there is support for it (pixel rounding uses this).

[emilsjolander]

@woehrl01 did you get a chance to check out other browsers?

[woehrl01]

@emilsjolander I looked at the output on Edge 38, Firefox 50 and Chrome 54. They are all producing the same output. So I think that my theory is wrong. But I have another theory, based on a deeper look on the generated values without this fix. I think that my changes are a workaround for a bug inside the library, where under the condition of no wrap the values are added a second time later in the algorithm. I think the current "fix" is enough at the current time. When I have a little more time, I'll take the spec and will have a drill down on where the possible bug is hiding. What do you think?

[emilsjolander]

@woehrl01 sounds good. I might take a look at it as well. Thanks!

[woehrl01]

@emilsjolander what's the/your current state of this PR?

[emilsjolander]

@woehrl01 it's semi-forgotten tbh. I have not had time to look into it much. I will try to revive it next week.