[css-flexbox] Apply min-height: auto to nested flexboxes again

[css-flexbox] Apply min-height: auto to nested flexboxes again

I am now convinced that the original bug that made me add this
was really https://codereview.chromium.org/1646413002 and we do
not need this check.

In particular, the main concern is that the child size we would
use here is one that was calculated based on a previous stretched
height. However, childHasIntrinsicMainAxisSize returns true
for a min-height: auto in an orthogonal flow (=the case where
that would be an issue), and in that case we make sure to force
a layout in computeNextFlexLine. Therefore, any size we use here
will be up-to-date and this should be safe.

BUG=596743
R=eae@chromium.org,dgrogan@chromium.org
TEST=css3/flexbox/nested-flexbox-min-size-auto.html

[cbiesinger, 2016-07-18 19:44:25]

The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 19:45:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cbiesinger, 2016-07-18 19:53:06]

Hm no, bug 580196's original testcase does fail with this patch. I'll have to
keep investigating this. Please hold off on reviewing.

[eae, 2016-07-18 20:04:18]

On 2016/07/18 19:53:06, cbiesinger wrote:
> Hm no, bug 580196's original testcase does fail with this patch. I'll have to
> keep investigating this. Please hold off on reviewing.

OK

[commit-bot: I haz the power, 2016-07-18 21:21:09]

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

[commit-bot: I haz the power, 2016-07-18 21:21:10]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[cbiesinger, 2016-07-19 14:46:22]

Description was changed from

==========
[css-flexbox] Apply min-height: auto to nested flexboxes again

I am now convinced that the original bug that made me add this
was really https://codereview.chromium.org/1646413002 and we do
not need this check.

In particular, the main concern is that the child size we would
use here is one that was calculated based on a previous stretched
height. However, childHasIntrinsicMainAxisSize returns true
for a min-height: auto in an orthogonal flow (=the case where
that would be an issue), and in that case we make sure to force
a layout in computeNextFlexLine. Therefore, any size we use here
will be up-to-date and this should be safe.

BUG=596743
R=eae@chromium.org,dgrogan@chromium.org
==========

to

==========
[css-flexbox] Apply min-height: auto to nested flexboxes again

I am now convinced that the original bug that made me add this
was really https://codereview.chromium.org/1646413002 and we do
not need this check.

In particular, the main concern is that the child size we would
use here is one that was calculated based on a previous stretched
height. However, childHasIntrinsicMainAxisSize returns true
for a min-height: auto in an orthogonal flow (=the case where
that would be an issue), and in that case we make sure to force
a layout in computeNextFlexLine. Therefore, any size we use here
will be up-to-date and this should be safe.

BUG=596743
R=eae@chromium.org,dgrogan@chromium.org
TEST=css3/flexbox/nested-flexbox-min-size-auto.html
==========

[cbiesinger, 2016-07-19 18:01:42]

On 2016/07/18 20:04:18, eae wrote:
> On 2016/07/18 19:53:06, cbiesinger wrote:
> > Hm no, bug 580196's original testcase does fail with this patch. I'll have
to
> > keep investigating this. Please hold off on reviewing.
> 
> OK

OK, the problem is that we only layout the child if it was marked as needing
layout. But it won't be marked as needing layout in this case because it didn't
change. Options are:
- Force layout in such a case, ie. if (childHasIntrinsicMainAxisSize(*child) &&
toLayoutBlock(*child).hasPercentHeightDescendants())
- Implement bug 581553, at least for min-height purposes, and use that

Have to think about this some.

[eae, 2016-08-09 21:37:41]

On 2016/07/19 18:01:42, cbiesinger wrote:
> On 2016/07/18 20:04:18, eae wrote:
> > On 2016/07/18 19:53:06, cbiesinger wrote:
> > > Hm no, bug 580196's original testcase does fail with this patch. I'll have
> to
> > > keep investigating this. Please hold off on reviewing.
> > 
> > OK
> 
> OK, the problem is that we only layout the child if it was marked as needing
> layout. But it won't be marked as needing layout in this case because it
didn't
> change. Options are:
> - Force layout in such a case, ie. if (childHasIntrinsicMainAxisSize(*child)
&&
> toLayoutBlock(*child).hasPercentHeightDescendants())
> - Implement bug 581553, at least for min-height purposes, and use that
> 
> Have to think about this some.

Do you still want to proceed with this change?

[cbiesinger, 2016-08-10 00:51:10]

On 2016/08/09 21:37:41, eae wrote:
> On 2016/07/19 18:01:42, cbiesinger wrote:
> > On 2016/07/18 20:04:18, eae wrote:
> > > On 2016/07/18 19:53:06, cbiesinger wrote:
> > > > Hm no, bug 580196's original testcase does fail with this patch. I'll
have
> > to
> > > > keep investigating this. Please hold off on reviewing.
> > > 
> > > OK
> > 
> > OK, the problem is that we only layout the child if it was marked as needing
> > layout. But it won't be marked as needing layout in this case because it
> didn't
> > change. Options are:
> > - Force layout in such a case, ie. if (childHasIntrinsicMainAxisSize(*child)
> &&
> > toLayoutBlock(*child).hasPercentHeightDescendants())
> > - Implement bug 581553, at least for min-height purposes, and use that
> > 
> > Have to think about this some.
> 
> Do you still want to proceed with this change?

No, let me close this. This is clearly a longer-term thing.