[CSS Grid Layout] Partial implementation of justify-content for Grid.

Source/core/rendering/RenderGrid.cpp

Index: Source/core/rendering/RenderGrid.cpp
diff --git a/Source/core/rendering/RenderGrid.cpp b/Source/core/rendering/RenderGrid.cpp
index 11474d6b9153b483983e0f05f4bbff8f4b23fdc0..792f8487f151d14fd509abf98f103ae2ca87c1dd 100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -1087,13 +1087,15 @@ void RenderGrid::layoutGridItems()
 {
     placeItemsOnGrid();
 
+    LayoutUnit availableSpaceForColumns = availableLogicalWidth();
+    LayoutUnit availableSpaceForRows = availableLogicalHeight(IncludeMarginBorderPadding);
     GridSizingData sizingData(gridColumnCount(), gridRowCount());
-    computeUsedBreadthOfGridTracks(ForColumns, sizingData);
+    computeUsedBreadthOfGridTracks(ForColumns, sizingData, availableSpaceForColumns);
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData.columnTracks));
-    computeUsedBreadthOfGridTracks(ForRows, sizingData);
+    computeUsedBreadthOfGridTracks(ForRows, sizingData, availableSpaceForRows);
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForRows, sizingData.rowTracks));
 
-    populateGridPositions(sizingData);
+    populateGridPositions(sizingData, availableSpaceForColumns, availableSpaceForRows);
     m_gridItemsOverflowingGridArea.resize(0);
 
     for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
@@ -1161,10 +1163,13 @@ LayoutUnit RenderGrid::gridAreaBreadthForChild(const RenderBox& child, GridTrack
     return gridAreaBreadth;
 }
 
-void RenderGrid::populateGridPositions(const GridSizingData& sizingData)
+void RenderGrid::populateGridPositions(const GridSizingData& sizingData, LayoutUnit availableSpaceForColumns, LayoutUnit availableSpaceForRows)
 {
-    m_columnPositions.resize(sizingData.columnTracks.size() + 1);
-    m_columnPositions[0] = borderAndPaddingStart();
+    unsigned numberOfColumnTracks = sizingData.columnTracks.size();
+    LayoutUnit columnOffset = contentPositionAndDistributionOffset(availableSpaceForColumns, style()->justifyContent(), style()->justifyContentDistribution(), numberOfColumnTracks);
+
+    m_columnPositions.resize(numberOfColumnTracks + 1);
+    m_columnPositions[0] = borderAndPaddingStart() + columnOffset;
     for (size_t i = 0; i < m_columnPositions.size() - 1; ++i)
         m_columnPositions[i + 1] = m_columnPositions[i] + sizingData.columnTracks[i].m_usedBreadth;
 
@@ -1496,6 +1501,87 @@ LayoutUnit RenderGrid::rowPositionForChild(const RenderBox& child) const
     return 0;
 }
 
+LayoutUnit static contentDistributionOffset(LayoutUnit availableFreeSpace, ContentPosition& fallbackPosition, ContentDistributionType distribution, unsigned numberOfItems)

[Julien - ping for review, 2014/11/13 01:37:24]

Note that numberOfItems is not great as it could refer to the number of *grid*
items.

[jfernandez, 2014/11/13 12:22:35]

Well, I tried to use the term defined in the Box Alignment spec, which was
intended to be generic for good reasons. It's true that in the case of grid
layout. where we decided to interpret the alignment subject as tracks, instead
of grid items which is used n the rest of the alignment properties, it might be
confusing. 

I'm not sure what to do; from one hand, think is useful to use the same terms
and the reference specification document, but I agree in this particular case it
may lead to confusion. Perhaps 'numberOfTracks' would be better in this case.

[Julien - ping for review, 2014/11/13 16:43:31]

numberOfTracks is a good name, even better numberOfGridTrack

[jfernandez, 2014/11/14 15:32:32]

Done.

+{
+    ASSERT(numberOfItems > 0);
+
+    switch (distribution) {
+    case ContentDistributionSpaceBetween:
+        if (fallbackPosition == ContentPositionAuto)
+            fallbackPosition = ContentPositionStart;
+        // FIXME: for the time being, spec states that it will always fallback for Grids, but
+        // discussion is ongoing.
+        return -1;
+    case ContentDistributionSpaceAround:
+        if (fallbackPosition == ContentPositionAuto)
+            fallbackPosition = ContentPositionCenter;
+        // FIXME: for the time being, spec states that it will always fallback for Grids, but
+        // discussion is ongoing.
+        return -1;
+    case ContentDistributionSpaceEvenly:
+        if (fallbackPosition == ContentPositionAuto)
+            fallbackPosition = ContentPositionCenter;
+        // FIXME: for the time being, spec states that it will always fallback for Grids, but
+        // discussion is ongoing.
+        return -1;
+    case ContentDistributionStretch:
+        if (fallbackPosition == ContentPositionAuto)
+            fallbackPosition = ContentPositionStart;
+        // FIXME: for the time being, spec states that it will always fallback for Grids, but
+        // discussion is ongoing.
+        return -1;
+    case ContentDistributionDefault:
+        return -1;
+    default:
+        ASSERT_NOT_REACHED();
+        return -1;
+    }
+
+    return -1;
+}
+
+LayoutUnit RenderGrid::contentPositionAndDistributionOffset(LayoutUnit availableFreeSpace, ContentPosition position, ContentDistributionType distribution, unsigned numberOfItems) const
+{
+    LayoutUnit offset = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfItems);

[Julien - ping for review, 2014/11/13 01:37:24]

This function is stubbed so it's hard to see what purpose it serves. The 'auto'
resolution should be its own function as it would make the code more readable
and remove the need for the mostly stubbed function.

[jfernandez, 2014/11/13 12:22:35]

Acknowledged.

+    if (offset >= 0)
+        return offset;
+
+    switch (position) {
+    case ContentPositionLeft:
+        // If the property's axis is not parallel with the inline axis, this is equivalent to ‘start’.
+        if (!isHorizontalWritingMode())
+            return 0;
+        if (style()->isLeftToRightDirection())
+            return 0;
+        return availableFreeSpace;

[Julien - ping for review, 2014/11/13 01:37:24]

I think this should account for the children's size or else we won't have the
content truly justified as we want (think of center which doesn't seem useful).

[jfernandez, 2014/11/13 12:22:35]

I don't understand what you mean here; content alignment should not alter the
grid item's size. As far as I understand the spec, it just changes the position
of the grid tracks to the available empty space is redistributed. In the case of
<content-position> values such distribution happens outside the grid tracks
boundaries, while <content-distribution> affects both, external and internal
boundaries. 

There is only one exception, the 'stretch' <content-distribution> value, which
is under discussion in in the w3c mailing list with many doubts. Actually, grid
is supposed to fallaback all the content-distribution values. I'm working on a
possible approach, though, which may lead to a change in the specification to
include those values in certain cases.

[Julien - ping for review, 2014/11/13 16:43:31]

I didn't mean as we should change the grid items' sizes. If you want to justify
your grid items, you need to account for their sizes, else the position is going
to be wrong. Available width / height only return the content width / height and
don't remove any space taken by the children.
ACK!

[jfernandez, 2014/11/14 15:32:32]

I'm not sure whether you notice that availableSpaceForColumns is initialized to
availableLogicalWidth and then passed to the computeUsedBreadthOfGridTracks
method, which indeed remove the space taken by the children (in case of
contentSized tracks) or the tracks themselves, which is in the end what we want
to align with these properties.

Alignment specs state that for Grids the alignment subject is the grid
boundaries, which  I understand it's are the grid tracks. Except in the cases
where grid item's size overflows, I think it does not affect to the content
alignment logic. Overflow should be handled by the 'overflow' keyword, as it
happens with the other alignment properties. Other than that I think we should
do nothing about overflow, regarding alignment.

[Julien - ping for review, 2014/11/18 16:14:36]

I missed that. With that fact, the logic makes more sense.
AFAICT we don't support the 'overflow' value yet but that's fine. We should have
FIXME about it though.

[jfernandez, 2014/11/20 11:59:39]

Done.

+    case ContentPositionRight:
+        // If the property's axis is not parallel with the inline axis, this is equivalent to ‘start’.
+        if (!isHorizontalWritingMode())
+            return 0;
+        if (style()->isLeftToRightDirection())
+            return availableFreeSpace;
+        return 0;
+    case ContentPositionCenter:
+        return availableFreeSpace / 2;
+    case ContentPositionFlexEnd:
+        // Only used in flex layout, for other layout, it's equivalent to 'End'.
+    case ContentPositionEnd:
+        return availableFreeSpace;
+    case ContentPositionFlexStart:
+        // Only used in flex layout, for other layout, it's equivalent to 'Start'.
+    case ContentPositionStart:
+        return 0;
+    case ContentPositionBaseline:
+    case ContentPositionLastBaseline:
+        // FIXME: Implement the previous values. For now, we always start align.
+        return 0;
+    default:

[Julien - ping for review, 2014/11/13 01:37:24]

We seems to be missing one case ContentPositionAuto so it would be better to
just implement it. The compiler would automatically detect new cases for us if
we did that.

[jfernandez, 2014/11/13 12:22:35]

Auto value is resolved by the StyleAdjuster. It could be kept, though, when
<content-distribution> values are used instead. What we can do, though, is to
move the fallback position resolution to the StyleAdjuster as well; I'll think
about it if you agree.

[Julien - ping for review, 2014/11/13 16:43:31]

I don't think StyleAdjuster would work as the computed value should be the
specified value. The comment was just about how this logic is structured not
about changing it completely. Having the explicitly missing value would make the
code more future-proof.

[jfernandez, 2014/11/14 15:32:32]

Acknowledged.

+        ASSERT_NOT_REACHED();
+    }
+
+    return 0;
+}
+
 LayoutPoint RenderGrid::findChildLogicalPosition(const RenderBox& child) const
 {
     return LayoutPoint(columnPositionForChild(child), rowPositionForChild(child));