[CSS Grid Layout] Fix positioning of grid items with margins
authorsvillar@igalia.com <svillar@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Nov 2013 08:43:07 +0000 (08:43 +0000)
committersvillar@igalia.com <svillar@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Nov 2013 08:43:07 +0000 (08:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=124345

Reviewed by David Hyatt.

From Blink r157925 and r158041 by <jchaffraix@chromium.org>

Source/WebCore:

Test: fast/css-grid-layout/grid-item-margin-resolution.html

Adds margin start/before to the positions of grid items (removing
several FIXME's in the current code). This means calling
findChildLogicalPosition() after the layout in order to have the
right values for the margins.

In order to match flexbox and author's intents we're also
including the margins of grid items in the intrinsic size of the
grid. That's why flexbox's marginLogicalPositionForChild() is
moved up to RenderBlock in order to share it with RenderGrid.

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::marginIntrinsicLogicalWidthForChild): Moved
from RenderFlexibleBox::marginLogicalWidthForChild().
* rendering/RenderBlock.h:
* rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::computeIntrinsicLogicalWidths):
* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::computePreferredTrackWidth):
(WebCore::RenderGrid::layoutGridItems):
(WebCore::RenderGrid::findChildLogicalPosition):

LayoutTests:

New test case for grid items margin resolution. Extended the
preferred logical widths checks with grid items with margins.

* fast/css-grid-layout/grid-item-margin-resolution-expected.txt: Added.
* fast/css-grid-layout/grid-item-margin-resolution.html: Added.
* fast/css-grid-layout/grid-preferred-logical-widths-expected.txt:
* fast/css-grid-layout/grid-preferred-logical-widths.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@159809 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/css-grid-layout/grid-item-margin-resolution-expected.txt [new file with mode: 0644]
LayoutTests/fast/css-grid-layout/grid-item-margin-resolution.html [new file with mode: 0644]
LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths-expected.txt
LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html [changed mode: 0644->0755]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderBlock.h
Source/WebCore/rendering/RenderFlexibleBox.cpp
Source/WebCore/rendering/RenderGrid.cpp

index a09a846..f8b87aa 100644 (file)
@@ -1,3 +1,20 @@
+2013-11-14  Sergio Villar Senin  <svillar@igalia.com>
+
+        [CSS Grid Layout] Fix positioning of grid items with margins
+        https://bugs.webkit.org/show_bug.cgi?id=124345
+
+        Reviewed by David Hyatt.
+
+        From Blink r157925 and r158041 by <jchaffraix@chromium.org>
+
+        New test case for grid items margin resolution. Extended the
+        preferred logical widths checks with grid items with margins.
+
+        * fast/css-grid-layout/grid-item-margin-resolution-expected.txt: Added.
+        * fast/css-grid-layout/grid-item-margin-resolution.html: Added.
+        * fast/css-grid-layout/grid-preferred-logical-widths-expected.txt:
+        * fast/css-grid-layout/grid-preferred-logical-widths.html:
+
 2013-11-26  Sergio Villar Senin  <svillar@igalia.com>
 
         [CSS Grid Layout] Support grid-definition-{rows|columns} repeat() syntax
diff --git a/LayoutTests/fast/css-grid-layout/grid-item-margin-resolution-expected.txt b/LayoutTests/fast/css-grid-layout/grid-item-margin-resolution-expected.txt
new file mode 100644 (file)
index 0000000..0acc469
--- /dev/null
@@ -0,0 +1,9 @@
+This test checks that the grid items are shifted by theirs before and start margins inside their grid area.
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
diff --git a/LayoutTests/fast/css-grid-layout/grid-item-margin-resolution.html b/LayoutTests/fast/css-grid-layout/grid-item-margin-resolution.html
new file mode 100644 (file)
index 0000000..5166ddf
--- /dev/null
@@ -0,0 +1,102 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner)
+    testRunner.overridePreference("WebKitCSSGridLayoutEnabled", 1);
+</script>
+<script src="../../resources/check-layout.js"></script>
+<link href="resources/grid.css" rel="stylesheet">
+<style>
+.grid {
+    -webkit-grid-definition-rows: 100px 100px;
+    -webkit-grid-definition-columns: 100px 100px;
+    width: 200px;
+}
+
+.gridItem {
+    margin: 20px 30px 40px 50px;
+    width: 20px;
+    height: 40px;
+}
+</style>
+</head>
+<body onload="checkLayout('.grid')">
+
+<div>This test checks that the grid items are shifted by theirs before and start margins inside their grid area.</div>
+
+<div style="position: relative">
+    <div class="grid" data-expected-width="200" data-expected-height="200">
+        <div class="gridItem firstRowFirstColumn" data-offset-x="50" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem firstRowSecondColumn" data-offset-x="150" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowFirstColumn" data-offset-x="50" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowSecondColumn" data-offset-x="150" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+    </div>
+</div>
+
+<div style="position: relative">
+    <div class="grid verticalRL" data-expected-width="200" data-expected-height="200">
+        <div class="gridItem firstRowFirstColumn" data-offset-x="150" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem firstRowSecondColumn" data-offset-x="150" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowFirstColumn" data-offset-x="50" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowSecondColumn" data-offset-x="50" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+    </div>
+</div>
+
+<div style="position: relative">
+    <div class="grid verticalLR" data-expected-width="200" data-expected-height="200">
+        <div class="gridItem firstRowFirstColumn" data-offset-x="50" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem firstRowSecondColumn" data-offset-x="50" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowFirstColumn" data-offset-x="150" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowSecondColumn" data-offset-x="150" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+    </div>
+</div>
+
+<div style="position: relative">
+    <div class="grid horizontalBT" data-expected-width="200" data-expected-height="200">
+        <div class="gridItem firstRowFirstColumn" data-offset-x="50" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem firstRowSecondColumn" data-offset-x="150" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowFirstColumn" data-offset-x="50" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowSecondColumn" data-offset-x="150" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+    </div>
+</div>
+
+<!-- rtl direction -->
+<div style="position: relative">
+    <div class="grid directionRTL" data-expected-width="200" data-expected-height="200">
+        <div class="gridItem firstRowFirstColumn" data-offset-x="30" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem firstRowSecondColumn" data-offset-x="130" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowFirstColumn" data-offset-x="30" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowSecondColumn" data-offset-x="130" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+    </div>
+</div>
+
+<div style="position: relative">
+    <div class="grid verticalRL directionRTL" data-expected-width="200" data-expected-height="200">
+        <div class="gridItem firstRowFirstColumn" data-offset-x="150" data-offset-y="40" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem firstRowSecondColumn" data-offset-x="150" data-offset-y="140" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowFirstColumn" data-offset-x="50" data-offset-y="40" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowSecondColumn" data-offset-x="50" data-offset-y="140" data-expected-width="20" data-expected-height="40"></div>
+    </div>
+</div>
+
+<div style="position: relative">
+    <div class="grid verticalLR directionRTL" data-expected-width="200" data-expected-height="200">
+        <div class="gridItem firstRowFirstColumn" data-offset-x="50" data-offset-y="40" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem firstRowSecondColumn" data-offset-x="50" data-offset-y="140" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowFirstColumn" data-offset-x="150" data-offset-y="40" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowSecondColumn" data-offset-x="150" data-offset-y="140" data-expected-width="20" data-expected-height="40"></div>
+    </div>
+</div>
+
+<div style="position: relative">
+    <div class="grid horizontalBT directionRTL" data-expected-width="200" data-expected-height="200">
+        <div class="gridItem firstRowFirstColumn" data-offset-x="30" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem firstRowSecondColumn" data-offset-x="130" data-offset-y="120" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowFirstColumn" data-offset-x="30" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+        <div class="gridItem secondRowSecondColumn" data-offset-x="130" data-offset-y="20" data-expected-width="20" data-expected-height="40"></div>
+    </div>
+</div>
+
+</body>
+</html>
index 9d03c5e..6630ec2 100644 (file)
@@ -26,3 +26,21 @@ PASS
 XX XX XX
 XX XX XX
 PASS
+XX XX XX
+XX XX XX
+PASS
+XX XX XX
+XX XX XX
+PASS
+XXXXX XXXXX
+XXXXX XXXXX
+PASS
+XXXXX XXXXX
+XXXXX XXXXX
+PASS
+XX XX XX
+XX XX XX
+PASS
+XX XX XX
+XX XX XX
+PASS
old mode 100644 (file)
new mode 100755 (executable)
index 669560e..144e8e7
     -webkit-grid-definition-rows: 10px;
     font: 10px/1 Ahem;
 }
+
+.margins {
+    margin: 10px 20px 30px 40px;
+}
 </style>
 </head>
 <script src="../../resources/check-layout.js"></script>
     <div class="firstRowSecondColumn">XX XX XX</div>
 </div>
 
+<!-- Now with margin on one of the grid items. -->
+<div class="grid gridMinContentFixed min-content" data-expected-height="10" data-expected-width="100">
+    <div class="firstRowFirstColumn">XX XX XX</div>
+    <div class="firstRowSecondColumn margins">XX XX XX</div>
+</div>
+<div class="grid gridMinContentFixed max-content" data-expected-height="10" data-expected-width="120">
+    <div class="firstRowFirstColumn margins">XX XX XX</div>
+    <div class="firstRowSecondColumn">XX XX XX</div>
+</div>
+<div class="grid gridFixedMinContent min-content" data-expected-height="10" data-expected-width="60">
+    <div class="firstRowFirstColumn">XXXXX XXXXX</div>
+    <div class="firstRowSecondColumn margins">XXXXX XXXXX</div>
+</div>
+<div class="grid gridFixedMinContent max-content" data-expected-height="10" data-expected-width="160">
+    <div class="firstRowFirstColumn margins">XXXXX XXXXX</div>
+    <div class="firstRowSecondColumn">XXXXX XXXXX</div>
+</div>
+<div class="grid gridFixedMaxContent min-content" data-expected-height="10" data-expected-width="80">
+    <div class="firstRowFirstColumn">XX XX XX</div>
+    <div class="firstRowSecondColumn margins">XX XX XX</div>
+</div>
+<div class="grid gridFixedMaxContent max-content" data-expected-height="10" data-expected-width="220">
+    <div class="firstRowFirstColumn margins">XX XX XX</div>
+    <div class="firstRowSecondColumn">XX XX XX</div>
+</div>
+
 </body>
 </html>
index 713e3ab..08a26eb 100644 (file)
@@ -1,3 +1,35 @@
+2013-11-14  Sergio Villar Senin  <svillar@igalia.com>
+
+        [CSS Grid Layout] Fix positioning of grid items with margins
+        https://bugs.webkit.org/show_bug.cgi?id=124345
+
+        Reviewed by David Hyatt.
+
+        From Blink r157925 and r158041 by <jchaffraix@chromium.org>
+
+        Test: fast/css-grid-layout/grid-item-margin-resolution.html
+
+        Adds margin start/before to the positions of grid items (removing
+        several FIXME's in the current code). This means calling
+        findChildLogicalPosition() after the layout in order to have the
+        right values for the margins.
+
+        In order to match flexbox and author's intents we're also
+        including the margins of grid items in the intrinsic size of the
+        grid. That's why flexbox's marginLogicalPositionForChild() is
+        moved up to RenderBlock in order to share it with RenderGrid.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::marginIntrinsicLogicalWidthForChild): Moved
+        from RenderFlexibleBox::marginLogicalWidthForChild().
+        * rendering/RenderBlock.h:
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::computeIntrinsicLogicalWidths):
+        * rendering/RenderGrid.cpp:
+        (WebCore::RenderGrid::computePreferredTrackWidth):
+        (WebCore::RenderGrid::layoutGridItems):
+        (WebCore::RenderGrid::findChildLogicalPosition):
+
 2013-11-26  Sergio Villar Senin  <svillar@igalia.com>
 
         [CSS Grid Layout] Support grid-definition-{rows|columns} repeat() syntax
index 0298145..291a477 100644 (file)
@@ -2083,6 +2083,21 @@ void RenderBlock::markFixedPositionObjectForLayoutIfNeeded(RenderObject& child)
     }
 }
 
+LayoutUnit RenderBlock::marginIntrinsicLogicalWidthForChild(RenderBox& child) const
+{
+    // A margin has three types: fixed, percentage, and auto (variable).
+    // Auto and percentage margins become 0 when computing min/max width.
+    // Fixed margins can be added in as is.
+    Length marginLeft = child.style().marginStartUsing(&style());
+    Length marginRight = child.style().marginEndUsing(&style());
+    LayoutUnit margin = 0;
+    if (marginLeft.isFixed())
+        margin += marginLeft.value();
+    if (marginRight.isFixed())
+        margin += marginRight.value();
+    return margin;
+}
+
 void RenderBlock::layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly)
 {
     TrackedRendererListHashSet* positionedDescendants = positionedObjects();
index 38b456e..fb726dc 100644 (file)
@@ -409,6 +409,8 @@ protected:
     void layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly = false);
     void markFixedPositionObjectForLayoutIfNeeded(RenderObject& child);
 
+    LayoutUnit marginIntrinsicLogicalWidthForChild(RenderBox&) const;
+
     virtual void paint(PaintInfo&, const LayoutPoint&) OVERRIDE;
     virtual void paintObject(PaintInfo&, const LayoutPoint&) OVERRIDE;
     virtual void paintChildren(PaintInfo& forSelf, const LayoutPoint&, PaintInfo& forChild, bool usePrintRect);
index e161d33..6783023 100644 (file)
@@ -91,21 +91,6 @@ const char* RenderFlexibleBox::renderName() const
     return "RenderFlexibleBox";
 }
 
-static LayoutUnit marginLogicalWidthForChild(RenderBox& child, RenderStyle* parentStyle)
-{
-    // A margin has three types: fixed, percentage, and auto (variable).
-    // Auto and percentage margins become 0 when computing min/max width.
-    // Fixed margins can be added in as is.
-    Length marginLeft = child.style().marginStartUsing(parentStyle);
-    Length marginRight = child.style().marginEndUsing(parentStyle);
-    LayoutUnit margin = 0;
-    if (marginLeft.isFixed())
-        margin += marginLeft.value();
-    if (marginRight.isFixed())
-        margin += marginRight.value();
-    return margin;
-}
-
 void RenderFlexibleBox::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
 {
     // FIXME: We're ignoring flex-basis here and we shouldn't. We can't start honoring it though until
@@ -115,7 +100,7 @@ void RenderFlexibleBox::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidt
         if (child->isOutOfFlowPositioned())
             continue;
 
-        LayoutUnit margin = marginLogicalWidthForChild(*child, &style());
+        LayoutUnit margin = marginIntrinsicLogicalWidthForChild(*child);
         bool hasOrthogonalWritingMode = child->isHorizontalWritingMode() != isHorizontalWritingMode();
         LayoutUnit minPreferredLogicalWidth = hasOrthogonalWritingMode ? child->logicalHeight() : child->minPreferredLogicalWidth();
         LayoutUnit maxPreferredLogicalWidth = hasOrthogonalWritingMode ? child->logicalHeight() : child->maxPreferredLogicalWidth();
index b2eb353..f40fdac 100644 (file)
@@ -272,20 +272,18 @@ LayoutUnit RenderGrid::computePreferredTrackWidth(const GridLength& gridLength,
     if (length.isMinContent()) {
         LayoutUnit minContentSize = 0;
         GridIterator iterator(m_grid, ForColumns, trackIndex);
-        while (RenderBox* gridItem = iterator.nextGridItem()) {
-            // FIXME: We should include the child's fixed margins like RenderFlexibleBox.
-            minContentSize = std::max(minContentSize, gridItem->minPreferredLogicalWidth());
-        }
+        while (RenderBox* gridItem = iterator.nextGridItem())
+            minContentSize = std::max(minContentSize, gridItem->minPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(*gridItem));
+
         return minContentSize;
     }
 
     if (length.isMaxContent()) {
         LayoutUnit maxContentSize = 0;
         GridIterator iterator(m_grid, ForColumns, trackIndex);
-        while (RenderBox* gridItem = iterator.nextGridItem()) {
-            // FIXME: We should include the child's fixed margins like RenderFlexibleBox.
-            maxContentSize = std::max(maxContentSize, gridItem->maxPreferredLogicalWidth());
-        }
+        while (RenderBox* gridItem = iterator.nextGridItem())
+            maxContentSize = std::max(maxContentSize, gridItem->maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(*gridItem));
+
         return maxContentSize;
     }
 
@@ -785,15 +783,11 @@ void RenderGrid::layoutGridItems()
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForRows, sizingData.rowTracks));
 
     for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        LayoutPoint childPosition = findChildLogicalPosition(child, sizingData);
-
         // 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();
 
-        // FIXME: For children in a content sized track, we clear the overrideContainingBlockContentLogicalHeight
-        // in minContentForChild / maxContentForChild which means that we will always relayout the child.
         LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(child, ForColumns, sizingData.columnTracks);
         LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChild(child, ForRows, sizingData.rowTracks);
         if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && (child->hasRelativeLogicalHeight() || child->hasViewportPercentageLogicalHeight())))
@@ -809,8 +803,7 @@ void RenderGrid::layoutGridItems()
         // now, just size as if we were a regular child.
         child->layoutIfNeeded();
 
-        // FIXME: Handle border & padding on the grid element.
-        child->setLogicalLocation(childPosition);
+        child->setLogicalLocation(findChildLogicalPosition(child, sizingData));
 
         // If the child moved, we have to repaint it as well as any floating/positioned
         // descendants. An exception is if we need a layout. In this case, we know we're going to
@@ -1055,14 +1048,13 @@ LayoutPoint RenderGrid::findChildLogicalPosition(RenderBox* child, const GridSiz
     const GridCoordinate& coordinate = cachedGridCoordinate(child);
 
     // The grid items should be inside the grid container's border box, that's why they need to be shifted.
-    LayoutPoint offset(borderAndPaddingStart(), borderAndPaddingBefore());
+    LayoutPoint offset(borderAndPaddingStart() + marginStartForChild(*child), borderAndPaddingBefore() + marginBeforeForChild(*child));
     // FIXME: |columnTrack| and |rowTrack| should be smaller than our column / row count.
     for (size_t i = 0; i < coordinate.columns.initialPositionIndex && i < sizingData.columnTracks.size(); ++i)
         offset.setX(offset.x() + sizingData.columnTracks[i].m_usedBreadth);
     for (size_t i = 0; i < coordinate.rows.initialPositionIndex && i < sizingData.rowTracks.size(); ++i)
         offset.setY(offset.y() + sizingData.rowTracks[i].m_usedBreadth);
 
-    // FIXME: Handle margins on the grid item.
     return offset;
 }