Blink doesn't honor percent heights on children of "align-self:stretch" flex items in a fixed-height

Blink doesn't honor percent heights on children of "align-self:stretch" flex items in a fixed-height flex container
    
Blink does not honor percent heights on children of flex items
having align-self:stretch in a fixed-height flex container. It
ignores the height specified in percent and take the height as
min-content height.
    
This bug occurs because while applying stretch alignment to the flex-item
we set the OverrideLogicalContentHeight for the flex-item but while
computing PercentageLogicalHeight for the children of the flex-item
in the api RenderBox::computePercentageLogicalHeight we are checking for
hasOverrideContainingBlockLogicalHeight() which checks if the child which
is having percentage height has overrideLogicalHeight or not. Since for
the child we had not set overideLogicalHeight so this condtion returns
false and available height for child becomes -1. Hence it takes height
as min-content height.
    
This patch fixes this bug by using overrideLogicalContentHeight() of 
containing block as availableHeight while computing the 
percentage heights of children of flex item having stretch alignment. 
Also for the cases where desiredLogicalHeight == logicalHeight() and 
flex item hasPercentHeightDescendants() we are forcing a relayout
of the flex item as overrideLogicalContentHeight() 
gets cleared off while calculating preferredMainAxisContentExtentForChild
    
BUG=341310, 346275
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182735

[harpreet.sk, 2014-06-16 15:21:02]

Please have a look. I will add the layout test for the same in next patch set.

[tony, 2014-06-16 16:45:54]

Where in the spec does it say we should skip the flexbox height when resolving
percentage heights for flex items?  What does IE do?

Please include new tests with the change.  E.g., what should happen if the
container does not have height: auto.

[cbiesinger, 2014-06-17 01:50:07]

I am fairly sure this change is incorrect, because the real bug is described in
https://code.google.com/p/chromium/issues/detail?id=341310

[cbiesinger, 2014-06-17 01:50:32]

not lgtm

[harpreet.sk, 2014-06-18 14:04:59]

On 2014/06/17 01:50:07, cbiesinger wrote:
> I am fairly sure this change is incorrect, because the real bug is described
in
> https://code.google.com/p/chromium/issues/detail?id=341310

I have uploaded a new patch based on the original bug=341310. Added layout test
for the same and fixed the existing layout test which are getting failed because
of it. The title and description has been updated. Please take a look.

[tony, 2014-06-18 17:53:18]

This didn't look right, so I ran it through the try bots.  Looks like this
breaks all the grid layout tests.

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

https://codereview.chromium.org/331203002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2678: availableHeight =
cb->overrideContainingBlockContentLogicalHeight();
Why remove the previous check?  Shouldn't this be an additional check?

[arpitabahuguna, 2014-06-19 08:49:56]

The CQ bit was checked by a.bah@chromium.org

[arpitabahuguna, 2014-06-19 08:50:48]

The CQ bit was unchecked by a.bah@chromium.org

[harpreet.sk, 2014-06-19 12:22:39]

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

https://codereview.chromium.org/331203002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2678: availableHeight =
cb->overrideContainingBlockContentLogicalHeight();
On 2014/06/18 17:53:18, tony wrote:
> Why remove the previous check?  Shouldn't this be an additional check?

You are right. I didn't have to remove this check. Actually this check was used
in RenderGrid Layout. In RenderGrid::logicalHeightForChild they are setting
OverrideContainingBlockContentLogicalHeight to be -1 for children having
percentage logical height so that they do not override their intrinsic height.
Then while resolving percentage height they check for
hasOverrideContainingBlockLogicalHeight() whhich return true and set available
heightt to be -1. This is the reason why with this change all grid layout tests
were getting failed. 

I now had added my check as an additional check.

[harpreet.sk, 2014-06-19 12:24:11]

On 2014/06/18 17:53:18, tony wrote:
> This didn't look right, so I ran it through the try bots.  Looks like this
> breaks all the grid layout tests.
> 

Thanks tony for your comment. I have uploaded a new patch set that will not
break the grid layout test. Please have a look.

[tony, 2014-06-19 16:37:47]

https://codereview.chromium.org/331203002/diff/60001/LayoutTests/css3/flexbox...
File
LayoutTests/css3/flexbox/percent-height-children-of-alignSelf-stretch-flex-item.html
(right):

https://codereview.chromium.org/331203002/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/percent-height-children-of-alignSelf-stretch-flex-item.html:27:
<div data-expected-height="70" style="height: 70%;
background-color:lime">Horizontal Writing Mode</div>
Please add tests with padding and borders.  Also add a test case that changes
the height of the grandchild in JS.

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

https://codereview.chromium.org/331203002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1381:
child->clearOverrideContainingBlockContentLogicalHeight();
Why do we clear the value?  If we need to relayout the grandchild, will it
relayout properly?  E.g., if we change the height of the grandchild to a
different percent value, will it be able to compute itself properly?

[harpreet.sk, 2014-06-20 14:54:32]

Please have a look.

https://codereview.chromium.org/331203002/diff/60001/LayoutTests/css3/flexbox...
File
LayoutTests/css3/flexbox/percent-height-children-of-alignSelf-stretch-flex-item.html
(right):

https://codereview.chromium.org/331203002/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/percent-height-children-of-alignSelf-stretch-flex-item.html:27:
<div data-expected-height="70" style="height: 70%;
background-color:lime">Horizontal Writing Mode</div>
On 2014/06/19 16:37:46, tony wrote:
> Please add tests with padding and borders.  Also add a test case that changes
> the height of the grandchild in JS.

Done.

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

https://codereview.chromium.org/331203002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1381:
child->clearOverrideContainingBlockContentLogicalHeight();
On 2014/06/19 16:37:46, tony wrote:
> Why do we clear the value?  If we need to relayout the grandchild, will it
> relayout properly?  E.g., if we change the height of the grandchild to a
> different percent value, will it be able to compute itself properly?


I clear the value because consider the case below:

<div class="flexbox" style="height:50px">
  <img id="green1" data-expected-height="25" style="max-height:50%"
src="../images/resources/green-10.png">
</div>

In this case when it enters the applyStretchAlignmentToChild first time it sets
the OverrideContainingBlockContentLogicalHeight to be 25. When it enters the
second time  and computes desired logical height it computes max-height which is
50% of OverrideContainingBlockContentLogicalHeight which we have set and which
is 25 and made the height of image equal to 12.5 which is wrong.

Since this case was failing so i thought to
clearOverideContainingBlockContentLogicalHeight.

Now i have find a better solution in which we will set
OverrideContainingBlockContentLogicalHeight equal to stretchedLogicalHeight -
child->borderAndPaddingLogicalHeight() and not desiredLogicalHeight
and so we dont need to clear the value. All cases are working fine with the new
solution.

[tony, 2014-06-23 16:57:53]

This LGTM, but I am not an OWNER.  I started a job on the trybots.

[leviw_travelin_and_unemployed, 2014-06-23 17:09:23]

Owner lgtm.

[harpreet.sk, 2014-06-23 18:16:20]

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

[commit-bot: I haz the power, 2014-06-23 18:17:02]

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

[commit-bot: I haz the power, 2014-06-23 18:17:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-23 18:17:05]

A disapproval has been posted by following reviewers: cbiesinger@chromium.org.
Please make sure to get positive review before another CQ attempt.

[harpreet.sk, 2014-06-23 18:19:03]

@Christian can you please give lgtm

[cbiesinger, 2014-06-24 03:01:28]

Please fix the issues below, and also add a testcase for the example tony
mentioned. Also add a testcase for your example in #c12. Thanks!

https://codereview.chromium.org/331203002/diff/80001/LayoutTests/css3/flexbox...
File
LayoutTests/css3/flexbox/percent-height-children-of-alignSelf-stretch-flex-item.html
(right):

https://codereview.chromium.org/331203002/diff/80001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/percent-height-children-of-alignSelf-stretch-flex-item.html:3:
.flexbox-horizontal {
I'd really prefer you to use the stylesheet in
css3/flexbox/resources/flexbox.css

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

https://codereview.chromium.org/331203002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2679: else if
(cb->hasOverrideContainingBlockLogicalHeight())
So, we're now giving overrideContainingBlockLogicalHeight a new meaning. In
fact, it now has *two* incompatible meanings. That doesn't seem right? At the
very least, it seems confusing.

Cunningly, this will actually not break percentage sizing the flex item  itself
due to the child->style()->logicalHeight().isAuto() check and the ordering of
applying min/max, though perhaps that could cause problems with later relayouts.

But does this work correctly in all cases with grid?

How about making this much simpler: change this code to

  else if (cb->isFlexItemIncludingDeprecated())
      availableHeight = cb->overrideLogicalContentHeight();

(actually, we should probably add an isFlexItem() function that doesn't include
deprecated)

Then, you shouldn't even have to change RenderFlexibleBox.

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

https://codereview.chromium.org/331203002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1378:
child->setOverrideContainingBlockContentLogicalHeight(stretchedLogicalHeight -
child->borderAndPaddingLogicalHeight());
I wrote this comment below before I thought of the suggestion I gave above. I
decided to keep it here in case it is useful for future reference.

---

So, I see in the comments that you changed this from desiredLogicalHeight, but
this new code is wrong. It has to be desiredLogicalHeight, otherwise the
percentage height of the children of the flex item will be incorrect if there's
a max-height on the flex item.

Irrespective of that you do need to clear this width so that the code will work
correctly if the align-self property gets changed from stretch to something
else. Tony's example should be fine because when we mark a node as needing
layout we also mark its ancestors as needing layout, but you should add a test
to make sure.

[cbiesinger, 2014-06-24 03:04:09]

Oh, please also add a testcase that has a max-height on the flexitem and then a
percentage height on the child.

[harpreet.sk, 2014-06-24 07:57:00]

Thanks christian for the review. I had added the test cases which you have asked
and made changes as asked. Please have a look.

https://codereview.chromium.org/331203002/diff/80001/LayoutTests/css3/flexbox...
File
LayoutTests/css3/flexbox/percent-height-children-of-alignSelf-stretch-flex-item.html
(right):

https://codereview.chromium.org/331203002/diff/80001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/percent-height-children-of-alignSelf-stretch-flex-item.html:3:
.flexbox-horizontal {
On 2014/06/24 03:01:28, cbiesinger wrote:
> I'd really prefer you to use the stylesheet in
> css3/flexbox/resources/flexbox.css

Now using flexbox.css

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

https://codereview.chromium.org/331203002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2679: else if
(cb->hasOverrideContainingBlockLogicalHeight())
On 2014/06/24 03:01:28, cbiesinger wrote:
> So, we're now giving overrideContainingBlockLogicalHeight a new meaning. In
> fact, it now has *two* incompatible meanings. That doesn't seem right? At the
> very least, it seems confusing.
> 
> Cunningly, this will actually not break percentage sizing the flex item 
itself
> due to the child->style()->logicalHeight().isAuto() check and the ordering of
> applying min/max, though perhaps that could cause problems with later
relayouts.
> 
> But does this work correctly in all cases with grid?
> 
> How about making this much simpler: change this code to
> 
>   else if (cb->isFlexItemIncludingDeprecated())
>       availableHeight = cb->overrideLogicalContentHeight();
> 
> (actually, we should probably add an isFlexItem() function that doesn't
include
> deprecated)
> 
> Then, you shouldn't even have to change RenderFlexibleBox.


We simply cannot change the code as 

else if (cb->isFlexItemIncludingDeprecated()) 
    availableHeight = cb->overrideLogicalContentHeight();

It will cause the browser to crash for our test cases. It is because it enters
the given api first without cb->overrideLogicalContentHeight being set. So with
this condtion becoming true will call overrideLogicalContentHeight which does
not exist and hence it will get crash.

 Currently the code

 else if (cb->hasOverrideLogicalContentHeight()) 
    availableHeight = cb->overrideLogicalContentHeight();

 is working fine for all the cases for grid but to avoid problems in future
relayouts we can add an additional check of isFlexItem(). In my new patch i have
added this check in order to avoid porblem.
Added the api isFlexItem() in RenderBox.h

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

https://codereview.chromium.org/331203002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderFlexibleBox.cpp:1378:
child->setOverrideContainingBlockContentLogicalHeight(stretchedLogicalHeight -
child->borderAndPaddingLogicalHeight());
On 2014/06/24 03:01:28, cbiesinger wrote:
> I wrote this comment below before I thought of the suggestion I gave above. I
> decided to keep it here in case it is useful for future reference.
> 
> ---
> 
> So, I see in the comments that you changed this from desiredLogicalHeight, but
> this new code is wrong. It has to be desiredLogicalHeight, otherwise the
> percentage height of the children of the flex item will be incorrect if
there's
> a max-height on the flex item.
> 
> Irrespective of that you do need to clear this width so that the code will
work
> correctly if the align-self property gets changed from stretch to something
> else. Tony's example should be fine because when we mark a node as needing
> layout we also mark its ancestors as needing layout, but you should add a test
> to make sure.


I had made the changes to add desiredLogicalHeight instead of
stretchedLogicalHeight and clearing the 
overrideContainingBlockContentLogicalHeight afterwards. 
The case tony mention of updating height of grandchild to different percent
value is already there in layouttest.

[cbiesinger, 2014-06-24 22:59:25]

Thanks for updating the patch. I would like to try some more testcases with it,
I will need another day or two for this review.

[cbiesinger, 2014-06-25 04:14:32]

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

https://codereview.chromium.org/331203002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1376: if (desiredLogicalHeight !=
child->logicalHeight()) {
So I just realized there's another bug here. Even if desiredLogicalHeight ==
child->logicalHeight(), we still need to deal with the children of the child
here.

Testcase:
http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQN5GwFHTUZZHAGCnRYREVGiAYOm...
(green should be visible. first orange box should be 150px high. green box
should be 75px high.)

[cbiesinger, 2014-06-25 04:39:08]

On 2014/06/24 07:57:00, harpreet.sk wrote:
> else if (cb->isFlexItemIncludingDeprecated()) 
>     availableHeight = cb->overrideLogicalContentHeight();
> 
> It will cause the browser to crash for our test cases. It is because it enters
> the given api first without cb->overrideLogicalContentHeight being set. So
with
> this condtion becoming true will call overrideLogicalContentHeight which does
> not exist and hence it will get crash.

What if you also add an && hasOverrideLogicalContentHeight() check to the if?

[harpreet.sk, 2014-06-25 14:43:08]

Uploaded with some new changes to support the test case as mention in previous
comments. Test case is also added in the existing layout test. Please have a
look.

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

https://codereview.chromium.org/331203002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1376: if (desiredLogicalHeight !=
child->logicalHeight()) {
On 2014/06/25 04:14:32, cbiesinger wrote:
> So I just realized there's another bug here. Even if desiredLogicalHeight ==
> child->logicalHeight(), we still need to deal with the children of the child
> here.
> 
> Testcase:
>
http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQN5GwFHTUZZHAGCnRYREVGiAYOm...
> (green should be visible. first orange box should be 150px high. green box
> should be 75px high.)
> 


For the given case i think we need to do relayout of flex item. We need to add
an extra check other than desiredLogicalHeight != child->logicalHeight which
checks if the content of the flex item depends on its height or not. In new
solution i have added this check. I had added this test case in layout test as
well.

[harpreet.sk, 2014-06-26 18:12:23]

PING

[cbiesinger, 2014-06-26 19:54:19]

Sorry, I won't have time for a full review for the rest of the week. However, a
quick comment. It is not enough to just look at direct children of the flex
item. You need to look at all elements whose containing block is the flex item.
Also, this introduces an additional reflow that is not really necessary. I don't
think this approach is workable. Why not go back to the approach I suggested
before, just using the override height of the flex item in
RenderBox::computePercentageLogicalHeight if cb->isFlexItem() && alignment is
stretch && hasOverrideHeight()? Isn't that a lot simpler? Or does that not work
for some reason?

[harpreet.sk, 2014-07-30 09:16:26]

On 2014/06/26 19:54:19, cbiesinger wrote:
> Sorry, I won't have time for a full review for the rest of the week. However,
a
> quick comment. It is not enough to just look at direct children of the flex
> item. You need to look at all elements whose containing block is the flex
item.
> Also, this introduces an additional reflow that is not really necessary. I
don't
> think this approach is workable. Why not go back to the approach I suggested
> before, just using the override height of the flex item in
> RenderBox::computePercentageLogicalHeight if cb->isFlexItem() && alignment is
> stretch && hasOverrideHeight()? Isn't that a lot simpler? Or does that not
work
> for some reason?


@Chris: Thanks for the comment. The solution which you told of using override
height if (cb->isFlexItem() && alignment is stretch && hasOverrideHeight()) will
work for cases when desiredLogicalHeight != child->logicalHeight(). So we can
use override height for those cases.

But for cases where desiredLogicalHeight == child->logicalHeight() override
height values will get reset to 0 whenever it computes percentageLogicalHeight.
So for thoose case we cannot use overrideHeight. In my new solution i am using
OverrideContainingBlockContentLogicalHeight for cases of desiredLogicalHeight ==
child->logicalHeihgt() because it will not be reset to 0 whenever we compute
percentageLogicalHeight. 

This patch also solves BUG=346275 and BUG=396397. I think they might be the
duplicate of our original bug. Can you please confirm that whether these bug are
duplicate or not?

New solution is uploaded. Please take a look.

[harpreet.sk, 2014-08-05 19:09:33]

ping...

[cbiesinger, 2014-08-06 03:04:42]

> But for cases where desiredLogicalHeight == child->logicalHeight() override
> height values will get reset to 0 whenever it computes
percentageLogicalHeight.

Sorry, where is that code? I don't immediately see it, can you point me to it?

[cbiesinger, 2014-08-06 03:11:01]

Thanks for pointing out the bugs, commented/resolved them.

Also thanks for doing all the flexbox work you've been doing, and my apologies
for being slow in reviewing them!

[harpreet.sk, 2014-08-06 12:34:32]

On 2014/08/06 03:04:42, cbiesinger wrote:
> > But for cases where desiredLogicalHeight == child->logicalHeight() override
> > height values will get reset to 0 whenever it computes
> percentageLogicalHeight.
> 
> Sorry, where is that code? I don't immediately see it, can you point me to it?

@Chris: Sorry, i meant to say that for the cases where desiredLogicalHeight ==
child->logicalHeight() overideLogicalContentHeight gets cleared off and been
reset to initial value -1. The api 
RenderBox::clearOverrideLogicalContentHeight() is getting called which resets
the value of overideLogicalContentHeight to -1.

[cbiesinger, 2014-08-07 03:19:00]

On 2014/08/06 12:34:32, harpreet.sk wrote:
> On 2014/08/06 03:04:42, cbiesinger wrote:
> > > But for cases where desiredLogicalHeight == child->logicalHeight()
override
> > > height values will get reset to 0 whenever it computes
> > percentageLogicalHeight.
> > 
> > Sorry, where is that code? I don't immediately see it, can you point me to
it?
> 
> @Chris: Sorry, i meant to say that for the cases where desiredLogicalHeight ==
> child->logicalHeight() overideLogicalContentHeight gets cleared off and been
> reset to initial value -1. The api 
> RenderBox::clearOverrideLogicalContentHeight() is getting called which resets
> the value of overideLogicalContentHeight to -1.

Where does that call happen? I don't see it in
https://code.google.com/p/chromium/codesearch#search/&q=clearOverrideLogicalC...
:(

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

On 2014/08/07 03:19:00, cbiesinger wrote:
> On 2014/08/06 12:34:32, harpreet.sk wrote:

> > @Chris: Sorry, i meant to say that for the cases where desiredLogicalHeight
==
> > child->logicalHeight() overideLogicalContentHeight gets cleared off and been
> > reset to initial value -1. The api 
> > RenderBox::clearOverrideLogicalContentHeight() is getting called which
resets
> > the value of overideLogicalContentHeight to -1.
> 
> Where does that call happen? I don't see it in
>
https://code.google.com/p/chromium/codesearch#search/&q=clearOverrideLogicalC...
> :(


@Chris: The call happens in
RenderFlexibleBox::preferredMainAxisContentExtentForChild. It calls the api
clearOverrideSize() for the child of flexbox which then calls
clearOverrideLogicalContentHeight().

Since the layout for the case of percentage logical height happens in multiple
passes so say if we set overrideLogicalHeight in first pass it gets reset to -1
in second pass while calculating preferredMainAxisContentExtentForChild. So
that's why we need to setOverrideContainingBlockContentLogicalHeight.

[cbiesinger, 2014-08-08 03:21:02]

On 2014/08/07 12:26:08, harpreet.sk wrote:
> On 2014/08/07 03:19:00, cbiesinger wrote:
> > On 2014/08/06 12:34:32, harpreet.sk wrote:
> 
> > > @Chris: Sorry, i meant to say that for the cases where
desiredLogicalHeight
> ==
> > > child->logicalHeight() overideLogicalContentHeight gets cleared off and
been
> > > reset to initial value -1. The api 
> > > RenderBox::clearOverrideLogicalContentHeight() is getting called which
> resets
> > > the value of overideLogicalContentHeight to -1.
> > 
> > Where does that call happen? I don't see it in
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=clearOverrideLogicalC...
> > :(
> 
> 
> @Chris: The call happens in
> RenderFlexibleBox::preferredMainAxisContentExtentForChild. It calls the api
> clearOverrideSize() for the child of flexbox which then calls
> clearOverrideLogicalContentHeight().
> 
> Since the layout for the case of percentage logical height happens in multiple
> passes so say if we set overrideLogicalHeight in first pass it gets reset to
-1
> in second pass while calculating preferredMainAxisContentExtentForChild. So
> that's why we need to setOverrideContainingBlockContentLogicalHeight.

Hm... I kinda get what you're saying, but this does not seem like the best
solution for the problem. It feels like what we need is to make
applyStretchAlignmentForChild do something like "if
(child->percentHeightDescendants()) { child->forceLayout(); }" (because at this
point, the override height is already set... right?).

[harpreet.sk, 2014-08-11 12:13:14]

On 2014/08/08 03:21:02, cbiesinger wrote: 
> Hm... I kinda get what you're saying, but this does not seem like the best
> solution for the problem. It feels like what we need is to make
> applyStretchAlignmentForChild do something like "if
> (child->percentHeightDescendants()) { child->forceLayout(); }" (because at
this
> point, the override height is already set... right?).


Override height is not set for the case of desiredLogicalHeight ==
logicalHeight. We need to set it first before calling forceLayout for the child
(similar to the case of desiredLogicalHeight != logicalHeight). 

Uploaded a new patch. Please take a look...

[cbiesinger, 2014-08-11 22:54:23]

lgtm. Perfect! Thanks for bearing with me.

[harpreet.sk, 2014-08-18 08:55:32]

On 2014/08/11 22:54:23, cbiesinger wrote:
> lgtm. Perfect! Thanks for bearing with me.

Thanks Chris for your approval.

This patch also solves BUG=346275, 398980.

For bug 346275 you decided to keep it seperate from our main bug as it does not
relies on the flexbox using align-items:stretch. Can you also confirm for bug
398980 whether we want it to merge with our original bug or not.

Should i add these bugs in the description here?

[cbiesinger, 2014-08-19 02:44:55]

not lgtm

sorry, I just talked to a coworker and realized that this is forcing a layout of
the flex item every time the flex box gets layed out, even if the flex item size
does not change. Unfortunately, we could not really think of a good solution.

I am out of time today but I think we need to talk some more about this. I will
email some people tomorrow. I am sorry for delaying this further, but we don't
want common flexbox cases to be slow :(

[cbiesinger, 2014-08-21 23:59:22]

To avoid that unnecessary layout, I think the following approach will work:

We keep a vector (or map) of all child renderobjects that we call layout on
(making sure to only count those that actually got layed out, i.e. we don't want
to count layoutIfNeeded unless it was marked for layout). Then here in
applyStretchAlignment, we only need to call layout *if* the child did get layed
out during this layout round. Reason: if we didn't lay it out, nothing has
changed, all percentage descendants are still correct. If we did lay it out,
percentage descendants are very likely to be incorrect.

(note, we have to make sure that such flex items are not layout roots, because
that would mess up this algorithm, I think?)

[harpreet.sk, 2014-08-25 11:47:54]

On 2014/08/21 23:59:22, cbiesinger wrote:
> To avoid that unnecessary layout, I think the following approach will work:
> 
> We keep a vector (or map) of all child renderobjects that we call layout on
> (making sure to only count those that actually got layed out, i.e. we don't
want
> to count layoutIfNeeded unless it was marked for layout). Then here in
> applyStretchAlignment, we only need to call layout *if* the child did get
layed
> out during this layout round. Reason: if we didn't lay it out, nothing has
> changed, all percentage descendants are still correct. If we did lay it out,
> percentage descendants are very likely to be incorrect.
> 
> (note, we have to make sure that such flex items are not layout roots, because
> that would mess up this algorithm, I think?)


Thanks Chris for the approach. BTW do we really required to keep a vector (or
map) of all child renderobjects ? 

Since as you told in your approach that we need to call the layout *if* the
child did get layed out so how about we simply add check like
this in applyStretchAlignment api:

  if (child->layoutDidGetCalled() && child->hasPercentHeightDescendants())
      child->forceChildLayout();


The above solution is working fine. I have uploaded the patch with this
solution. PTAL...

[cbiesinger, 2014-08-27 00:53:40]

On 2014/08/25 11:47:54, harpreet.sk wrote:
> On 2014/08/21 23:59:22, cbiesinger wrote:
> > To avoid that unnecessary layout, I think the following approach will work:
> > 
> > We keep a vector (or map) of all child renderobjects that we call layout on
> > (making sure to only count those that actually got layed out, i.e. we don't
> want
> > to count layoutIfNeeded unless it was marked for layout). Then here in
> > applyStretchAlignment, we only need to call layout *if* the child did get
> layed
> > out during this layout round. Reason: if we didn't lay it out, nothing has
> > changed, all percentage descendants are still correct. If we did lay it out,
> > percentage descendants are very likely to be incorrect.
> > 
> > (note, we have to make sure that such flex items are not layout roots,
because
> > that would mess up this algorithm, I think?)
> 
> 
> Thanks Chris for the approach. BTW do we really required to keep a vector (or
> map) of all child renderobjects ? 
> 
> Since as you told in your approach that we need to call the layout *if* the
> child did get layed out so how about we simply add check like
> this in applyStretchAlignment api:
> 
>   if (child->layoutDidGetCalled() && child->hasPercentHeightDescendants())
>       child->forceChildLayout();
> 
> 
> The above solution is working fine. I have uploaded the patch with this
> solution. PTAL...

Thanks for the updated patch. I am not familiar with this function and have
emailed blink-dev for guidance about it. Thanks! It does look promising at first
sight.

[harpreet.sk, 2014-08-27 10:20:32]

On 2014/08/27 00:53:40, cbiesinger wrote:

> Thanks for the updated patch. I am not familiar with this function and have
> emailed blink-dev for guidance about it. Thanks! It does look promising at
first
> sight.


As per Adam comment on blink-dev it seems using layoutDidGetCalled() is
incorrect.

Now i have uploaded a new patch in which we don't need to require to do
forceChildLayout(). As per our previous dicussion i mentioned that
RenderFlexibleBox::preferredMainAxisContentExtentForChild is responsible for
clearing the overrideLogicalContentHeight() (Comment#35). 

I tried to debug and found out that we need to clear
overrideLogicalContentHeight only for Orthogonal flow cases and for the other
cases we only need to clear overrideLogicalContentWidth. But in the api
preferredMainAxisContentExtentForChild it was incorrectly clearing both
overrideLogicalContentWidth/Height.

I think my new solution is correct and working fine also for all test cases.
PTAL...

[cbiesinger, 2014-08-28 03:05:56]

This is a great idea and I think it will work. Just two minor comments below.

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

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:998: if (hasOrthogonalFlow(child))
When the flexbox changes from a column to a row flexbox (or vice versa), we have
to make sure to clear both override sizes, right? But you can handle that using
styleDidChange.

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1364:
child->setOverrideLogicalContentHeight(desiredLogicalHeight -
child->borderAndPaddingLogicalHeight());
I think we also have to relayout if the old overrideLogicalContentHeight != the
new logicalContentHeight (at least if hasPercentHeightDecendants()) -- consider
this example:

<div class="flexbox"><div style="height: 100px"><div
height="50%"></div></div></div>

The flexbox height will be identical to the intrinsic <div> height, so the if
below will be false, so the child div will be incorrectly sized, right? I don't
see a testcase for that in your test, either.

[ojan, 2014-08-28 03:23:09]

This is a good idea. I'm a little scared of not clearing the override size from
run to run though. We used to have bugs before where it would persist to the
next layout and we'd get the wrong result, but maybe it's OK for the cross-axis
direction. It's definitely not OK for the main-axis direction.

We can try it.

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:2284: if
(flexitem->style()->marginBefore().isAuto() ||
flexitem->style()->marginAfter().isAuto())
I think you just want this. You don't need the isColumnFlexDirection block at
all. Both code paths can call this. Calling marginStart/End was just a premature
performance optimization I think.

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

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:996: void
RenderFlexibleBox::clearLogicalOverrideSize(RenderBox* child)
LogicalOverrideSize doesn't tell you which size you're clearing.
LocicalCOntentHeight and logicalContentWidth are both logical override sizes.

[harpreet.sk, 2014-08-28 14:30:20]

Made changes. PTAL...

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:2284: if
(flexitem->style()->marginBefore().isAuto() ||
flexitem->style()->marginAfter().isAuto())
On 2014/08/28 03:23:09, ojan-only-code-yellow-reviews wrote:
> I think you just want this. You don't need the isColumnFlexDirection block at
> all. Both code paths can call this. Calling marginStart/End was just a
premature
> performance optimization I think.


I tried removing it but because of it following layout test is getting failed
"css3/flexbox/columns-center-with-margins.html"

In above LayoutTest the flex-item of colmun flexbox having auto margins is not
getting centered.

I think it's not correct to remove isColumnFlexDirection block .

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

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:996: void
RenderFlexibleBox::clearLogicalOverrideSize(RenderBox* child)
On 2014/08/28 03:23:09, ojan-only-code-yellow-reviews wrote:
> LogicalOverrideSize doesn't tell you which size you're clearing.
> LocicalCOntentHeight and logicalContentWidth are both logical override sizes.

I named this api like this just to keep consistency with above api
"setLogicalOverrideSize" . Since above api is responsible for setting
overrideLogicalHeight/Width based on flow so i thought to named this api as
clearLogicalOverrideSize.

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:998: if (hasOrthogonalFlow(child))
On 2014/08/28 03:05:56, cbiesinger wrote:
> When the flexbox changes from a column to a row flexbox (or vice versa), we
have
> to make sure to clear both override sizes, right? But you can handle that
using
> styleDidChange.

Added line to clear both override sizes.

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1364:
child->setOverrideLogicalContentHeight(desiredLogicalHeight -
child->borderAndPaddingLogicalHeight());
On 2014/08/28 03:05:56, cbiesinger wrote:
> I think we also have to relayout if the old overrideLogicalContentHeight !=
the
> new logicalContentHeight (at least if hasPercentHeightDecendants()) --
consider
> this example:
> 
> <div class="flexbox"><div style="height: 100px"><div
> height="50%"></div></div></div>
> 
> The flexbox height will be identical to the intrinsic <div> height, so the if
> below will be false, so the child div will be incorrectly sized, right? I
don't
> see a testcase for that in your test, either.


@Chris: The example which you have stated i think is not related to the bug. It
is because the flex-item (*child* here) have fixed height and you can see above
that our first condition checks
  
     if (!isColumnFlow() && child->style()->logicalHeight().isAuto())

Since style()->logicalHeight() of child is not auto (*fixed* here) so the child
will never stretch. The test case will never enter into this condition.

I cross checked this test case it was working fine in earlier version of chrome
also. Should i add this testcase although it's irrevelant to the bug.

[cbiesinger, 2014-08-29 00:28:10]

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

https://codereview.chromium.org/331203002/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1364:
child->setOverrideLogicalContentHeight(desiredLogicalHeight -
child->borderAndPaddingLogicalHeight());
On 2014/08/28 14:30:20, harpreet.sk wrote:
> On 2014/08/28 03:05:56, cbiesinger wrote:
> > I think we also have to relayout if the old overrideLogicalContentHeight !=
> the
> > new logicalContentHeight (at least if hasPercentHeightDecendants()) --
> consider
> > this example:
> > 
> > <div class="flexbox"><div style="height: 100px"><div
> > height="50%"></div></div></div>
> > 
> > The flexbox height will be identical to the intrinsic <div> height, so the
if
> > below will be false, so the child div will be incorrectly sized, right? I
> don't
> > see a testcase for that in your test, either.
> 
> 
> @Chris: The example which you have stated i think is not related to the bug.
It
> is because the flex-item (*child* here) have fixed height and you can see
above
> that our first condition checks
>   
>      if (!isColumnFlow() && child->style()->logicalHeight().isAuto())
> 
> Since style()->logicalHeight() of child is not auto (*fixed* here) so the
child
> will never stretch. The test case will never enter into this condition.
> 
> I cross checked this test case it was working fine in earlier version of
chrome
> also. Should i add this testcase although it's irrevelant to the bug. 
> 

Apologies, you are correct. I tried to make a testcase that hits the
desiredLogicalHeight == child->logicalHeight case below. I still think that your
patch does not handle that case correctly?

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

https://codereview.chromium.org/331203002/diff/260001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:217: child->clearOverrideSize();
Hmm, so we do need to do this in this case, but we also need to do it in other
cases - the child item may have been stretched because of align-self: stretch,
so we need to clear this also when flex-direction changes.

[harpreet.sk, 2014-09-01 08:10:59]

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

https://codereview.chromium.org/331203002/diff/260001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:217: child->clearOverrideSize();
On 2014/08/29 00:28:09, cbiesinger wrote:
> Hmm, so we do need to do this in this case, but we also need to do it in other
> cases - the child item may have been stretched because of align-self: stretch,
> so we need to clear this also when flex-direction changes.


Sorry Chris we do not required clearOverrideSize here. I tried various test
cases of row to column and column to row transformation for both align-items and
align-self equal to stretch and all are working fine.

[harpreet.sk, 2014-09-01 08:15:44]

On 2014/08/29 00:28:10, cbiesinger wrote:
 
> Apologies, you are correct. I tried to make a testcase that hits the
> desiredLogicalHeight == child->logicalHeight case below. I still think that
your
> patch does not handle that case correctly?

I already had a test case of desiredLogicalHeight == child->logicalHeight in my
layoutTest (see 6th test case)
which is handled properly. Which test case are you talking about. Can you please
clarify on it.

[harpreet.sk, 2014-09-04 13:51:17]

ping..

[cbiesinger, 2014-09-05 00:34:15]

Sorry, I don't see how the 6th testcase tests that. Maybe I counted wrong - can
you paste the testcase you are referring to?

[harpreet.sk, 2014-09-05 12:20:34]

On 2014/09/05 00:34:15, cbiesinger wrote:
> Sorry, I don't see how the 6th testcase tests that. Maybe I counted wrong -
can
> you paste the testcase you are referring to?

Test case which i was telling was: 

<div class="flexbox wrap align-content-flex-start" style="width: 100px; height:
100px; background-color: yellow; padding: 3px;">
  <div data-expected-height="30" style="min-height: 30%; width: 80px;
background-color: purple; margin: 2px; ">
    <div data-expected-height="15" style="background-color: red; height: 50%;
width: 20px;"></div>
  </div>
  <div data-expected-height="40" style="height: 40px; width: 80px;
background-color: green; margin: 2px;"></div>
</div>


Actually in the above test case desiredLogicalHeight == child->logicalHeight.
Actually as per our discussion earlier and as per comment #24 you suggested this
test case of desiredLogicalHeight == child->logicalHeight
link:
http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQN5GwFHTUZZHAGCnRYREVGiAYOm...

[cbiesinger, 2014-09-06 01:01:46]

On 2014/09/05 12:20:34, harpreet.sk wrote:
> On 2014/09/05 00:34:15, cbiesinger wrote:
> > Sorry, I don't see how the 6th testcase tests that. Maybe I counted wrong -
> can
> > you paste the testcase you are referring to?
> 
> Test case which i was telling was: 
> 
> <div class="flexbox wrap align-content-flex-start" style="width: 100px;
height:
> 100px; background-color: yellow; padding: 3px;">
>   <div data-expected-height="30" style="min-height: 30%; width: 80px;
> background-color: purple; margin: 2px; ">
>     <div data-expected-height="15" style="background-color: red; height: 50%;
> width: 20px;"></div>
>   </div>
>   <div data-expected-height="40" style="height: 40px; width: 80px;
> background-color: green; margin: 2px;"></div>
> </div>
> 
> 
> Actually in the above test case desiredLogicalHeight == child->logicalHeight.
> Actually as per our discussion earlier and as per comment #24 you suggested
this
> test case of desiredLogicalHeight == child->logicalHeight
> link:
>
http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQN5GwFHTUZZHAGCnRYREVGiAYOm...

Ah, neat. I missed that. Thanks!

But I don't understand how it works. We do the first layout and the element gets
sized to the right size. But we haven't set the override height yet because we
don't know it yet, so the children are incorrectly sized. Then, we get to
applyStretchAlignmentToChild. We set the override height but we don't layout
again, because the height is already matching desiredLogicalHeight. Where is my
logic wrong? I'm sorry that I'm holding up this CL even though it is practically
done, but I'd really like to understand this issue first. Thanks!

[harpreet.sk, 2014-09-08 09:48:56]

On 2014/09/06 01:01:46, cbiesinger wrote:
> Ah, neat. I missed that. Thanks!
> 
> But I don't understand how it works. We do the first layout and the element
gets
> sized to the right size. But we haven't set the override height yet because we
> don't know it yet, so the children are incorrectly sized. Then, we get to
> applyStretchAlignmentToChild. We set the override height but we don't layout
> again, because the height is already matching desiredLogicalHeight. Where is
my
> logic wrong? I'm sorry that I'm holding up this CL even though it is
practically
> done, but I'd really like to understand this issue first. Thanks!


Your logic is correct but what you have missed is that for
percentHeightDescendant() say for eg in our test case above the layout is done
in "multiple passes or multiple layout ". So say in the first layout the flex
item gets sized to correct size but since we haven't set the override height yet
because we don't know it yet, so the children are incorrectly sized. Then, we
get to applyStretchAlignmentToChild where we set the override height. Although
we are not explicitly calling child->forceChildLayout(); because the height is
already matching desiredLogicalHeight but as i told above that layout happens in
multiple passes so again layout is done for the flex-item.

Now in the second layout we have the overrideLogicalHeight set (as now we are
not clearing the overrideHeight which was happening earlier in
preferredMainAxisContentExtentForChild) for the flex item so children will also
get sized correctly as they know the correct sizes of their containing block (ie
flex-item).



I tried to debug the test case of desiredLogicalHeight == child->logicalHeight()
by adding LOGS and found that layout of the flex-item having percent height
descendants happen in multiple passes. So that the percent height children of
flexitem are sized correctly.

[cbiesinger, 2014-09-08 22:25:36]

On 2014/09/08 09:48:56, harpreet.sk wrote:
> On 2014/09/06 01:01:46, cbiesinger wrote:
> > Ah, neat. I missed that. Thanks!
> > 
> > But I don't understand how it works. We do the first layout and the element
> gets
> > sized to the right size. But we haven't set the override height yet because
we
> > don't know it yet, so the children are incorrectly sized. Then, we get to
> > applyStretchAlignmentToChild. We set the override height but we don't layout
> > again, because the height is already matching desiredLogicalHeight. Where is
> my
> > logic wrong? I'm sorry that I'm holding up this CL even though it is
> practically
> > done, but I'd really like to understand this issue first. Thanks!
> 
> 
> Your logic is correct but what you have missed is that for
> percentHeightDescendant() say for eg in our test case above the layout is done
> in "multiple passes or multiple layout ". So say in the first layout the flex
> item gets sized to correct size but since we haven't set the override height
yet
> because we don't know it yet, so the children are incorrectly sized. Then, we
> get to applyStretchAlignmentToChild where we set the override height. Although
> we are not explicitly calling child->forceChildLayout(); because the height is
> already matching desiredLogicalHeight but as i told above that layout happens
in
> multiple passes so again layout is done for the flex-item.
> 
> Now in the second layout we have the overrideLogicalHeight set (as now we are
> not clearing the overrideHeight which was happening earlier in
> preferredMainAxisContentExtentForChild) for the flex item so children will
also
> get sized correctly as they know the correct sizes of their containing block
(ie
> flex-item).
> 
> 
> 
> I tried to debug the test case of desiredLogicalHeight ==
child->logicalHeight()
> by adding LOGS and found that layout of the flex-item having percent height
> descendants happen in multiple passes. So that the percent height children of
> flexitem are sized correctly.

Hmm, I see that you're right in that the testcase passes, and so does your
plexode URL. However, I believe that it still relies on another layout being
triggered somewhere. For example, http://jsbin.com/gojupikenuci/1 does not work
for me, and that's just your testcase pasted into its own HTML file. I am
assuming, though I did not fully verify, that the additional layout is caused by
the dirtyForLayoutFromPercentageHeightDescendants call in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...,
so this will only work if the layout actually gets triggered.

[harpreet.sk, 2014-09-09 09:25:27]

On 2014/09/08 22:25:36, cbiesinger wrote:

> Hmm, I see that you're right in that the testcase passes, and so does your
> plexode URL. However, I believe that it still relies on another layout being
> triggered somewhere. For example, http://jsbin.com/gojupikenuci/1 does not
work
> for me, and that's just your testcase pasted into its own HTML file. 



The reason why http://jsbin.com/gojupikenuci/1 does not work for you because of
missing <!DOCTYPE html>
Just add it and it will work fine. If doctype tag is not there then the document
enters in quirks mode and as per api
RenderBox::skipContainingBlockForPercentHeightCalculation it skips auto-height
containingBlocks when computing percentages in quirks mode. That's the reason
the jsbin test case was not working.



>I am
> assuming, though I did not fully verify, that the additional layout is caused
by
> the dirtyForLayoutFromPercentageHeightDescendants call in
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...,
> so this will only work if the layout actually gets triggered.


You are correct the additional layout is caused by
dirtyForLayoutFromPercentageHeightDescendants. Actually what exactly is
happening is that RenderBlockFlow::layoutBlock calls the api
RenderBlockFlow::layoutBlockFlow
in a loop until the layout has been completed for the block something like this

    while (!done)
        done = layoutBlockFlow(relayoutChildren, pageLogicalHeight,
layoutScope);


In RenderBlockFlow::layoutBlockFlow the api layoutBlockChildren is get called
which then calls the api
dirtyForLayoutFromPercentageHeightDescendants(layoutScope);

After the layoutBlockChildren it checks for conditions like if height changed or
not and based on that decide whether to relayout or not.

So the layout is happening 2-3 times in our case and so the children are getting
sized correctly. So we do not require a forceChildLayout() in
applyStretchAlignment for case of desiredLogicalHeight ==
child->logicalHeight().


I think desiredLogicalHeight == child->logicalHeight() case is working fine for
all scenario. 

Just want to ask do we need the behaviour of the test case to be fixed for
document in quirks mode as well. I think it will be a one line change adding
!containingBlock->isFlexItem() in the last line of the api
RenderBox::skipContainingBlockForPercentHeightCalculation.

[cbiesinger, 2014-09-09 22:31:21]

On 2014/09/09 09:25:27, harpreet.sk wrote:
> On 2014/09/08 22:25:36, cbiesinger wrote:
> 
> > Hmm, I see that you're right in that the testcase passes, and so does your
> > plexode URL. However, I believe that it still relies on another layout being
> > triggered somewhere. For example, http://jsbin.com/gojupikenuci/1 does not
> work
> > for me, and that's just your testcase pasted into its own HTML file. 
> 
> 
> 
> The reason why http://jsbin.com/gojupikenuci/1 does not work for you because
of
> missing <!DOCTYPE html>
> Just add it and it will work fine. If doctype tag is not there then the
document
> enters in quirks mode and as per api
> RenderBox::skipContainingBlockForPercentHeightCalculation it skips auto-height
> containingBlocks when computing percentages in quirks mode. That's the reason
> the jsbin test case was not working.
> 
> 
> 
> >I am
> > assuming, though I did not fully verify, that the additional layout is
caused
> by
> > the dirtyForLayoutFromPercentageHeightDescendants call in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...,
> > so this will only work if the layout actually gets triggered.
> 
> 
> You are correct the additional layout is caused by
> dirtyForLayoutFromPercentageHeightDescendants. Actually what exactly is
> happening is that RenderBlockFlow::layoutBlock calls the api
> RenderBlockFlow::layoutBlockFlow
> in a loop until the layout has been completed for the block something like
this
> 
>     while (!done)
>         done = layoutBlockFlow(relayoutChildren, pageLogicalHeight,
> layoutScope);
> 
> 
> In RenderBlockFlow::layoutBlockFlow the api layoutBlockChildren is get called
> which then calls the api
> dirtyForLayoutFromPercentageHeightDescendants(layoutScope);
> 
> After the layoutBlockChildren it checks for conditions like if height changed
or
> not and based on that decide whether to relayout or not.
> 
> So the layout is happening 2-3 times in our case and so the children are
getting
> sized correctly. So we do not require a forceChildLayout() in
> applyStretchAlignment for case of desiredLogicalHeight ==
> child->logicalHeight().
> 
> 
> I think desiredLogicalHeight == child->logicalHeight() case is working fine
for
> all scenario. 
> 
> Just want to ask do we need the behaviour of the test case to be fixed for
> document in quirks mode as well. I think it will be a one line change adding
> !containingBlock->isFlexItem() in the last line of the api
> RenderBox::skipContainingBlockForPercentHeightCalculation.

But, which of the "return false" in layoutBlockFlow gets hit? None of them seem
like they should.

Thanks for pointing out the missing doctype. Still, I added the doctype in
http://jsbin.com/gojupikenuci/10, and the red rectangle only appears once I
resize the window (ie. once I trigger an additional layout).

[harpreet.sk, 2014-09-10 11:57:13]

On 2014/09/09 22:31:21, cbiesinger wrote:

> But, which of the "return false" in layoutBlockFlow gets hit? None of them
seem
> like they should.

Sorry i was wrong. It will never return false. Loop will only runs once.
Actually that api itself is running 3 times and not the loop. I had done some
more debugging and come with the following. Hope this time it is correct.


Example:   

    <div id="flexbox1" class="flexbox wrap align-content-flex-start"
style="width: 100px; height: 100px; background-color: yellow; padding: 3px;">
  <div id="item1" data-expected-height="30" style="min-height: 30%; width: 80px;
background-color: purple; margin: 2px; ">
    <div id="child1" data-expected-height="15" style="background-color: red;
height: 50%; width: 20px;"></div>
  </div>
  <div id="item2" data-expected-height="40" style="height: 40px; width: 80px;
background-color: green; margin: 2px;"></div>
</div>


Flow:

1.  FrameView::performLayout is called mutliple times. It calls the layout on
document (basically layoutRoot)
     
      rootForThisLayout->layout();


2.  Because layoutRoot is getting laid out multiple times so
RenderFlexibleBox::layoutBlock is called multiple times. (It was entering this
api 3 times for our case).


3. Then in RenderFlexibleBox::layoutAndPlaceChildren it calls the api 
child->layoutIfNeeded();

   layoutIfNeeded simply checks for needsLayout() and based on that calls
layout(). 

   In our example above needsLayout() always return true for item1 for all 3
rounds (m_bitfields.normalChildNeedsLayout() returns true for it). For item2 it
is true

   for 1st round but remains false for 2nd and 3rd round. So for item1 layout()
is getting called 3 times

   wherease for item2 it is called 1 time.

4. Now RenderBlock::layout() is called for each flex-item which then calls
layoutBlock()

5. Then RenderBlockFlow::layoutBlock

6. Then RenderBlockFlow::layoutBlockFlow

7. Then RenderBlockFlow::layoutBlockChildren and so on.


So item1 is getting laid out in 3 rounds and hence getting correct size for all
its children.



 
> Thanks for pointing out the missing doctype. Still, I added the doctype in
> http://jsbin.com/gojupikenuci/10, and the red rectangle only appears once I
> resize the window (ie. once I trigger an additional layout).


Can you please check it again. I checked the following link
http://jsbin.com/gojupikenuci/10 multiple times and is working correctly (no
need to resize window) . Even i checked on the systems of my coworker by apply
patch set 15 and it works fine on their system too. Can you please verify it
again. Thanks ! :)

[cbiesinger, 2014-09-11 06:27:07]

On 2014/09/10 11:57:13, harpreet.sk wrote:
> On 2014/09/09 22:31:21, cbiesinger wrote:
> 
> > But, which of the "return false" in layoutBlockFlow gets hit? None of them
> seem
> > like they should.
> 
> Sorry i was wrong. It will never return false. Loop will only runs once.
> Actually that api itself is running 3 times and not the loop. I had done some
> more debugging and come with the following. Hope this time it is correct.
> 
> 
> Example:   
> 
>     <div id="flexbox1" class="flexbox wrap align-content-flex-start"
> style="width: 100px; height: 100px; background-color: yellow; padding: 3px;">
>   <div id="item1" data-expected-height="30" style="min-height: 30%; width:
80px;
> background-color: purple; margin: 2px; ">
>     <div id="child1" data-expected-height="15" style="background-color: red;
> height: 50%; width: 20px;"></div>
>   </div>
>   <div id="item2" data-expected-height="40" style="height: 40px; width: 80px;
> background-color: green; margin: 2px;"></div>
> </div>
> 
> 
> Flow:
> 
> 1.  FrameView::performLayout is called mutliple times. It calls the layout on
> document (basically layoutRoot)
>      
>       rootForThisLayout->layout();
> 
> 
> 2.  Because layoutRoot is getting laid out multiple times so
> RenderFlexibleBox::layoutBlock is called multiple times. (It was entering this
> api 3 times for our case).
> 
> 
> 3. Then in RenderFlexibleBox::layoutAndPlaceChildren it calls the api 
> child->layoutIfNeeded();
> 
>    layoutIfNeeded simply checks for needsLayout() and based on that calls
> layout(). 
> 
>    In our example above needsLayout() always return true for item1 for all 3
> rounds (m_bitfields.normalChildNeedsLayout() returns true for it). For item2
it
> is true
> 
>    for 1st round but remains false for 2nd and 3rd round. So for item1
layout()
> is getting called 3 times
> 
>    wherease for item2 it is called 1 time.
> 
> 4. Now RenderBlock::layout() is called for each flex-item which then calls
> layoutBlock()
> 
> 5. Then RenderBlockFlow::layoutBlock
> 
> 6. Then RenderBlockFlow::layoutBlockFlow
> 
> 7. Then RenderBlockFlow::layoutBlockChildren and so on.
> 
> 
> So item1 is getting laid out in 3 rounds and hence getting correct size for
all
> its children.
> 
> 
> 
>  
> > Thanks for pointing out the missing doctype. Still, I added the doctype in
> > http://jsbin.com/gojupikenuci/10, and the red rectangle only appears once I
> > resize the window (ie. once I trigger an additional layout).
> 
> 
> Can you please check it again. I checked the following link
> http://jsbin.com/gojupikenuci/10 multiple times and is working correctly (no
> need to resize window) . Even i checked on the systems of my coworker by apply
> patch set 15 and it works fine on their system too. Can you please verify it
> again. Thanks ! :)

I'll have to think about your comment later, but just briefly: I did my testing
in content shell on trunk, on Linux. What did you use for your testing?

[harpreet.sk, 2014-09-11 06:34:51]

On 2014/09/11 06:27:07, cbiesinger wrote:

> I'll have to think about your comment later, but just briefly: I did my
testing
> in content shell on trunk, on Linux. What did you use for your testing?


I was checking this on chrome on trunk on linux (ninja -C out/Release chrome)
and even i check now on content shell it was working fine.

[harpreet.sk, 2014-09-16 11:17:37]

ping ..

[cbiesinger, 2014-09-17 02:02:47]

I am so sorry for my late response. What if you simply copy/paste my jsbin link
into a local file and run "content_shell --dump-render-tree foo.html", do you
still see a nonzero height for the red div? I see:

          RenderBlock {DIV} at (0,0) size 20x0 [bgcolor=#FF0000]

So the height is zero but it should be 75.

[harpreet.sk, 2014-09-18 14:18:24]

On 2014/09/17 02:02:47, cbiesinger wrote:
> I am so sorry for my late response. What if you simply copy/paste my jsbin
link
> into a local file and run "content_shell --dump-render-tree foo.html", do you
> still see a nonzero height for the red div? I see:
> 
>           RenderBlock {DIV} at (0,0) size 20x0 [bgcolor=#FF0000]
> 
> So the height is zero but it should be 75.


Yes it showing zero height instead of 75 when we copy/paste jsbin link into
local file. Chrome is behaving differently on different scenario.  Say 

1) If we run through test runner for chrome or do out/Release/chrome foo.html it
behave fine (that's the reason my test case above was working correctly) as in
that layout of flexbox was called 2 times. 

2) If we remove the second flexbox test case from foo.html then it will work
fine. At that time layout of flexbox is called 2 times.

3) Even for jsbin and codepen the examples was working fine.


So i think we need to do an extra layout for desiredLogicalHeight ==
childLogicalHeight case with percentHeightDescendants as we do not have any
better option. What's your opinion about it ?

[cbiesinger, 2014-09-19 02:47:36]

On 2014/09/18 14:18:24, harpreet.sk wrote:
> On 2014/09/17 02:02:47, cbiesinger wrote:
> > I am so sorry for my late response. What if you simply copy/paste my jsbin
> link
> > into a local file and run "content_shell --dump-render-tree foo.html", do
you
> > still see a nonzero height for the red div? I see:
> > 
> >           RenderBlock {DIV} at (0,0) size 20x0 [bgcolor=#FF0000]
> > 
> > So the height is zero but it should be 75.
> 
> 
> Yes it showing zero height instead of 75 when we copy/paste jsbin link into
> local file. Chrome is behaving differently on different scenario.  Say 
> 
> 1) If we run through test runner for chrome or do out/Release/chrome foo.html
it
> behave fine (that's the reason my test case above was working correctly) as in
> that layout of flexbox was called 2 times. 
> 
> 2) If we remove the second flexbox test case from foo.html then it will work
> fine. At that time layout of flexbox is called 2 times.
> 
> 3) Even for jsbin and codepen the examples was working fine.
> 
> 
> So i think we need to do an extra layout for desiredLogicalHeight ==
> childLogicalHeight case with percentHeightDescendants as we do not have any
> better option. What's your opinion about it ?

I think we only need that *if* overrideLogicalHeight != desiredLogicalHeight,
can you test that?

[harpreet.sk, 2014-09-22 17:54:54]

On 2014/09/19 02:47:36, cbiesinger wrote:

> I think we only need that *if* overrideLogicalHeight != desiredLogicalHeight,
> can you test that?

The above condition will not work. It is because overrideLogicalHeight is not
been set uptil now so hasOverrideHeight() will return false and hence above
condition will not work.

[cbiesinger, 2014-09-22 23:39:11]

On 2014/09/22 17:54:54, harpreet.sk wrote:
> On 2014/09/19 02:47:36, cbiesinger wrote:
> 
> > I think we only need that *if* overrideLogicalHeight !=
desiredLogicalHeight,
> > can you test that?
> 
> The above condition will not work. It is because overrideLogicalHeight is not
> been set uptil now so hasOverrideHeight() will return false and hence above
> condition will not work.

I'm not sure I follow. If hasOverrideHeight() returns false, we want to treat
that the same as if the override has changed, right? So in that case we want to
force a layout.

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

On 2014/09/22 23:39:11, cbiesinger wrote:
 
> I'm not sure I follow. If hasOverrideHeight() returns false, we want to treat
> that the same as if the override has changed, right? So in that case we want
to
> force a layout.

Sorry i misunderstood it earlier. Thanks for the suggestion. I have uploaded new
patch with the suggested changes. All cases are working fine. I checked on
content_shell also along with the test case mention in jsbin link in previous
comment. Please have a look.

[cbiesinger, 2014-09-25 00:50:31]

lgtm! Just a couple minor changes. Thanks for bearing with me for this really
long code review.

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

https://codereview.chromium.org/331203002/diff/360001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1367: if (!child.hasOverrideHeight()
|| ((desiredLogicalHeight - child.borderAndPaddingLogicalHeight()) !=
child.overrideLogicalContentHeight())) {
I think you have to *also* keep desiredLogicalHeight != child.logicalHeight() as
a condition here because there could be a change to the height of the child, in
which case we'd not relayout.

Also, please make desiredLogicalHeight - child.borderAndPaddingLogicalHeight() a
local variable so that the if is easier to read.

[harpreet.sk, 2014-09-25 12:58:57]

Made changes. Please have a look.

Also i need to ask regarding Bug=346275. Our patch fixes that bug. The reason
why that bug was occuring was because for computing percent logical height of
content it was not using overrideLogicalContentHeight which was set in the api
RenderFlexibleBox::setLogicalOverrideSize for orthogonal flow cases.

In our patch since we are using overrideLogicalContentHeight in
RenderBox::computePercentageLogicalHeight for flex-items so the given bug is
getting fixed with this patch. 

Please let me know your opinion about it and should we include that bug in our
description or not.

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

https://codereview.chromium.org/331203002/diff/360001/Source/core/rendering/R...
Source/core/rendering/RenderFlexibleBox.cpp:1367: if (!child.hasOverrideHeight()
|| ((desiredLogicalHeight - child.borderAndPaddingLogicalHeight()) !=
child.overrideLogicalContentHeight())) {
On 2014/09/25 00:50:31, cbiesinger wrote:
> I think you have to *also* keep desiredLogicalHeight != child.logicalHeight()
as
> a condition here because there could be a change to the height of the child,
in
> which case we'd not relayout.
> 
> Also, please make desiredLogicalHeight - child.borderAndPaddingLogicalHeight()
a
> local variable so that the if is easier to read.

Done.

[cbiesinger, 2014-09-25 22:12:41]

lgtm

I added that bug to this patch and will put it in the commit queue. But let's
leave the other bug open so we can resolve the last comment I just added to that
bug (low priority).

[cbiesinger, 2014-09-25 22:13:03]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2014-09-25 22:14:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/331203002/380001

[commit-bot: I haz the power, 2014-09-25 22:24:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-25 22:24:23]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)

[harpreet.sk, 2014-09-26 05:30:20]

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

[commit-bot: I haz the power, 2014-09-26 05:31:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/331203002/380001

[commit-bot: I haz the power, 2014-09-26 06:26:06]

Committed patchset #20 (id:380001) as 182735

[alph, 2014-09-29 16:11:00]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/616583002/ by alph@chromium.org.

The reason for reverting is: The patch broke a canvas with forced height in
DevTools. See crbug.com/418591.