Don't stretch margin: auto items in a vertical flexbox

Don't stretch margin: auto items in a vertical flexbox

Even with align-self: stretch (the default), we shouldn't stretch items
that have margin: auto;

BUG=242654
R=ojan@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151314

[ojan, 2013-05-24 01:21:57]

https://codereview.chromium.org/15946003/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/15946003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:2228: if
(!parent()->style()->isColumnFlexDirection() || parent()->style()->flexWrap() !=
FlexNoWrap)
It's not obvious to me that this new code is right for flex-wrapping. Mind
adding a wrapping + margin:auto test case as well?

[cbiesinger, 2013-05-24 01:27:57]

On 2013/05/24 01:21:57, ojan wrote:
>
https://codereview.chromium.org/15946003/diff/1/Source/core/rendering/RenderB...
> File Source/core/rendering/RenderBox.cpp (right):
> 
>
https://codereview.chromium.org/15946003/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBox.cpp:2228: if
> (!parent()->style()->isColumnFlexDirection() || parent()->style()->flexWrap()
!=
> FlexNoWrap)
> It's not obvious to me that this new code is right for flex-wrapping. Mind
> adding a wrapping + margin:auto test case as well?

I think the code is right (due to the earlier return true for vertical flexbox),
but I'll add a testcase for that + for vertical writing modes

[cbiesinger, 2013-05-24 21:52:59]

On 2013/05/24 01:27:57, cbiesinger wrote:
> On 2013/05/24 01:21:57, ojan wrote:
> >
>
https://codereview.chromium.org/15946003/diff/1/Source/core/rendering/RenderB...
> > File Source/core/rendering/RenderBox.cpp (right):
> > 
> >
>
https://codereview.chromium.org/15946003/diff/1/Source/core/rendering/RenderB...
> > Source/core/rendering/RenderBox.cpp:2228: if
> > (!parent()->style()->isColumnFlexDirection() ||
parent()->style()->flexWrap()
> !=
> > FlexNoWrap)
> > It's not obvious to me that this new code is right for flex-wrapping. Mind
> > adding a wrapping + margin:auto test case as well?
> 
> I think the code is right (due to the earlier return true for vertical
flexbox),
> but I'll add a testcase for that + for vertical writing modes

I added a testcase for vertical writing modes (and fixed the code to support
them). For wrapped flexboxes there's a separate bug (also in trunk), is it OK to
do that in a later CL or do you want me to fix it in here?

[ojan, 2013-05-24 22:39:09]

lgtm

Fixing the other bug in a separate patch makes sense.

https://codereview.chromium.org/15946003/diff/5001/Source/core/rendering/Rend...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/15946003/diff/5001/Source/core/rendering/Rend...
Source/core/rendering/RenderBox.cpp:2185: // widths, so we don't have to check
marginBefore/marginAfter.
Instead of adding (or maybe in addition to) the extra "Note" section, how about
adding an ASSERT(parent()->style()->isColumnFlexDirection()); and renaming this
function to columnFlexItemHasStretchAlignment?

[commit-bot: I haz the power, 2013-05-24 23:49:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/15946003/12001

[commit-bot: I haz the power, 2013-05-25 00:46:33]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-28 17:15:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/15946003/12001

[commit-bot: I haz the power, 2013-05-28 18:32:41]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[cbiesinger, 2013-05-28 22:06:43]

Committed patchset #3 manually as r151314 (presubmit successful).