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

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

Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index 1f24ae7d2b8ef22e858796c8160305acfc8d1294..1852a50ed04ba0c7fbf002f6acf4e6cf2104bb94 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -232,7 +232,7 @@ struct LayoutGrid::GridSizingData {
 public:
     GridSizingData(size_t gridColumnCount, size_t gridRowCount)
         : columnTracks(gridColumnCount)
-        , rowTracks(gridRowCount)
+        , rowTracks(0)
     {
     }
 
@@ -359,12 +359,16 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
 
         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.
         if (logicalHeightWasIndefinite)
             computeIntrinsicLogicalHeight(sizingData);
         else
@@ -383,6 +387,19 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
         if (hasLineIfEmpty())
             setLogicalHeight(std::max(logicalHeight(), minimumLogicalHeightForEmptyLine()));
 
+        // 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).
+        // TODO (lajava): is enough just verifying whether there are any orthogonal item ?
+        // TODO (lajava): Could we find better ways to fulfill such spec statement ?
+        // TODO (lajava): Could we avoid iterating over all the grid items ?"
+        // TODO (lajava): Why we dont need to repeat row tracks sizing as spec suggests (I was unable to find any test case needing so) ?"

[svillar, 2015/12/01 12:43:14]

We need to do that for example if we have two items in the same cell, one with
horizontal writing mode and the other with vertical writing mode. In such a
configuration, changing the column/row size will force one of the items to
re-layout forcing us to recompute the row/column.

+        if (hasAnyOrthogonalChild()) {
+            // In orthogonal flow cases column tracks size is determined based on the computed
+            // row track sizes, hence we need to repeat computeUsedBreadthOfGridTracks for columns.
+            computeTrackSizesForDirection(ForColumns, sizingData, availableSpaceForColumns);
+        }
+

[svillar, 2015/12/01 12:43:14]

This should be placed before the if (hasLineIfEmpty()) block. Otherwise step 3
could compute a grid height too small to be editable.

[jfernandez, 2015/12/01 15:13:38]

Acknowledged.

         applyStretchAlignmentToTracksIfNeeded(ForColumns, sizingData);
         applyStretchAlignmentToTracksIfNeeded(ForRows, sizingData);
 
@@ -477,6 +494,9 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
     Vector<size_t> flexibleSizedTracksIndex;
     sizingData.contentSizedTracksIndex.shrink(0);
 
+    if (direction == ForRows && tracks.size() == 0)
+        tracks.grow(gridRowCount());

[cbiesinger, 2015/11/26 02:26:16]

why is this now needed, when it wasn't before?

[jfernandez, 2015/11/26 08:58:27]

Since I needed a way to detect we are using rowTracks vector before populating
it with data, I decided to not initializing it so I can check out whether it was
null or not. So, we have initialize the vector the first time we run the track
sizing algorithm in the ForRow direction.

+
     const LayoutUnit maxSize = std::max(LayoutUnit(), initialFreeSpace);
     // 1. Initialize per Grid track variables.
     for (size_t i = 0; i < tracks.size(); ++i) {
@@ -550,7 +570,7 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
                 if (i > 0 && span.resolvedInitialPosition.toInt() <= flexibleSizedTracksIndex[i - 1])
                     continue;
 
-                flexFraction = std::max(flexFraction, findFlexFactorUnitSize(tracks, span, direction, maxContentForChild(*gridItem, direction, sizingData.columnTracks)));
+                flexFraction = std::max(flexFraction, findFlexFactorUnitSize(tracks, span, direction, maxContentForChild(*gridItem, direction, sizingData)));
             }
         }
     }
@@ -648,11 +668,35 @@ double LayoutGrid::findFlexFactorUnitSize(const Vector<GridTrack>& tracks, const
     return computeFlexFactorUnitSize(tracks, direction, flexFactorSum, leftOverSpace, flexibleTracksIndexes);
 }
 
+bool LayoutGrid::hasAnyOrthogonalChild() const
+{
+    for (LayoutBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+        if (child->isHorizontalWritingMode() != isHorizontalWritingMode())
+            return true;
+    }
+    return false;
+}
+
 bool LayoutGrid::hasDefiniteLogicalSize(GridTrackSizingDirection direction) const
 {
     return (direction == ForRows) ? hasDefiniteLogicalHeight() : hasDefiniteLogicalWidth();
 }
 
+LayoutUnit LayoutGrid::overrideContainingBlockBreadthForChild(const LayoutBox& child, GridTrackSizingDirection direction)
+{

[svillar, 2015/12/01 12:43:14]

I'd ASSERT() on hasOverrideContainingBlockXXX() and force the caller to check it
before calling this function. Returning LayoutUnit() is not correct because 0 is
a valid override value.

[jfernandez, 2015/12/01 15:13:39]

I really wanted to return 0 (actually, it was the original implementation),
since we use this method to compare such value with the one computed via
gridAreaBreadthForChild. 

We wouldn't need a new assert in any case, since
overrideContainingBlockContentLogicalX has got it already. What we could do,
instead, is to call hasXXX before, so if it returns false, we will directly use
the computed value, but I think such logic is equivalent and it provides no
advantage. 

+    if (direction == ForRows)
+        return child.hasOverrideContainingBlockLogicalHeight() ? child.overrideContainingBlockContentLogicalHeight() : LayoutUnit();
+    return child.hasOverrideContainingBlockLogicalWidth() ? child.overrideContainingBlockContentLogicalWidth() : LayoutUnit();
+}
+
+void LayoutGrid::setOverrideContainingBlockBreadthForChild(LayoutBox& child, GridTrackSizingDirection direction, LayoutUnit breadth)
+{
+    if (direction == ForRows)
+        child.setOverrideContainingBlockContentLogicalHeight(breadth);
+    else
+        child.setOverrideContainingBlockContentLogicalWidth(breadth);
+}
+
 GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size_t i) const
 {
     bool isForColumns = direction == ForColumns;
@@ -675,12 +719,15 @@ GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size
     return GridTrackSize(minTrackBreadth, maxTrackBreadth);
 }
 
-LayoutUnit LayoutGrid::logicalHeightForChild(LayoutBox& child, Vector<GridTrack>& columnTracks)
+LayoutUnit LayoutGrid::logicalHeightForChild(LayoutBox& child, const GridSizingData& sizingData)
 {
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();

[svillar, 2015/12/01 12:43:14]

This is repeated so many times that we should have a private method to handle
it.

[jfernandez, 2015/12/01 15:13:39]

Acknowledged.

+    GridTrackSizingDirection childInlineDirection = hasOrthogonalWritingMode ? ForRows : ForColumns;
     SubtreeLayoutScope layoutScope(child);
-    LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child.hasOverrideContainingBlockLogicalWidth() ? child.overrideContainingBlockContentLogicalWidth() : LayoutUnit();
-    LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(child, ForColumns, columnTracks);
-    if (child.hasRelativeLogicalHeight() || oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth) {
+    // Child's min-content contribution depends on the container's size along the child's inline-axis.
+    LayoutUnit oldGridAreaBreadth = overrideContainingBlockBreadthForChild(child, childInlineDirection);
+    LayoutUnit gridAreaBreadth = gridAreaBreadthForChild(child, childInlineDirection, sizingData);
+    if (child.hasRelativeLogicalHeight() || oldGridAreaBreadth != gridAreaBreadth) {
         layoutScope.setNeedsLayout(&child, LayoutInvalidationReason::GridChanged);
     }
 
@@ -689,75 +736,72 @@ LayoutUnit LayoutGrid::logicalHeightForChild(LayoutBox& child, Vector<GridTrack>
     if (hasOverrideHeight && child.needsLayout())
         child.clearOverrideLogicalContentHeight();
 
-    child.setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
+    setOverrideContainingBlockBreadthForChild(child, childInlineDirection, gridAreaBreadth);
     // If |child| has a relative logical height, we shouldn't let it override its intrinsic height, which is
-    // what we are interested in here. Thus we need to set the override logical height to -1 (no possible resolution).
+    // what we are interested in here. Thus we need to set the block-axis override breadth to -1 (no possible resolution).
     if (child.hasRelativeLogicalHeight())
-        child.setOverrideContainingBlockContentLogicalHeight(-1);
+        setOverrideContainingBlockBreadthForChild(child, childInlineDirection == ForColumns ? ForRows : ForColumns, -1);
     child.layoutIfNeeded();
     // If the child was stretched we should use its intrinsic height.
     return (hasOverrideHeight ? childIntrinsicHeight(child) : child.logicalHeight()) + child.marginLogicalHeight();
 }
 
-LayoutUnit LayoutGrid::minSizeForChild(LayoutBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
+static inline GridTrackSizingDirection flowAwareDirection(bool hasOrthogonalWritingMode, GridTrackSizingDirection direction)
 {
-    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    // TODO(svillar): Properly support orthogonal writing mode.
     if (hasOrthogonalWritingMode)
-        return LayoutUnit();
+        return direction == ForColumns ? ForRows : ForColumns;
+    return direction;
+}

[svillar, 2015/12/01 12:43:14]

With the private method I mentioned above we could transform this in another
private method which does not need the bool (would call the other one
internally).

Also this can be a one-liner using another ternary operator.

[jfernandez, 2015/12/01 15:13:39]

We would change an static inline method for a class method; are you really sure
it'd be better ? 
I'm not sure whether a nested ternary operator results n a clearer code.
actually, I was asked several times to avoid it. 

 
-    const Length& childMinSize = direction == ForColumns ? child.style()->logicalMinWidth() : child.style()->logicalMinHeight();
+LayoutUnit LayoutGrid::minSizeForChild(LayoutBox& child, GridTrackSizingDirection direction, const GridSizingData& sizingData)
+{
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    const Length& childMinSize = flowAwareDirection(hasOrthogonalWritingMode, direction) == ForColumns ? child.style()->logicalMinWidth() : child.style()->logicalMinHeight();
     if (childMinSize.isAuto()) {
         // TODO(svillar): Implement intrinsic aspect ratio support (transferred size in specs).
-        return minContentForChild(child, direction, columnTracks);
+        return minContentForChild(child, direction, sizingData);
     }
 
-    if (direction == ForColumns)
+    if (flowAwareDirection(hasOrthogonalWritingMode, direction) == ForColumns)
         return child.computeLogicalWidthUsing(MinSize, childMinSize, contentLogicalWidth(), this);
 
     return child.computeContentLogicalHeight(MinSize, childMinSize, child.logicalHeight()) + child.scrollbarLogicalHeight();
 }
 
-LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
+LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirection direction, const GridSizingData& sizingData)
 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    // FIXME: Properly support orthogonal writing mode.
-    if (hasOrthogonalWritingMode)
-        return 0;
-
-    if (direction == ForColumns) {
+    if (flowAwareDirection(hasOrthogonalWritingMode, direction) == ForColumns) {
         // 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 override logical width to -1 (no possible resolution).
+        // what we are interested in here. Thus we need to set the inline-axis override breadth to -1 (no possible resolution).
+        GridTrackSizingDirection childInlineDirection = hasOrthogonalWritingMode ? ForRows : ForColumns;
         if (child.hasRelativeLogicalWidth())
-            child.setOverrideContainingBlockContentLogicalWidth(-1);
+            setOverrideContainingBlockBreadthForChild(child, childInlineDirection, -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);
     }
 
-    return logicalHeightForChild(child, columnTracks);
+    return logicalHeightForChild(child, sizingData);
 }
 
-LayoutUnit LayoutGrid::maxContentForChild(LayoutBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
+LayoutUnit LayoutGrid::maxContentForChild(LayoutBox& child, GridTrackSizingDirection direction, const GridSizingData& sizingData)
 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    // FIXME: Properly support orthogonal writing mode.
-    if (hasOrthogonalWritingMode)
-        return LayoutUnit();
-
-    if (direction == ForColumns) {
+    if (flowAwareDirection(hasOrthogonalWritingMode, direction) == ForColumns) {
         // 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 override logical width to -1 (no possible resolution).
+        GridTrackSizingDirection childInlineDirection = hasOrthogonalWritingMode ? ForRows : ForColumns;
         if (child.hasRelativeLogicalWidth())
-            child.setOverrideContainingBlockContentLogicalWidth(-1);
+            setOverrideContainingBlockBreadthForChild(child, childInlineDirection, -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);
     }
 
-    return logicalHeightForChild(child, columnTracks);
+    return logicalHeightForChild(child, sizingData);
 }
 
 // We're basically using a class instead of a std::pair because of accessing gridItem() or gridSpan() is much more
@@ -804,7 +848,7 @@ void LayoutGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirectio
             if (itemsSet.add(gridItem).isNewEntry) {
                 const GridSpan& span = cachedGridSpan(*gridItem, direction);
                 if (span.integerSpan() == 1) {
-                    resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, span, *gridItem, track, sizingData.columnTracks);
+                    resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, span, *gridItem, track, sizingData);
                 } else if (!spanningItemCrossesFlexibleSizedTracks(span, direction)) {
                     sizingData.itemsSortedByIncreasingSpan.append(GridItemWithSpan(*gridItem, span));
                 }
@@ -832,22 +876,22 @@ void LayoutGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirectio
     }
 }
 
-void LayoutGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection direction, const GridSpan& span, LayoutBox& gridItem, GridTrack& track, Vector<GridTrack>& columnTracks)
+void LayoutGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection direction, const GridSpan& span, LayoutBox& gridItem, GridTrack& track, const GridSizingData& sizingData)
 {
     const GridResolvedPosition trackPosition = span.resolvedInitialPosition;
     GridTrackSize trackSize = gridTrackSize(direction, trackPosition.toInt());
 
     if (trackSize.hasMinContentMinTrackBreadth())
-        track.setBaseSize(std::max(track.baseSize(), minContentForChild(gridItem, direction, columnTracks)));
+        track.setBaseSize(std::max(track.baseSize(), minContentForChild(gridItem, direction, sizingData)));
     else if (trackSize.hasMaxContentMinTrackBreadth())
-        track.setBaseSize(std::max(track.baseSize(), maxContentForChild(gridItem, direction, columnTracks)));
+        track.setBaseSize(std::max(track.baseSize(), maxContentForChild(gridItem, direction, sizingData)));
     else if (trackSize.hasAutoMinTrackBreadth())
-        track.setBaseSize(std::max(track.baseSize(), minSizeForChild(gridItem, direction, columnTracks)));
+        track.setBaseSize(std::max(track.baseSize(), minSizeForChild(gridItem, direction, sizingData)));
 
     if (trackSize.hasMinContentMaxTrackBreadth())
-        track.setGrowthLimit(std::max(track.growthLimit(), minContentForChild(gridItem, direction, columnTracks)));
+        track.setGrowthLimit(std::max(track.growthLimit(), minContentForChild(gridItem, direction, sizingData)));
     else if (trackSize.hasMaxContentOrAutoMaxTrackBreadth())
-        track.setGrowthLimit(std::max(track.growthLimit(), maxContentForChild(gridItem, direction, columnTracks)));
+        track.setGrowthLimit(std::max(track.growthLimit(), maxContentForChild(gridItem, direction, sizingData)));
 }
 
 static const LayoutUnit& trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase phase, const GridTrack& track, TrackSizeRestriction restriction)
@@ -955,17 +999,17 @@ static void updateTrackSizeForTrackSizeComputationPhase(TrackSizeComputationPhas
     ASSERT_NOT_REACHED();
 }
 
-LayoutUnit LayoutGrid::currentItemSizeForTrackSizeComputationPhase(TrackSizeComputationPhase phase, LayoutBox& gridItem, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
+LayoutUnit LayoutGrid::currentItemSizeForTrackSizeComputationPhase(TrackSizeComputationPhase phase, LayoutBox& gridItem, GridTrackSizingDirection direction, const GridSizingData& sizingData)
 {
     switch (phase) {
     case ResolveIntrinsicMinimums:
-        return minSizeForChild(gridItem, direction, columnTracks);
+        return minSizeForChild(gridItem, direction, sizingData);
     case ResolveContentBasedMinimums:
     case ResolveIntrinsicMaximums:
-        return minContentForChild(gridItem, direction, columnTracks);
+        return minContentForChild(gridItem, direction, sizingData);
     case ResolveMaxContentMinimums:
     case ResolveMaxContentMaximums:
-        return maxContentForChild(gridItem, direction, columnTracks);
+        return maxContentForChild(gridItem, direction, sizingData);
     case MaximizeTracks:
         ASSERT_NOT_REACHED();
         return 0;
@@ -1010,7 +1054,7 @@ void LayoutGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizing
 
         spanningTracksSize += guttersSize(direction, itemSpan.integerSpan());
 
-        LayoutUnit extraSpace = currentItemSizeForTrackSizeComputationPhase(phase, gridItemWithSpan.gridItem(), direction, sizingData.columnTracks) - spanningTracksSize;
+        LayoutUnit extraSpace = currentItemSizeForTrackSizeComputationPhase(phase, gridItemWithSpan.gridItem(), direction, sizingData) - spanningTracksSize;
         extraSpace = std::max<LayoutUnit>(extraSpace, 0);
         auto& tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? sizingData.filteredTracks : sizingData.growBeyondGrowthLimitsTracks;
         distributeSpaceToTracks<phase>(sizingData.filteredTracks, &tracksToGrowBeyondGrowthLimits, sizingData, extraSpace);
@@ -1528,8 +1572,35 @@ GridSpan LayoutGrid::cachedGridSpan(const LayoutBox& gridItem, GridTrackSizingDi
     return direction == ForColumns ? coordinate.columns : coordinate.rows;
 }
 
-LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrackSizingDirection direction, const Vector<GridTrack>& tracks) const
+bool LayoutGrid::gridLengthIsIndefinite(const GridLength& length, GridTrackSizingDirection direction) const
 {
+    return length.isContentSized() || length.isFlex() || (length.hasPercentage() && !hasDefiniteLogicalSize(direction));
+}

[svillar, 2015/12/01 12:43:14]

I panicked when I saw this one :). We should not use hasDefiniteLogicalXXX in
new code as it is completely wrong whenever we specify a min|max content
restriction (like width: 10px; min-width: min-content;) as it only checks
width/height properties.

Couldn't you just check that the growthLimit() of the track is not -1?

[jfernandez, 2015/12/01 15:13:39]

Umm, can you confirm that checking that out is enough to fulfill the
specification statement of "a track having a definite max track sizing function"
? 

https://drafts.csswg.org/css-grid/#algo-overview

+
+LayoutUnit LayoutGrid::assumedRowsBreadthForOrthogonalChild(const LayoutBox& child) const
+{
+    ASSERT(child.isHorizontalWritingMode() != isHorizontalWritingMode());
+    const GridSpan& span = cachedGridSpan(child, ForRows);
+    LayoutUnit gridAreaBreadth = 0;
+    for (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); ++trackPosition) {

[svillar, 2015/12/01 12:43:14]

Better do:

for (auto trackPosition : span)

since GridSpan is iteratable

[jfernandez, 2015/12/01 15:13:39]

Acknowledged.

+        const GridLength& maxTrackBreadth = gridTrackSize(ForRows, trackPosition.toInt()).maxTrackBreadth();
+        if (gridLengthIsIndefinite(maxTrackBreadth, ForRows)) {
+            gridAreaBreadth = child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
+            break;
+        }
+        gridAreaBreadth += valueForLength(maxTrackBreadth.length(), 0);
+    }
+
+    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;
+    if (child.isHorizontalWritingMode() != isHorizontalWritingMode() && tracks.isEmpty())

[svillar, 2015/12/01 12:43:14]

Don't get why you need the isEmpty().

[jfernandez, 2015/12/01 15:13:39]

I needed a way to detect whether row tracks were already set when computing
column tracks. The only/easier way was to avoid initializing the row tracks
vector. That's why I use "isEmpty".

+        return assumedRowsBreadthForOrthogonalChild(child);
     const GridSpan& span = cachedGridSpan(child, direction);
     LayoutUnit gridAreaBreadth = 0;
     for (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); ++trackPosition)
@@ -1569,7 +1640,7 @@ void LayoutGrid::populateGridPositions(GridSizingData& sizingData)
     LayoutUnit trackGap = guttersSize(ForColumns, 2);
     m_columnPositions.resize(numberOfLines);
     m_columnPositions[0] = borderAndPaddingStart() + offset.positionOffset;
-    for (unsigned i = 0; i < lastLine; ++i)
+    for (unsigned i = 0; i < nextToLastLine; ++i)

[cbiesinger, 2015/11/26 02:26:16]

Why this change now?

[jfernandez, 2015/11/26 08:58:27]

I admit that this change is unrelated to the issue, but it's a silly mistake we
made long time ago and,. being one line, I  like to keep it. The mistake didn't
cause a bug because the same value is overwritten in the next line, but I wanted
to avoid doing an unnecessary lopp iteration.

         m_columnPositions[i + 1] = m_columnPositions[i] + offset.distributionOffset + sizingData.columnTracks[i].baseSize() + trackGap;
     m_columnPositions[lastLine] = m_columnPositions[nextToLastLine] + sizingData.columnTracks[nextToLastLine].baseSize();
 
@@ -1581,7 +1652,7 @@ void LayoutGrid::populateGridPositions(GridSizingData& sizingData)
     trackGap = guttersSize(ForRows, 2);
     m_rowPositions.resize(numberOfLines);
     m_rowPositions[0] = borderAndPaddingBefore() + offset.positionOffset;
-    for (unsigned i = 0; i < lastLine; ++i)
+    for (unsigned i = 0; i < nextToLastLine; ++i)
         m_rowPositions[i + 1] = m_rowPositions[i] + offset.distributionOffset + sizingData.rowTracks[i].baseSize() + trackGap;
     m_rowPositions[lastLine] = m_rowPositions[nextToLastLine] + sizingData.rowTracks[nextToLastLine].baseSize();
 }
@@ -2033,8 +2104,9 @@ ContentAlignmentData LayoutGrid::computeContentPositionAndDistributionOffset(Gri
 
 LayoutPoint LayoutGrid::findChildLogicalPosition(const LayoutBox& child, GridSizingData& sizingData) const
 {
+    LayoutUnit columnAxisOffset = columnAxisOffsetForChild(child);
     LayoutUnit rowAxisOffset = rowAxisOffsetForChild(child);
-    // We stored m_columnPosition s's data ignoring the direction, hence we might need now
+    // We stored m_columnPositions'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();
@@ -2042,7 +2114,13 @@ LayoutPoint LayoutGrid::findChildLogicalPosition(const LayoutBox& child, GridSiz
         rowAxisOffset = rightGridEdgePosition - (rowAxisOffset + child.logicalWidth());
     }
 
-    return LayoutPoint(rowAxisOffset, columnAxisOffsetForChild(child));
+    // "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);
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    return hasOrthogonalWritingMode ? childLocation.transposedPoint() : childLocation;
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const