[css-grid] Handle min-content/max-content with orthogonal flows

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 231 matching lines @@
 
     // Performance optimization: hold onto these Vectors until the end of Layout to avoid repeated malloc / free.
     Vector<GridTrack*> filteredTracks;
     Vector<GridItemWithSpan> itemsSortedByIncreasingSpan;
     Vector<GridTrack*> growBeyondGrowthLimitsTracks;
 
     LayoutUnit& freeSpaceForDirection(GridTrackSizingDirection direction) { return direction == ForColumns ? freeSpaceForColumns : freeSpaceForRows; }
 
     enum SizingOperation { TrackSizing, IntrinsicSizeComputation };
     SizingOperation sizingOperation { TrackSizing };
-
+    enum SizingState { ColumnSizingFirstIteration, RowSizingFirstIteration, ColumnSizingSecondIteration, RowSizingSecondIteration};
+    SizingState sizingState { ColumnSizingFirstIteration };
+    void nextState()
+    {
+        switch (sizingState) {
+        case ColumnSizingFirstIteration:
+            sizingState = RowSizingFirstIteration;
+            break;
+        case RowSizingFirstIteration:
+            sizingState = ColumnSizingSecondIteration;
+            break;
+        case ColumnSizingSecondIteration:
+            sizingState = RowSizingSecondIteration;
+            break;
+        case RowSizingSecondIteration:
+            sizingState = ColumnSizingFirstIteration;
+            break;
+        default:
+            NOTREACHED();
+            sizingState = ColumnSizingFirstIteration;
+        }
+    }

[svillar, 2016/04/14 08:58:42]

I think it's better to merge SizingOperation and SizingState in a single enum.

[jfernandez, 2016/05/18 10:31:40]

I'm not sure about this, for several reasons. First, the SizingState enum
defines intermediate states of the TrackSizing algorithm; the intrinsic sizing
logic is torally unrelated. We can see these states as sub-states of
TrackSizing. Second, there is no clear transition between
IntrinsicSizeComputation and any of the TrackSizing related states. As a matter
of fact, computeIntrinsicLogicalHeight uses the same Sizingdata instance being
used during the TrackSizing operation, which implies we have to set the original
state back.

+    bool sanityCheck(GridTrackSizingDirection direction)
+    {
+        switch (sizingState) {
+        case ColumnSizingFirstIteration:
+            return direction == ForColumns ? true : false;
+        case RowSizingFirstIteration:
+            return direction == ForRows ? true : false;
+        case ColumnSizingSecondIteration:
+            if (direction == ForRows)
+                sizingState = RowSizingFirstIteration;
+            return true;
+        case RowSizingSecondIteration:
+            return direction == ForRows ? true : false;
+        }
+        NOTREACHED();
+        return false;
+    }

[svillar, 2016/04/14 08:58:41]

Let's enclose this in debugging guards as it's meant only to be used inside
DCHECK I guess. Also perhaps a more descriptive name? sanityCheck looks fine but
a little bit too generic.

[jfernandez, 2016/05/18 10:31:40]

Done.

 private:
     LayoutUnit freeSpaceForColumns { };
     LayoutUnit freeSpaceForRows { };
 };
 
 struct GridItemsSpanGroupRange {
     Vector<GridItemWithSpan>::iterator rangeStart;
     Vector<GridItemWithSpan>::iterator rangeEnd;
 };
 
@@ skipping 62 matching lines @@
         logicalHeight += row.baseSize();
 
     logicalHeight += guttersSize(ForRows, sizingData.rowTracks.size());
 
     return logicalHeight;
 }
 
 void LayoutGrid::computeTrackSizesForDirection(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit freeSpace)
 {
     ASSERT(freeSpace >= 0);
+    DCHECK(sizingData.sanityCheck(direction));
     sizingData.freeSpaceForDirection(direction) = freeSpace - guttersSize(direction, direction == ForRows ? gridRowCount() : gridColumnCount());
     sizingData.sizingOperation = GridSizingData::TrackSizing;
 
     LayoutUnit baseSizes, growthLimits;
     computeUsedBreadthOfGridTracks(direction, sizingData, baseSizes, growthLimits, AvailableSpaceDefinite);
     ASSERT(tracksAreWiderThanMinTrackBreadth(direction, sizingData));
+    sizingData.nextState();
+}
+
+void LayoutGrid::repeatTracksSizingIfNeeded(GridSizingData& sizingData, LayoutUnit availableSpaceForColumns, LayoutUnit availableSpaceForRows)
+{
+    DCHECK(sizingData.sizingState > GridSizingData::RowSizingFirstIteration);
+
+    // In orthogonal flow cases column track's size is determined by using the computed
+    // row track's size, which it was estimated during the first cycle of the sizing
+    // algorithm. Hence we need to repeat computeUsedBreadthOfGridTracks for both,
+    // columns and rows, to determine the final values.
+    // TODO (lajava): orthogonal flows is just one of the cases which may require

[svillar, 2016/04/14 08:58:41]

which other cases?

[jfernandez, 2016/05/18 10:31:40]

Well, the spec states the following on this regard: 

"This cycle is necessary for cases where the inline size of a grid item depends
on the block size of its grid area."

Orthogonal flows are just an example of the above scenario; the spec mentions as
well wrapped column flex containers and multi-column elements. I wonder whether
there are other cases which apply directly to grid fulfilling the mentioned
condition. 

+    // a new cycle of the sizing algorithm; there may be more. In addition, not all the
+    // cases with orthogonal flows require this extra cycle; we need a more specific
+    // condition to detect whether child's min-content contribution has changed or not.
+    if (m_hasAnyOrthogonalChild) {
+        computeTrackSizesForDirection(ForColumns, sizingData, availableSpaceForColumns);
+        computeTrackSizesForDirection(ForRows, sizingData, availableSpaceForRows);
+    }
 }
 
 void LayoutGrid::layoutBlock(bool relayoutChildren)
 {
     ASSERT(needsLayout());
 
     if (!relayoutChildren && simplifiedLayout())
         return;
 
     {
@@ skipping 10 matching lines @@
 
         placeItemsOnGrid();
 
         GridSizingData sizingData(gridColumnCount(), gridRowCount());
 
         // 1- First, the track sizing algorithm is used to resolve the sizes of the grid columns.
         // At this point the logical width is always definite as the above call to updateLogicalWidth()
         // properly resolves intrinsic sizes. We cannot do the same for heights though because many code
         // paths inside updateLogicalHeight() require a previous call to setLogicalHeight() to resolve
         // heights properly (like for positioned items for example).
-        computeTrackSizesForDirection(ForColumns, sizingData, availableLogicalWidth());
+        LayoutUnit availableSpaceForColumns = availableLogicalWidth();
+        computeTrackSizesForDirection(ForColumns, sizingData, availableSpaceForColumns);
 
         // 2- Next, the track sizing algorithm resolves the sizes of the grid rows, using the
         // grid column sizes calculated in the previous step.
+        LayoutUnit availableSpaceForRows = availableLogicalHeight(ExcludeMarginBorderPadding);

[svillar, 2016/04/14 08:58:41]

Better not do this. Calling availableXXX only makes sense for definite logical
heights. Also you are not using this value because you're redefining the
variable later. So I'll rather keep it as it was before.

[jfernandez, 2016/05/18 10:31:40]

Done.

         if (logicalHeightWasIndefinite)
             computeIntrinsicLogicalHeight(sizingData);
         else
-            computeTrackSizesForDirection(ForRows, sizingData, availableLogicalHeight(ExcludeMarginBorderPadding));
+            computeTrackSizesForDirection(ForRows, sizingData, availableSpaceForRows);
         setLogicalHeight(computeTrackBasedLogicalHeight(sizingData) + borderAndPaddingLogicalHeight());
 
         LayoutUnit oldClientAfterEdge = clientLogicalBottom();
         updateLogicalHeight();
 
         // The above call might have changed the grid's logical height depending on min|max height restrictions.
         // Update the sizes of the rows whose size depends on the logical height (also on definite|indefinite sizes).
+        availableSpaceForRows = contentLogicalHeight();

[svillar, 2016/04/14 08:58:41]

Instead you can define this variable right here.

[jfernandez, 2016/05/18 10:31:40]

Done.

         if (logicalHeightWasIndefinite)
-            computeTrackSizesForDirection(ForRows, sizingData, contentLogicalHeight());
+            computeTrackSizesForDirection(ForRows, sizingData, availableSpaceForRows);
+
+        // 3- If the min-content contribution of any grid items have changed based on the row
+        // sizes calculated in step 2, steps 1 and 2 are repeated with the new min-content
+        // contribution (once only).
+        repeatTracksSizingIfNeeded(sizingData, availableSpaceForColumns, availableSpaceForRows);
 
         // Grid container should have the minimum height of a line if it's editable. That doesn't affect track sizing though.
         if (hasLineIfEmpty())
             setLogicalHeight(std::max(logicalHeight(), minimumLogicalHeightForEmptyLine()));
 
         applyStretchAlignmentToTracksIfNeeded(ForColumns, sizingData);
         applyStretchAlignmentToTracksIfNeeded(ForRows, sizingData);
 
         layoutGridItems(sizingData);
 
@@ skipping 262 matching lines @@
     return computeFlexFactorUnitSize(tracks, direction, flexFactorSum, leftOverSpace, flexibleTracksIndexes);
 }
 
 bool LayoutGrid::hasDefiniteLogicalSize(GridTrackSizingDirection direction) const
 {
     return (direction == ForRows) ? hasDefiniteLogicalHeight() : hasDefiniteLogicalWidth();
 }
 
 static bool hasOverrideContainingBlockContentSizeForChild(const LayoutBox& child, GridTrackSizingDirection direction)
 {
-    return direction == ForColumns ? child.hasOverrideContainingBlockLogicalWidth() : child.overrideContainingBlockContentLogicalHeight();
+    return direction == ForColumns ? child.hasOverrideContainingBlockLogicalWidth() : child.hasOverrideContainingBlockLogicalHeight();

[svillar, 2016/04/14 08:58:41]

Ups, this is a bug per se. Let's do it separately with a test.

 }
 
 static LayoutUnit overrideContainingBlockContentSizeForChild(const LayoutBox& child, GridTrackSizingDirection direction)
 {
     return direction == ForColumns ? child.overrideContainingBlockContentLogicalWidth() : child.overrideContainingBlockContentLogicalHeight();
 }
 
 static void setOverrideContainingBlockContentSizeForChild(LayoutBox& child, GridTrackSizingDirection direction, LayoutUnit size)
 {
     if (direction == ForColumns)
@@ skipping 57 matching lines @@
     return child.logicalHeight() + child.marginLogicalHeight();
 }
 
 GridTrackSizingDirection LayoutGrid::flowAwareDirectionForChild(const LayoutBox& child, GridTrackSizingDirection direction) const
 {
     return !isOrthogonalChild(child) ? direction : (direction == ForColumns ? ForRows : ForColumns);
 }
 
 LayoutUnit LayoutGrid::minSizeForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
 {
-    // TODO(svillar): Properly support orthogonal writing mode.
-    if (isOrthogonalChild(child))
-        return LayoutUnit();
     GridTrackSizingDirection childInlineDirection = flowAwareDirectionForChild(child, ForColumns);
     bool isRowAxis = direction == childInlineDirection;
     const Length& childSize = isRowAxis ? child.styleRef().logicalWidth() : child.styleRef().logicalHeight();
     const Length& childMinSize = isRowAxis ? child.styleRef().logicalMinWidth() : child.styleRef().logicalMinHeight();
     if (!childSize.isAuto() || childMinSize.isAuto())
         return minContentForChild(child, direction, sizingData);
 
     bool overrideSizeHasChanged = updateOverrideContainingBlockContentSizeForChild(child, childInlineDirection, sizingData);
     if (isRowAxis) {
         LayoutUnit marginLogicalWidth = sizingData.sizingOperation == GridSizingData::TrackSizing ? computeMarginLogicalSizeForChild(InlineDirection, child) : marginIntrinsicLogicalWidthForChild(child);
@@ skipping 11 matching lines @@
     LayoutUnit overrideSize = gridAreaBreadthForChild(child, direction, sizingData);
     if (hasOverrideContainingBlockContentSizeForChild(child, direction) && overrideContainingBlockContentSizeForChild(child, direction) == overrideSize)
         return false;
 
     setOverrideContainingBlockContentSizeForChild(child, direction, overrideSize);
     return true;
 }
 
 LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
 {
-    // FIXME: Properly support orthogonal writing mode.
-    if (isOrthogonalChild(child))
-        return LayoutUnit();
     GridTrackSizingDirection childInlineDirection = flowAwareDirectionForChild(child, ForColumns);
     if (direction == childInlineDirection) {
         // If |child| has a relative logical width, we shouldn't let it override its intrinsic width, which is
         // what we are interested in here. Thus we need to set the inline-axis override size to -1 (no possible resolution).
         if (shouldClearOverrideContainingBlockContentSizeForChild(child, ForColumns))
             setOverrideContainingBlockContentSizeForChild(child, childInlineDirection, LayoutUnit(-1));
 
         // FIXME: It's unclear if we should return the intrinsic width or the preferred width.
         // See http://lists.w3.org/Archives/Public/www-style/2013Jan/0245.html
         return child.minPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
     SubtreeLayoutScope layouter(child);
     if (updateOverrideContainingBlockContentSizeForChild(child, childInlineDirection, sizingData))
         child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
     return logicalHeightForChild(child, sizingData);
 }
 
 LayoutUnit LayoutGrid::maxContentForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
 {
-    if (isOrthogonalChild(child))
-        return LayoutUnit();
     GridTrackSizingDirection childInlineDirection = flowAwareDirectionForChild(child, ForColumns);
     if (direction == childInlineDirection) {
         // If |child| has a relative logical width, we shouldn't let it override its intrinsic width, which is
         // what we are interested in here. Thus we need to set the inline-axis override size to -1 (no possible resolution).
         if (shouldClearOverrideContainingBlockContentSizeForChild(child, ForColumns))
             setOverrideContainingBlockContentSizeForChild(child, childInlineDirection, LayoutUnit(-1));
 
         // FIXME: It's unclear if we should return the intrinsic width or the preferred width.
         // See http://lists.w3.org/Archives/Public/www-style/2013Jan/0245.html
         return child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
@@ skipping 366 matching lines @@
 
     ASSERT(m_gridItemArea.isEmpty());
 
     populateExplicitGridAndOrderIterator();
 
     // We clear the dirty bit here as the grid sizes have been updated.
     m_gridIsDirty = false;
 
     Vector<LayoutBox*> autoMajorAxisAutoGridItems;
     Vector<LayoutBox*> specifiedMajorAxisAutoGridItems;
+    m_hasAnyOrthogonalChild = false;
     for (LayoutBox* child = m_orderIterator.first(); child; child = m_orderIterator.next()) {
         if (child->isOutOfFlowPositioned())
             continue;
 
+        m_hasAnyOrthogonalChild = m_hasAnyOrthogonalChild || isOrthogonalChild(*child);
+
         GridArea area = cachedGridArea(*child);
         if (!area.rows.isIndefinite())
             area.rows.translate(abs(m_smallestRowStart));
         if (!area.columns.isIndefinite())
             area.columns.translate(abs(m_smallestColumnStart));
         m_gridItemArea.set(child, area);
 
         if (area.rows.isIndefinite() || area.columns.isIndefinite()) {
             GridSpan majorAxisPositions = (autoPlacementMajorAxisDirection() == ForColumns) ? area.columns : area.rows;
             if (majorAxisPositions.isIndefinite())
@@ skipping 262 matching lines @@
 
         // Because the grid area cannot be styled, we don't need to adjust
         // the grid breadth to account for 'box-sizing'.
         LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child->hasOverrideContainingBlockLogicalWidth() ? child->overrideContainingBlockContentLogicalWidth() : LayoutUnit();
         LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child->hasOverrideContainingBlockLogicalHeight() ? child->overrideContainingBlockContentLogicalHeight() : LayoutUnit();
 
         LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChildIncludingAlignmentOffsets(*child, ForColumns, sizingData);
         LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChildIncludingAlignmentOffsets(*child, ForRows, sizingData);
 
         SubtreeLayoutScope layoutScope(*child);
-        if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight()))
+        if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && (child->hasRelativeLogicalHeight() || isOrthogonalChild(*child))))
             layoutScope.setNeedsLayout(child, LayoutInvalidationReason::GridChanged);
 
         child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
         child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
 
         // Stretching logic might force a child layout, so we need to run it before the layoutIfNeeded
         // call to avoid unnecessary relayouts. This might imply that child margins, needed to correctly
         // determine the available space before stretching, are not set yet.
         applyStretchAlignmentToChildIfNeeded(*child);
 
@@ skipping 141 matching lines @@
     ASSERT(m_gridItemArea.contains(&gridItem));
     return m_gridItemArea.get(&gridItem);
 }
 
 GridSpan LayoutGrid::cachedGridSpan(const LayoutBox& gridItem, GridTrackSizingDirection direction) const
 {
     GridArea area = cachedGridArea(gridItem);
     return direction == ForColumns ? area.columns : area.rows;
 }
 
+bool LayoutGrid::gridLengthIsIndefinite(const GridLength& length, GridTrackSizingDirection direction) const

[svillar, 2016/04/14 08:58:41]

The name suggests that you're checking the length of the grid container. Let's
use a different name because what you're actually checking is the size of a grid
track.

[jfernandez, 2016/05/18 10:31:40]

Acknowledged.

+{
+    return length.isContentSized() || length.isFlex() || !hasDefiniteLogicalSize(direction);

[svillar, 2016/04/14 08:58:42]

This usage of hasDefiniteLogicalSize() is not correct. Note that it checks the
logical size of the grid container.

Something like this:
<div style="display:grid; height: min-content; grid-template-rows: 10px 10px;">

will return false for every single row which is wrong. We should definitely
revisit the tests because if they are working then something weird is happening
here (or perhaps we don't have enough)

I think you don't need that check at all.

[jfernandez, 2016/05/18 10:31:40]

I don't think there is anything wrong with current tests, at most, not enough
coverage. However, this logic is just intended for the initial state where we
don't know yet row's size for computing the orthogonal grid item's inline size.
This assumed value will be overwritten later during the second cycle of the
tracks sizing algorithm.
I agree. 

[svillar, 2016/05/26 08:49:02]

BTW you are considering percentages as definite sizes when that is not always
the case.

[jfernandez, 2016/05/27 09:41:22]

Note that the GridLength is extracted suing the gridTracksize function, which
already deals with percentages, so either they are converted to 'auto', hence
content-sized, or they are considered as definite.

Anyway, I admit this "isIndefinite" function is not very clear. I'd try to
implement something better.

+}
+
+LayoutUnit LayoutGrid::assumedRowsBreadthForOrthogonalChild(const LayoutBox& child) const

[svillar, 2016/04/14 08:58:41]

Breadth -> Size

[jfernandez, 2016/05/18 10:31:40]

Done.

+{
+    ASSERT(isOrthogonalChild(child));

[svillar, 2016/04/14 08:58:41]

DCHECK

[jfernandez, 2016/05/18 10:31:40]

Done.

+    const GridSpan& span = cachedGridSpan(child, ForRows);
+    LayoutUnit gridAreaBreadth;
+    for (auto trackPosition : span) {
+        const GridLength& maxTrackBreadth = gridTrackSize(ForRows, trackPosition).maxTrackBreadth();
+        if (gridLengthIsIndefinite(maxTrackBreadth, ForRows)) {
+            gridAreaBreadth = child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
+            break;

[svillar, 2016/04/14 08:58:41]

This looks wrong, because the final value of gridAreaBreadth will be always its
max preferred width. That's correct only when the max preferred width is larger
than the size of the tracks the item spans through.

Imagine the following example. An item with max preferred with of 20px which
spans 3 rows defined like this "grid-template-rows: 10px max-content 50px;"

The code above will return 20px as the grid area breadth which is wrong. The
max-content row will be 0px because with the other 2 we have more than enough
for the max preferred width of the item. So the grid area breadth will be 60px
and not 20px.

[jfernandez, 2016/05/18 10:31:40]

I understand your concerns, however, I think that for the case we are using this
function bot values are correct. Let me explain why so we can decide later which
approach we should follow. 

The assumed row's size is used to determine the size of the column tracks when
the item contributing to such size is orthogonal and we still don't know its 
block-size because it depends on its inline-size (determined based on the size
of the rows track). That's why we only call this function under the following 
condition:

    if (direction == ForRows && sizingData.sizingState ==
GridSizingData::ColumnSizingFirstIteration)

In this case, we only want to know whether tracks are definite, so we use such
definite size, or not. In the later case, we will just assume we have enough
space to laid out item's full content, hence the size of the column will be set
as
the item's minimum height. 

As you can see, it doesn't matter if we return 20px or 60px, because the size of
the columns track will be the same in both cases. 

If you prefer, we can use max(max-preferred, sunmOfDefiniteTracksinSpan),

[svillar, 2016/05/26 08:49:02]

If this is the case then we should have some ASSERTs verifying that.
I understand what you mean, but still I prefer the function to return a valid
value, if it's computing the area breadth then it should return that, not the
item's max inline size.

[jfernandez, 2016/05/27 09:41:22]

I don't think ASSERT that makes sense. One should know when we need to
estimate the size, because it's not possible to be determined at this phase of
the track sizing algorithm. However, I agree with you on that not being a valid
argument for not retuning a precise value, as you suggest in the comment
below.
I understand your point and will provide a change in that direction.

+        }
+        gridAreaBreadth += valueForLength(maxTrackBreadth.length(), LayoutUnit());

[svillar, 2016/04/14 08:58:41]

If I am not wrong the second value is the maximum which is used to resolve
percentages. I'd bet that percentages (against definite heights) are not
correctly resolved here and would always be 0. Do you have tests for that?

[jfernandez, 2016/05/18 10:31:40]

Your assumption on the usage of such second argument is correct. Also, you are
right that we lack tests for percentage sizes and orthogonal cases. 

Do you think we need that for this patch ? I mean, it's expectable to provide
support for all the cases combining orthogonal flows in a single patch ? Perhaps
a TODO here would be enough. What do you think ?

[svillar, 2016/05/26 08:49:02]

Yes I expect it to work for all the potential cases. If we cannot do that then
perhaps it means that we should split the patch in smaller pieces.

[jfernandez, 2016/05/27 09:41:22]

Ill fix the case of percentage sizes for tracks and provide tests to verify it
works as expected with orthogonal flows.

+    }
+
+    gridAreaBreadth += guttersSize(ForRows, span.integerSpan());
+
+    return gridAreaBreadth;
+}
+
 LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrackSizingDirection direction, const GridSizingData& sizingData) const
 {
-    const Vector<GridTrack>& tracks = direction == ForColumns ? sizingData.columnTracks : sizingData.rowTracks;
+    // To determine the column track's size based on an orthogonal grid item we need it's logical height, which
+    // may depend on the row track's size. It's possible that the row tracks sizing logic has not been performed yet,
+    // so we will need to do an estimation.
+    if (direction == ForRows && sizingData.sizingState == GridSizingData::ColumnSizingFirstIteration)
+        return assumedRowsBreadthForOrthogonalChild(child);
+
+    const Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;

[svillar, 2016/04/14 08:58:41]

No need for parentheses.

[jfernandez, 2016/05/18 10:31:40]

Done.

     const GridSpan& span = cachedGridSpan(child, direction);
     LayoutUnit gridAreaBreadth;
     for (const auto& trackPosition : span)
         gridAreaBreadth += tracks[trackPosition].baseSize();
 
     gridAreaBreadth += guttersSize(direction, span.integerSpan());
 
     return gridAreaBreadth;
 }
 
@@ skipping 447 matching lines @@
     case ContentPositionNormal:
         break;
     }
 
     ASSERT_NOT_REACHED();
     return {LayoutUnit(), LayoutUnit()};
 }
 
 LayoutPoint LayoutGrid::findChildLogicalPosition(const LayoutBox& child, GridSizingData& sizingData) const
 {
+    LayoutUnit columnAxisOffset = columnAxisOffsetForChild(child, sizingData);
     LayoutUnit rowAxisOffset = rowAxisOffsetForChild(child, sizingData);
     // We stored m_columnPosition's data ignoring the direction, hence we might need now
     // to translate positions from RTL to LTR, as it's more convenient for painting.
     if (!style()->isLeftToRightDirection()) {
         LayoutUnit alignmentOffset =  m_columnPositions[0] - borderAndPaddingStart();
         LayoutUnit rightGridEdgePosition = m_columnPositions[m_columnPositions.size() - 1] + alignmentOffset + borderAndPaddingLogicalLeft();
         rowAxisOffset = rightGridEdgePosition - (rowAxisOffset + child.logicalWidth());
     }
 
-    return LayoutPoint(rowAxisOffset, columnAxisOffsetForChild(child, sizingData));
+    // "In the positioning phase [...] calculations are performed according to the writing mode
+    // of the containing block of the box establishing the orthogonal flow." However, the
+    // resulting LayoutPoint will be used in 'setLogicalPosition' in order to set the child's
+    // logical position, which will only take into account the child's writing-mode.
+    LayoutPoint childLocation(rowAxisOffset, columnAxisOffset);
+    return isOrthogonalChild(child) ? childLocation.transposedPoint() : childLocation;
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const
 {
     GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 } // namespace blink