Inline flexbox width is wrongly calculated when wrapping vertically

Inline flexbox width is wrongly calculated when wrapping vertically

Inline flexbox width is calculated wrongly when flow of flexbox is
column and flex-wrap property set to wrap or wrap-reverse. The width
is calculated as if inside elements were layed out horizontally. 
This results in extra unnecessary space.

This patch fixes this bug by removing lines that set the 
maxPreferredWidth with the assumptions that children of flex item 
will be layed out horizontally and maxPrefferedWidth is set as 
width of flex-item having maximum contribution. 

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

[harpreet.sk, 2014-05-16 10:55:56]

PTAL

[tony, 2014-05-19 16:14:42]

https://codereview.chromium.org/289903007/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/289903007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlexibleBox.cpp:104: totalChildHeight +=
child->style()->height().value();
Is this correct?  What about min-height?  Also, it seems weird to be using the
height during computation of the preferred width.  Tab, can you clarify if this
is the desired behavior?

https://codereview.chromium.org/289903007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlexibleBox.cpp:128: if (isMultiline() &&
boxMightWrap) {
If boxMightWrap is true, then we assume that all the flex items are placed
horizontally (i.e., wrap after every flex item).  That doesn't seem correct.  It
seems like we need place as many boxes in the column that can fit until we wrap.

[tony, 2014-05-19 16:17:21]

https://codereview.chromium.org/289903007/diff/1/LayoutTests/fast/flexbox/inl...
File
LayoutTests/fast/flexbox/inline-flexbox-wrap-vertically-width-calculation.html
(right):

https://codereview.chromium.org/289903007/diff/1/LayoutTests/fast/flexbox/inl...
LayoutTests/fast/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:9:
<div style="width: 20px; border: 1px solid red;">a</div>
Please include position (data-offset-y and data-offset-x) for the flex items. 
It would also be nice to have some more test cases, like one that wraps after 2
flex items.

[harpreet.sk, 2014-07-18 15:46:49]

Uploaded new solution... Please have a look

https://codereview.chromium.org/289903007/diff/1/LayoutTests/fast/flexbox/inl...
File
LayoutTests/fast/flexbox/inline-flexbox-wrap-vertically-width-calculation.html
(right):

https://codereview.chromium.org/289903007/diff/1/LayoutTests/fast/flexbox/inl...
LayoutTests/fast/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:9:
<div style="width: 20px; border: 1px solid red;">a</div>
On 2014/05/19 16:17:21, tony wrote:
> Please include position (data-offset-y and data-offset-x) for the flex items. 
> It would also be nice to have some more test cases, like one that wraps after
2
> flex items.

Done.

https://codereview.chromium.org/289903007/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/289903007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlexibleBox.cpp:104: totalChildHeight +=
child->style()->height().value();
On 2014/05/19 16:14:43, tony wrote:
> Is this correct?  What about min-height?  Also, it seems weird to be using the
> height during computation of the preferred width.  Tab, can you clarify if
this
> is the desired behavior?

Sorry this is incorrect. It not correct to use height during width computation.
New solution is uploaded.

https://codereview.chromium.org/289903007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlexibleBox.cpp:128: if (isMultiline() &&
boxMightWrap) {
On 2014/05/19 16:14:43, tony wrote:
> If boxMightWrap is true, then we assume that all the flex items are placed
> horizontally (i.e., wrap after every flex item).  That doesn't seem correct. 
It
> seems like we need place as many boxes in the column that can fit until we
wrap.

Done.

[harpreet.sk, 2014-07-21 17:08:54]

ping....

[tony, 2014-07-21 17:50:59]

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
File
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html
(right):

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:11:
-webkit-writing-mode: vertical-lr;
I would use vertical-rl instead.  Neither are very popular, but vertical-rl is
how you would write CJK.

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:31:
<div class="inline-flexbox column align-content-flex-start wrap"
data-expected-width="100">
I think all the flexboxes should have a expected-height too.

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:32:
<div style="width: 100px; height: 20px" data-offset-x="0"
data-offset-y="0"></div>
Looks like all the flex items have a width of 100px.  Can you change that in
some test cases?

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:45:
<div style="width: 300px; height:100px">
What is the height here for?

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:54:
<div style="width: 300px; height:100px">
What is the height here for?

https://codereview.chromium.org/289903007/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/289903007/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1120: } else if (isMultiline() &&
!style()->logicalWidth().isSpecified()) {
I don't think this is correct.  This part of the spec is about how to compute
the intrinsic/preferred sizes.  I don't understand why there would be changes to
the layout method.

I would revert this part of the change and only land the fix in
computeIntrinsicLogicalWidths.  That would make our behavior match that of
Firefox (IE too?) which is the most important part of the change.

[harpreet.sk, 2014-07-22 10:10:23]

Changes has been incorporated... ptal...

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
File
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html
(right):

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:11:
-webkit-writing-mode: vertical-lr;
On 2014/07/21 17:50:59, tony wrote:
> I would use vertical-rl instead.  Neither are very popular, but vertical-rl is
> how you would write CJK.

Done.

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:31:
<div class="inline-flexbox column align-content-flex-start wrap"
data-expected-width="100">
On 2014/07/21 17:50:59, tony wrote:
> I think all the flexboxes should have a expected-height too.

Done.

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:32:
<div style="width: 100px; height: 20px" data-offset-x="0"
data-offset-y="0"></div>
On 2014/07/21 17:50:59, tony wrote:
> Looks like all the flex items have a width of 100px.  Can you change that in
> some test cases?

Done.

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:45:
<div style="width: 300px; height:100px">
On 2014/07/21 17:50:58, tony wrote:
> What is the height here for?

No use of height so removed.

https://codereview.chromium.org/289903007/diff/100001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:54:
<div style="width: 300px; height:100px">
On 2014/07/21 17:50:59, tony wrote:
> What is the height here for?

No use of height so removed.

https://codereview.chromium.org/289903007/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/289903007/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1120: } else if (isMultiline() &&
!style()->logicalWidth().isSpecified()) {
On 2014/07/21 17:50:59, tony wrote:
> I don't think this is correct.  This part of the spec is about how to compute
> the intrinsic/preferred sizes.  I don't understand why there would be changes
to
> the layout method.
> 
> I would revert this part of the change and only land the fix in
> computeIntrinsicLogicalWidths.  That would make our behavior match that of
> Firefox (IE too?) which is the most important part of the change.

Acknowledged.

[tony, 2014-07-22 16:42:03]

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
File
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html
(right):

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:46:
<div class="inline-flexbox column align-content-flex-start wrap" style="width:
50%" data-expected-width="150" data-expected-height="60">
I don't really understand the point of a test with a fixed width.  I thought we
were trying to test the shrink-to-fit behavior, but a fixed width won't test
that.

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:64:
<div style="width: 50px; height: 20px" data-offset-x="230"
data-offset-y="0"></div>
I would add more variety to the heights in some of the vertical-rl tests.

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:78:
<div class="writing-mode-vertical" style="width: 100px; height: 20px"
data-offset-x="180" data-offset-y="0"></div>
Why do you set the writing mode on both the flexbox and the children? 
writing-mode is an inherited property.

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
File LayoutTests/css3/flexbox/multiline-justify-content.html (right):

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/multiline-justify-content.html:1521: 'child3': [40, 10,
40, -10],
Interesting.  I think this is correct (because of align-content: flex-start),
but it's a bit surprising.

[harpreet.sk, 2014-07-23 13:43:24]

Changes has been incorporated... some new test cases has been added including
column-reverse and wrap-reverse... ptal

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
File
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html
(right):

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:46:
<div class="inline-flexbox column align-content-flex-start wrap" style="width:
50%" data-expected-width="150" data-expected-height="60">
On 2014/07/22 16:42:02, tony wrote:
> I don't really understand the point of a test with a fixed width.  I thought
we
> were trying to test the shrink-to-fit behavior, but a fixed width won't test
> that.


This test case was added earlier to check for the working of old solution but
since it has been removed so we can remove this test case.

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:64:
<div style="width: 50px; height: 20px" data-offset-x="230"
data-offset-y="0"></div>
On 2014/07/22 16:42:02, tony wrote:
> I would add more variety to the heights in some of the vertical-rl tests.

Done.

https://codereview.chromium.org/289903007/diff/120001/LayoutTests/css3/flexbo...
LayoutTests/css3/flexbox/inline-flexbox-wrap-vertically-width-calculation.html:78:
<div class="writing-mode-vertical" style="width: 100px; height: 20px"
data-offset-x="180" data-offset-y="0"></div>
On 2014/07/22 16:42:02, tony wrote:
> Why do you set the writing mode on both the flexbox and the children? 
> writing-mode is an inherited property.

Oops! sorry i forget that. This test case has been removed and added some new
test cases.

[tony, 2014-07-23 16:07:41]

LGTM

[harpreet.sk, 2014-07-23 16:15:06]

@leviw: Require owner's lgtm

[esprehn, 2014-07-25 09:24:45]

On 2014/07/23 16:15:06, harpreet.sk wrote:
> @leviw: Require owner's lgtm

rslgtm based on tony' s review.

[harpreet.sk, 2014-07-25 10:08:48]

The CQ bit was checked by harpreet.sk@samsung.com

[commit-bot: I haz the power, 2014-07-25 10:09:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/289903007/140001

[commit-bot: I haz the power, 2014-07-25 11:04:10]

Change committed as 178925