> If the first item is in the first cell, as the height of that cell is definite (200px),
> it can resolve a percentage

I'm confused -- computePercentageLogicalHeight does not look at its own height, only at its containing block's height, right? Why does it matter that the height of that cell is definite?

BTW, I've since learned about hasAutoHeightOrContainingBlockWithAutoHeight which has more duplicated code :(

>> If the first item is in the first cell, as the height of that cell is
> definite (200px),
>> it can resolve a percentage
> 
> I'm confused -- computePercentageLogicalHeight does not look at its own
> height, only at its containing block's height, right? Why does it matter
> that the height of that cell is definite?

The thing is that for grid items, we resolve the percentages against the grid area of that item.
If you have a simple 1x1 grid like this:
  <div style="display: grid; grid: 100px / 100px;">
    <div style="height: 50%;">item</div>
  </div>

You want the height of the item to be resolved against the grid area, which is the 100x100 cell defined.
So the height of the item is 50%.
Despite of the fact that the containing block (the grid container) has an indefinite height,
we can resolve the percentage height against the grid area.

We've a check for this in computePercentageLogicalHeight():
https://chromium.googlesource.com/chromium/src.git/+/25492d28991f4cd464c8d8ab728e548ae735b8a2/third_party/WebKit/Source/core/layout/LayoutBox.cpp#2744

> BTW, I've since learned about
> hasAutoHeightOrContainingBlockWithAutoHeight which has more duplicated
> code :(

Wow, what a mess... :-/

I've been taking a look and we've to do some similar fixes to the ones you do there for flexbox:
* https://codereview.chromium.org/2070823002
* https://codereview.chromium.org/2086083002/

Some examples failing with grid layout:
* http://jsbin.com/tayucu/1/edit?html,css,output
* http://jsbin.com/gozoge/1/edit?html,css,output

I'll report them in a different bug.

JFTR, I've reported the  bug #624716  to fix the issues in hasAutoHeightOrContainingBlockWithAutoHeight().

Not sure I understand this correctly, is the issue that we call computePercentageLogicalHeight before we have set the override containing block logical height?

Thanks for filing the other bug!

Sorry for the previous example you might get confused indeed.

The issue is that we only check the first child (as I explain on the initial comment).
Imagine the following grid with 2 rows and 1 column:
  <div style="display: grid; grid: 100px 50% / 200px;">
    <div>i1</div>
    <div>i2</div>
  </div>

We're using "firstInFlowChildBox()->percentageLogicalHeightIsResolvable()".

This means that we check "i1", it's in a cell with a fixed width and height (200x100),
so a percentage can be properly resolved there. Then we wrongly assume that the height
of the grid container is definite.

So when trying to resolve the 50% for the second row, we assume that the height is definite
and we resolve it to 50px, which is wrong, it should behave as "auto".

Ah! I see. Yeah that's tricky...

Apart from this we've a different way trying to check if the height
of the grid container is indefinite:
        bool logicalHeightWasIndefinite = computeContentLogicalHeight(MainOrPreferredSize, style()->logicalHeight(), LayoutUnit(-1)) == LayoutUnit(-1);

That one is wrong too, as it returns TRUE for a positioned grid with offsets:
  <div style="height: 200px; position: relative;">
    <div style="display: grid; position: absolute; top: 0; bottom: 0;"></div>
  </div>

In this example the height of the grid container is definite and it's 200px.
However the above method considers it's indefinite.

I don't have any use cases failing because of that wrong check in
LayoutGrid::layoutBlock(). The only issue there is that we're doing some extra calls,
that we won't need if we properly detect the size as definite.

However, as we use a similar method to compute the auto-repeat tracks,
there's a bug there: http://jsbin.com/sijefe/1/edit?html,css,output



http://jsbin.com/sijefe/1/edit?html,css,output

I'm trying to extrat the logic from computePercentageLogicalHeight()
so that we can use it to determine that the height
of the grid container is definite or indefinite.

Another case that is currently wrong:
  <div style="display: grid; grid: 200px / 200px;">
    <div style="display: grid; grid: 50% / 1fr;">
      <div style="background: magenta;">item</div>
    </div>
  </div>

The inner grid is a stretch grid item. So it's height is definite and it's 200px.
The height of the item should be 100px and right now it's 200px.

And one more case that is wrong right now:
  <div style="position: relative; height: 400px; width: 400px;">
    <div style="position: absolute; top: 0; bottom: 0; display: grid; grid: 100px / 100px;">
      <div style="height: 50%; background: magenta;">item</div>
    </div>
  </div>

The height of the item should be 50px but it's 200px right now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a12b00b915eccd82d4444ecba101f27e2761769

commit 9a12b00b915eccd82d4444ecba101f27e2761769
Author: rego <rego@igalia.com>
Date: Tue Aug 30 23:00:30 2016

[css-grid] Fix indefinite height detection

The change introduced in r398680 was wrong. In that occasion
in order to check if the height of an element was indefinite,
we were picking the first child and checking if a percentage
was resolvable for it.
This is not right, for example on a grid container where the first cell
has a fixed height, if the first child is in that cell it can resolve
a percentage. Thus, we were detecting that the grid container has
a definite height when it might not be the case.

This patch introduces back a LayoutBlock::hasDefiniteLogicalHeight()
reusing the logic from LayoutBox::computePercentageLogicalHeight()
that was extracted to a separated static method
LayoutBlock::availableLogicalHeightForPercentageComputation().
That way we can call hasDefiniteLogicalHeight() directly on an element
to determine if the height is or not definite.

This fixes a TODO related to orthogonal flows and also the example
explained above. The orthogonal flows test results are updated
and a new test is included.

BUG= 624301 , 617876 
TEST=fast/css-grid-layout/grid-container-percentage-rows.html

Review-Url: https://codereview.chromium.org/2154593003
Cr-Commit-Position: refs/heads/master@{#415478}

[add] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-container-percentage-rows.html
[add] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-grid-container-percentage-rows-expected.html
[add] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-grid-container-percentage-rows.html
[modify] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html
[add] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/LayoutTests/fast/css-grid-layout/positioned-grid-container-item-percentage-size-expected.html
[add] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/LayoutTests/fast/css-grid-layout/positioned-grid-container-item-percentage-size.html
[modify] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
[modify] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/Source/core/layout/LayoutBlock.h
[modify] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/Source/core/layout/LayoutBox.h
[modify] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
[modify] https://crrev.com/9a12b00b915eccd82d4444ecba101f27e2761769/third_party/WebKit/Source/core/layout/LayoutGrid.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73bf22175e3b50c1c1c6d02d1858c9b60fee6b02

commit 73bf22175e3b50c1c1c6d02d1858c9b60fee6b02
Author: rego <rego@igalia.com>
Date: Wed Sep 07 06:27:51 2016

[css-grid] Fix definite height detection for auto-repet tracks

Previous code was considering that all positioned grid containers
with auto height have an indefinite height.
However if they have non-auto top and bottom properties,
the height is definite.

The patches uses LayoutBlock::hasDefiniteLogicalHeight() in
LayoutGrid::layoutBlock(). This is not fixing any bug,
but it's avoiding some unneeded calls, as we properly identify
the height as definite on the first step.

To fix the issue with auto-repeat tracks
LayoutGrid::computeAutoRepeatTracksCount() is modified in order to use
LayoutBlock::availableLogicalHeightForPercentageComputation().
That way the height of the grid container is properly calculated
and the number of repetitions is the right one.

Added a new test case to verify the expected behavior.

BUG= 624301 
TEST=fast/css-grid-layout/grid-auto-repeat-positioned-container.html

Review-Url: https://codereview.chromium.org/2304543002
Cr-Commit-Position: refs/heads/master@{#416863}

[add] https://crrev.com/73bf22175e3b50c1c1c6d02d1858c9b60fee6b02/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-positioned-container-expected.html
[add] https://crrev.com/73bf22175e3b50c1c1c6d02d1858c9b60fee6b02/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-positioned-container.html
[modify] https://crrev.com/73bf22175e3b50c1c1c6d02d1858c9b60fee6b02/third_party/WebKit/Source/core/layout/LayoutGrid.cpp

All the cases described in this bug have been fixed.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e09a66db87cd70ecbd3b045464bf10ef623a03f9

commit e09a66db87cd70ecbd3b045464bf10ef623a03f9
Author: rego <rego@igalia.com>
Date: Thu Sep 15 11:02:32 2016

[css-grid] Cache definite height detection

This is a refactoring patch which stores the definite/indefinite height
detection in a new attribute m_hasDefiniteLogicalHeight in LayoutGrid.
That way we just only call LayoutBlock::hasDefiniteLogicalHeight() once,
from LayoutGrid::layoutBlock(). Then in LayoutGrid::gridTrackSize()
we reuse the cached value.

No new tests, no change of behavior.

BUG= 624301 

Review-Url: https://codereview.chromium.org/2334133002
Cr-Commit-Position: refs/heads/master@{#418831}

[modify] https://crrev.com/e09a66db87cd70ecbd3b045464bf10ef623a03f9/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/e09a66db87cd70ecbd3b045464bf10ef623a03f9/third_party/WebKit/Source/core/layout/LayoutGrid.h