[CSS Grid Layout] Don't need to reset auto-margins during grid items layout
authorjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 6 Oct 2015 19:23:52 +0000 (19:23 +0000)
committerjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 6 Oct 2015 19:23:52 +0000 (19:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=149764

Reviewed by Darin Adler.

Source/WebCore:

This patch implements a refactoring of the auto-margin alignment code for grid
items so it uses start/end and before/after margin logic terms.

I addition, it avoids resetting the auto-margin values, which requires an extra
layout, before applying the alignment logic.

No new tests because there is no behavior change.

* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::computeMarginLogicalHeightForChild): Computing margins if child needs layout.
(WebCore::RenderGrid::availableAlignmentSpaceForChildBeforeStretching):
(WebCore::RenderGrid::updateAutoMarginsInRowAxisIfNeeded): Using start/end logical margins.
(WebCore::RenderGrid::updateAutoMarginsInColumnAxisIfNeeded): Using before/after logical margins.
(WebCore::RenderGrid::columnAxisOffsetForChild): Just added comment.
(WebCore::RenderGrid::rowAxisOffsetForChild): Just added comment.

LayoutTests:

Removed a duplicated layout tests.

* fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change-expected.txt: Removed.
* fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html: Removed.

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

LayoutTests/ChangeLog
LayoutTests/fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change-expected.txt [deleted file]
LayoutTests/fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html [deleted file]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderGrid.cpp

index e4e603c..ab1bd04 100644 (file)
@@ -1,3 +1,15 @@
+2015-10-06  Javier Fernandez  <jfernandez@igalia.com>
+
+        [CSS Grid Layout] Don't need to reset auto-margins during grid items layout
+        https://bugs.webkit.org/show_bug.cgi?id=149764
+
+        Reviewed by Darin Adler.
+
+        Removed a duplicated layout tests.
+
+        * fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change-expected.txt: Removed.
+        * fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html: Removed.
+
 2015-10-02  Jon Honeycutt  <jhoneycutt@apple.com>
 
         Import some Blink layout tests.
 2015-10-02  Jon Honeycutt  <jhoneycutt@apple.com>
 
         Import some Blink layout tests.
diff --git a/LayoutTests/fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change-expected.txt b/LayoutTests/fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change-expected.txt
deleted file mode 100644 (file)
index d950e0e..0000000
+++ /dev/null
@@ -1,4 +0,0 @@
-The grids below had initially 'stretched' items, but we have changed 'height' and 'margin' to values which don't allow stretch. This test verifies that the layout algorithm properly detects such changes and clear the override height accordingly.
-
-PASS
-PASS
diff --git a/LayoutTests/fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html b/LayoutTests/fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html
deleted file mode 100644 (file)
index 7b4f30f..0000000
+++ /dev/null
@@ -1,37 +0,0 @@
-<!DOCTYPE HTML>
-<link href="resources/grid.css" rel="stylesheet">
-<script src="../../resources/check-layout.js"></script>
-<style>
-.grid {
-    -webkit-grid-template: 200px 200px / 200px 200px;
-    width: -webkit-fit-content;
-    position: relative;
-}
-#fromFixedHeight { height: 100px; }
-#fromMarginAuto { margin: auto; }
-</style>
-<p>The grids below had initially 'stretched' items, but we have changed 'height' and 'margin' to values which don't allow stretch. This test verifies that the layout algorithm properly detects such changes and clear the override height accordingly.</p>
-<div class="grid">
-    <div id="toFixedHeight" class="firstRowFirstColumn" data-expected-height="100"></div>
-    <div class="firstRowSecondColumn" data-expected-height="200"></div>
-    <div class="secondRowFirstColumn" data-expected-height="200"></div>
-    <div id="fromFixedHeight" class="secondRowSecondColumn" data-expected-height="200"></div>
-</div>
-<div class="grid">
-    <div id="toMarginAuto" class="firstRowFirstColumn" data-expected-height="100">
-        <div style="height: 100px"></div>
-    </div>
-    <div class="firstRowSecondColumn" data-expected-height="200"></div>
-    <div class="secondRowFirstColumn" data-expected-height="200"></div>
-    <div id="fromMarginAuto" class="secondRowSecondColumn" data-expected-height="200">
-        <div style="height: 100px"></div>
-    </div>
-</div>
-<script>
-document.body.offsetLeft;
-document.getElementById("fromFixedHeight").style.height = "auto";
-document.getElementById("toFixedHeight").style.height = "100px";
-document.getElementById("fromMarginAuto").style.margin = "0";
-document.getElementById("toMarginAuto").style.margin = "auto";
-checkLayout(".grid");
-</script>
index bf32427..010113f 100644 (file)
@@ -1,3 +1,26 @@
+2015-10-06  Javier Fernandez  <jfernandez@igalia.com>
+
+        [CSS Grid Layout] Don't need to reset auto-margins during grid items layout
+        https://bugs.webkit.org/show_bug.cgi?id=149764
+
+        Reviewed by Darin Adler.
+
+        This patch implements a refactoring of the auto-margin alignment code for grid
+        items so it uses start/end and before/after margin logic terms.
+
+        I addition, it avoids resetting the auto-margin values, which requires an extra
+        layout, before applying the alignment logic.
+
+        No new tests because there is no behavior change.
+
+        * rendering/RenderGrid.cpp:
+        (WebCore::RenderGrid::computeMarginLogicalHeightForChild): Computing margins if child needs layout.
+        (WebCore::RenderGrid::availableAlignmentSpaceForChildBeforeStretching):
+        (WebCore::RenderGrid::updateAutoMarginsInRowAxisIfNeeded): Using start/end logical margins.
+        (WebCore::RenderGrid::updateAutoMarginsInColumnAxisIfNeeded): Using before/after logical margins.
+        (WebCore::RenderGrid::columnAxisOffsetForChild): Just added comment.
+        (WebCore::RenderGrid::rowAxisOffsetForChild): Just added comment.
+
 2015-10-06  Tim Horton  <timothy_horton@apple.com>
 
         Tile map shows a green rect when threaded scrolling is disabled
 2015-10-06  Tim Horton  <timothy_horton@apple.com>
 
         Tile map shows a green rect when threaded scrolling is disabled
index f7f3664..91da341 100644 (file)
@@ -1270,8 +1270,6 @@ void RenderGrid::layoutGridItems()
             || ((!oldOverrideContainingBlockContentLogicalHeight || oldOverrideContainingBlockContentLogicalHeight.value() != overrideContainingBlockContentLogicalHeight)
                 && child->hasRelativeLogicalHeight()))
             child->setNeedsLayout(MarkOnlyThis);
             || ((!oldOverrideContainingBlockContentLogicalHeight || oldOverrideContainingBlockContentLogicalHeight.value() != overrideContainingBlockContentLogicalHeight)
                 && child->hasRelativeLogicalHeight()))
             child->setNeedsLayout(MarkOnlyThis);
-        else
-            resetAutoMarginsAndLogicalTopInColumnAxis(*child);
 
         child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
         child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
 
         child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
         child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
@@ -1412,9 +1410,24 @@ LayoutUnit RenderGrid::marginLogicalHeightForChild(const RenderBox& child) const
     return isHorizontalWritingMode() ? child.verticalMarginExtent() : child.horizontalMarginExtent();
 }
 
     return isHorizontalWritingMode() ? child.verticalMarginExtent() : child.horizontalMarginExtent();
 }
 
+LayoutUnit RenderGrid::computeMarginLogicalHeightForChild(const RenderBox& child) const
+{
+    if (!child.style().hasMargin())
+        return 0;
+
+    LayoutUnit marginBefore;
+    LayoutUnit marginAfter;
+    child.computeBlockDirectionMargins(this, marginBefore, marginAfter);
+
+    return marginBefore + marginAfter;
+}
+
 LayoutUnit RenderGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUnit gridAreaBreadthForChild, const RenderBox& child) const
 {
 LayoutUnit RenderGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUnit gridAreaBreadthForChild, const RenderBox& child) const
 {
-    return gridAreaBreadthForChild - marginLogicalHeightForChild(child);
+    // Because we want to avoid multiple layouts, stretching logic might be performed before
+    // children are laid out, so we can't use the child cached values. Hence, we need to
+    // compute margins in order to determine the available height before stretching.
+    return gridAreaBreadthForChild - (child.needsLayout() ? computeMarginLogicalHeightForChild(child) : marginLogicalHeightForChild(child));
 }
 
 // FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to RenderBox.
 }
 
 // FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to RenderBox.
@@ -1481,57 +1494,24 @@ bool RenderGrid::hasAutoMarginsInRowAxis(const RenderBox& child) const
 }
 
 // FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to RenderBox.
 }
 
 // FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to RenderBox.
-void RenderGrid::resetAutoMarginsAndLogicalTopInColumnAxis(RenderBox& child)
-{
-    if (hasAutoMarginsInColumnAxis(child) || child.needsLayout()) {
-        child.clearOverrideLogicalContentHeight();
-        child.updateLogicalHeight();
-        if (isHorizontalWritingMode()) {
-            if (child.style().marginTop().isAuto())
-                child.setMarginTop(0);
-            if (child.style().marginBottom().isAuto())
-                child.setMarginBottom(0);
-        } else {
-            if (child.style().marginLeft().isAuto())
-                child.setMarginLeft(0);
-            if (child.style().marginRight().isAuto())
-                child.setMarginRight(0);
-        }
-
-    }
-}
-
-// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to RenderBox.
 void RenderGrid::updateAutoMarginsInRowAxisIfNeeded(RenderBox& child)
 {
     ASSERT(!child.isOutOfFlowPositioned());
 void RenderGrid::updateAutoMarginsInRowAxisIfNeeded(RenderBox& child)
 {
     ASSERT(!child.isOutOfFlowPositioned());
-    ASSERT(child.overrideContainingBlockContentLogicalWidth());
 
     LayoutUnit availableAlignmentSpace = child.overrideContainingBlockContentLogicalWidth().value() - child.logicalWidth();
     if (availableAlignmentSpace <= 0)
         return;
 
 
     LayoutUnit availableAlignmentSpace = child.overrideContainingBlockContentLogicalWidth().value() - child.logicalWidth();
     if (availableAlignmentSpace <= 0)
         return;
 
-    bool isHorizontal = isHorizontalWritingMode();
-    Length topOrLeft = isHorizontal ? child.style().marginLeft() : child.style().marginTop();
-    Length bottomOrRight = isHorizontal ? child.style().marginRight() : child.style().marginBottom();
-    if (topOrLeft.isAuto() && bottomOrRight.isAuto()) {
-        if (isHorizontal) {
-            child.setMarginLeft(availableAlignmentSpace / 2);
-            child.setMarginRight(availableAlignmentSpace / 2);
-        } else {
-            child.setMarginTop(availableAlignmentSpace / 2);
-            child.setMarginBottom(availableAlignmentSpace / 2);
-        }
-    } else if (topOrLeft.isAuto()) {
-        if (isHorizontal)
-            child.setMarginLeft(availableAlignmentSpace);
-        else
-            child.setMarginTop(availableAlignmentSpace);
-    } else if (bottomOrRight.isAuto()) {
-        if (isHorizontal)
-            child.setMarginRight(availableAlignmentSpace);
-        else
-            child.setMarginBottom(availableAlignmentSpace);
+    const RenderStyle& parentStyle = style();
+    Length marginStart = child.style().marginStartUsing(&parentStyle);
+    Length marginEnd = child.style().marginEndUsing(&parentStyle);
+    if (marginStart.isAuto() && marginEnd.isAuto()) {
+        child.setMarginStart(availableAlignmentSpace / 2, &parentStyle);
+        child.setMarginEnd(availableAlignmentSpace / 2, &parentStyle);
+    } else if (marginStart.isAuto()) {
+        child.setMarginStart(availableAlignmentSpace, &parentStyle);
+    } else if (marginEnd.isAuto()) {
+        child.setMarginEnd(availableAlignmentSpace, &parentStyle);
     }
 }
 
     }
 }
 
@@ -1539,33 +1519,21 @@ void RenderGrid::updateAutoMarginsInRowAxisIfNeeded(RenderBox& child)
 void RenderGrid::updateAutoMarginsInColumnAxisIfNeeded(RenderBox& child)
 {
     ASSERT(!child.isOutOfFlowPositioned());
 void RenderGrid::updateAutoMarginsInColumnAxisIfNeeded(RenderBox& child)
 {
     ASSERT(!child.isOutOfFlowPositioned());
-    ASSERT(child.overrideContainingBlockContentLogicalHeight());
 
     LayoutUnit availableAlignmentSpace = child.overrideContainingBlockContentLogicalHeight().value() - child.logicalHeight();
     if (availableAlignmentSpace <= 0)
         return;
 
 
     LayoutUnit availableAlignmentSpace = child.overrideContainingBlockContentLogicalHeight().value() - child.logicalHeight();
     if (availableAlignmentSpace <= 0)
         return;
 
-    bool isHorizontal = isHorizontalWritingMode();
-    Length topOrLeft = isHorizontal ? child.style().marginTop() : child.style().marginLeft();
-    Length bottomOrRight = isHorizontal ? child.style().marginBottom() : child.style().marginRight();
-    if (topOrLeft.isAuto() && bottomOrRight.isAuto()) {
-        if (isHorizontal) {
-            child.setMarginTop(availableAlignmentSpace / 2);
-            child.setMarginBottom(availableAlignmentSpace / 2);
-        } else {
-            child.setMarginLeft(availableAlignmentSpace / 2);
-            child.setMarginRight(availableAlignmentSpace / 2);
-        }
-    } else if (topOrLeft.isAuto()) {
-        if (isHorizontal)
-            child.setMarginTop(availableAlignmentSpace);
-        else
-            child.setMarginLeft(availableAlignmentSpace);
-    } else if (bottomOrRight.isAuto()) {
-        if (isHorizontal)
-            child.setMarginBottom(availableAlignmentSpace);
-        else
-            child.setMarginRight(availableAlignmentSpace);
+    const RenderStyle& parentStyle = style();
+    Length marginBefore = child.style().marginBeforeUsing(&parentStyle);
+    Length marginAfter = child.style().marginAfterUsing(&parentStyle);
+    if (marginBefore.isAuto() && marginAfter.isAuto()) {
+        child.setMarginBefore(availableAlignmentSpace / 2, &parentStyle);
+        child.setMarginAfter(availableAlignmentSpace / 2, &parentStyle);
+    } else if (marginBefore.isAuto()) {
+        child.setMarginBefore(availableAlignmentSpace, &parentStyle);
+    } else if (marginAfter.isAuto()) {
+        child.setMarginAfter(availableAlignmentSpace, &parentStyle);
     }
 }
 
     }
 }
 
@@ -1681,6 +1649,7 @@ LayoutUnit RenderGrid::columnAxisOffsetForChild(const RenderBox& child) const
         unsigned childEndLine = coordinate.rows.resolvedFinalPosition.next().toInt();
         LayoutUnit endOfRow = m_rowPositions[childEndLine];
         LayoutUnit childBreadth = child.logicalHeight() + child.marginLogicalHeight();
         unsigned childEndLine = coordinate.rows.resolvedFinalPosition.next().toInt();
         LayoutUnit endOfRow = m_rowPositions[childEndLine];
         LayoutUnit childBreadth = child.logicalHeight() + child.marginLogicalHeight();
+        // In order to properly adjust the Self Alignment values we need to consider the offset between tracks.
         if (childEndLine - childStartLine > 1 && childEndLine < m_rowPositions.size() - 1)
             endOfRow -= offsetBetweenTracks(style().resolvedAlignContentDistribution(), m_rowPositions, childBreadth);
         LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(RenderStyle::resolveAlignmentOverflow(style(), child.style()), endOfRow - startOfRow, childBreadth);
         if (childEndLine - childStartLine > 1 && childEndLine < m_rowPositions.size() - 1)
             endOfRow -= offsetBetweenTracks(style().resolvedAlignContentDistribution(), m_rowPositions, childBreadth);
         LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(RenderStyle::resolveAlignmentOverflow(style(), child.style()), endOfRow - startOfRow, childBreadth);
@@ -1710,6 +1679,7 @@ LayoutUnit RenderGrid::rowAxisOffsetForChild(const RenderBox& child) const
         unsigned childEndLine = coordinate.columns.resolvedFinalPosition.next().toInt();
         LayoutUnit endOfColumn = m_columnPositions[childEndLine];
         LayoutUnit childBreadth = child.logicalWidth() + child.marginLogicalWidth();
         unsigned childEndLine = coordinate.columns.resolvedFinalPosition.next().toInt();
         LayoutUnit endOfColumn = m_columnPositions[childEndLine];
         LayoutUnit childBreadth = child.logicalWidth() + child.marginLogicalWidth();
+        // In order to properly adjust the Self Alignment values we need to consider the offset between tracks.
         if (childEndLine - childStartLine > 1 && childEndLine < m_columnPositions.size() - 1)
             endOfColumn -= offsetBetweenTracks(style().resolvedJustifyContentDistribution(), m_columnPositions, childBreadth);
         LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(RenderStyle::resolveJustificationOverflow(style(), child.style()), endOfColumn - startOfColumn, childBreadth);
         if (childEndLine - childStartLine > 1 && childEndLine < m_columnPositions.size() - 1)
             endOfColumn -= offsetBetweenTracks(style().resolvedJustifyContentDistribution(), m_columnPositions, childBreadth);
         LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(RenderStyle::resolveJustificationOverflow(style(), child.style()), endOfColumn - startOfColumn, childBreadth);