flexbox: incorrect flexbox height with overflow:auto on flex item

flexbox: incorrect flexbox height with overflow:auto on flex item

Flexbox with flex-direction: row/row-reverse and with
overflow:auto on flex item shows incorrect flexbox height.
The flexbox seems to use the height from before
adding the horizontal scrollbar.
    
This bug is occuring because we are not taking into account
the horizontal scrollbar height of flex-item while computing
the childCrossAxisMarginBoxExtent. This patch fixes this bug
by adding scrollbarLogicalHeight while calculating childCrossAxisMarginBoxExtent.


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

[harpreet.sk, 2014-08-04 09:03:03]

ptal...

[harpreet.sk, 2014-08-05 15:33:47]

ptal..

[cbiesinger, 2014-08-06 02:52:49]

https://codereview.chromium.org/439063002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/439063002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1114: childCrossAxisMarginBoxExtent
= maxAscent + maxDescent + child->scrollbarLogicalHeight();
Doesn't this have to be scrollbarLogicalWidth for column flexboxes? (can you add
a test for column flexboxes?)

[harpreet.sk, 2014-08-06 13:20:23]

Incorporated the changes asked. Please take a look.

https://codereview.chromium.org/439063002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/439063002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1114: childCrossAxisMarginBoxExtent
= maxAscent + maxDescent + child->scrollbarLogicalHeight();
On 2014/08/06 02:52:49, cbiesinger wrote:
> Doesn't this have to be scrollbarLogicalWidth for column flexboxes? (can you
add
> a test for column flexboxes?)

@Chris: Thanks for the comment. Yes it has to be scrollbarLogicalWidth for
column flexboxes although childCrossAxisMarginBoxExtent will only been used to
setLogicalHeight when flex-direction:row.

Actually the bug is only for the case of flex-direction: row. For column case
behaviour is correct and is similar to Mozilla. I have added the column test in
the new patch.

[tony, 2014-08-06 16:43:09]

Please add some vertical-rl test cases.

[tony, 2014-08-06 16:47:22]

https://codereview.chromium.org/439063002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/439063002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1115: childCrossAxisMarginBoxExtent
= maxAscent + maxDescent + crossAxisScrollbarExtentForChild;
I don't see any align-item baseline test cases that go through this code path. 
Please add some.

[cbiesinger, 2014-08-07 02:57:01]

https://codereview.chromium.org/439063002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/439063002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1107: LayoutUnit
crossAxisScrollbarExtentForChild = !isColumnFlow() ?
child->scrollbarLogicalHeight() : child->instrinsicScrollbarLogicalWidth();
I'd really prefer you to flip the ?: around so you don't need the negation here.

isColumnFlow() ? child->instrinsicScrollbarLogicalWidth() :
child->scrollbarLogicalHeight()

But why not follow the example from the crossAxis* functions and write this
using isHorizontalFlow and the non-logical height/width?

[harpreet.sk, 2014-08-07 12:46:39]

Uploaded a new solution. In the new solution we are not adding 
crossAxisScrollbarExtentForChild for baseline cases
as while computing descent it takes crossAxisExtentForChild which will include
the scrollbar height.

I have added the test cases of align-item baseline just to make sure that they
are working fine. 

Also added vertical-rl testcases.

https://codereview.chromium.org/439063002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/439063002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1107: LayoutUnit
crossAxisScrollbarExtentForChild = !isColumnFlow() ?
child->scrollbarLogicalHeight() : child->instrinsicScrollbarLogicalWidth();
On 2014/08/07 02:57:01, cbiesinger wrote:
> I'd really prefer you to flip the ?: around so you don't need the negation
here.
> 
> isColumnFlow() ? child->instrinsicScrollbarLogicalWidth() :
> child->scrollbarLogicalHeight()
> 
> But why not follow the example from the crossAxis* functions and write this
> using isHorizontalFlow and the non-logical height/width?


Modified the solution to use isHorizontalFlow and the non-logical height/width

https://codereview.chromium.org/439063002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1115: childCrossAxisMarginBoxExtent
= maxAscent + maxDescent + crossAxisScrollbarExtentForChild;
On 2014/08/06 16:47:22, tony wrote:
> I don't see any align-item baseline test cases that go through this code path.

> Please add some.

Actually we should not add crossAxisScrollbarExtentForChild for baseline cases
as while computing descent it takes crossAxisExtentForChild which will include
the scrollbar height.

I added the test cases of align-item baseline just to make sure that they are
working fine.

[tony, 2014-08-07 16:42:31]

LGTM, but cbiesinger may want to take a look too.

https://codereview.chromium.org/439063002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/439063002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1116: LayoutUnit
crossAxisScrollbarExtentForChild = isHorizontalFlow() ?
child->horizontalScrollbarHeight() : child->verticalScrollbarWidth();
Nit: I would pull this out into a function to be consistent with the similar
functions.

[cbiesinger, 2014-08-08 03:47:29]

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html (right):

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html:6: border:5px
solid green;
nit: please consistently add a space after the colon

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html:28: <div
class="flexbox" data-expected-height="47">
I must be missing something. I don't understand why this has an expected height
of 47. Shouldn't it be 22 because that's what the inner div's height is, and
nothing else affects the height? That's what I Get when I paste this test into
jsbin, but clearly this test passes, so I must be missing something...

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html:29: <div
class="overflow"><div style="width:100px; height:20px"></div></div>
also in these attributes

[cbiesinger, 2014-08-08 03:56:39]

lgtm if you make tony's and my suggested changes

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html (right):

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html:28: <div
class="flexbox" data-expected-height="47">
On 2014/08/08 03:47:29, cbiesinger wrote:
> I must be missing something. I don't understand why this has an expected
height
> of 47. Shouldn't it be 22 because that's what the inner div's height is, and
> nothing else affects the height? That's what I Get when I paste this test into
> jsbin, but clearly this test passes, so I must be missing something...

I must've done something wrong in my test. it's 47 mostly because of the
scrollbar.

[harpreet.sk, 2014-08-08 05:35:04]

Incorporated the changes asked...

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html (right):

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html:6: border:5px
solid green;
On 2014/08/08 03:47:29, cbiesinger wrote:
> nit: please consistently add a space after the colon

Done.

https://codereview.chromium.org/439063002/diff/80001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html:29: <div
class="overflow"><div style="width:100px; height:20px"></div></div>
On 2014/08/08 03:47:29, cbiesinger wrote:
> also in these attributes

Done.

https://codereview.chromium.org/439063002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/439063002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1116: LayoutUnit
crossAxisScrollbarExtentForChild = isHorizontalFlow() ?
child->horizontalScrollbarHeight() : child->verticalScrollbarWidth();
On 2014/08/07 16:42:30, tony wrote:
> Nit: I would pull this out into a function to be consistent with the similar
> functions.

Done.

[leviw_travelin_and_unemployed, 2014-08-08 17:58:27]

lgtm

[commit-bot: I haz the power, 2014-08-08 17:59:04]

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

[commit-bot: I haz the power, 2014-08-08 19:46:36]

Change committed as 179861