[CSS Grid Layout] Setting height on a grid item doesn't have any effect
authorjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jun 2015 20:26:02 +0000 (20:26 +0000)
committerjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jun 2015 20:26:02 +0000 (20:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145604

Reviewed by Sergio Villar Senin.

Source/WebCore:

Box Alignment spec states that stretch is only possible when height is
'auto' and no 'auto' margins are used.

It might be the case that style changes so that stretching is not allowed,
hence we need to detect it and clear the override height the stretching
algorithm previously set. The new layout triggered by the style change
will then set grid item's height according to the new style rules.

Test: fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html

* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::applyStretchAlignmentToChildIfNeeded):

LayoutTests:

Tests to verify that we clear the override height set by the stretching logic
whenever height or margin change in a way they don't allow stretching anymore.

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

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@185327 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 [new file with mode: 0644]
LayoutTests/fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderGrid.cpp

index 52b9cfe..e4c661b 100644 (file)
@@ -1,3 +1,16 @@
+2015-06-08  Javier Fernandez  <jfernandez@igalia.com>
+
+        [CSS Grid Layout] Setting height on a grid item doesn't have any effect
+        https://bugs.webkit.org/show_bug.cgi?id=145604
+
+        Reviewed by Sergio Villar Senin.
+
+        Tests to verify that we clear the override height set by the stretching logic
+        whenever height or margin change in a way they don't allow stretching anymore.
+
+        * fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change-expected.txt: Added.
+        * fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html: Added.
+
 2015-06-08  Brady Eidson  <beidson@apple.com>
 
         Completely remove all IDB properties/constructors when it is disabled at runtime.
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
new file mode 100644 (file)
index 0000000..d950e0e
--- /dev/null
@@ -0,0 +1,4 @@
+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
new file mode 100644 (file)
index 0000000..7b4f30f
--- /dev/null
@@ -0,0 +1,37 @@
+<!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 9bce818..15a0747 100644 (file)
@@ -1,3 +1,23 @@
+2015-06-08  Javier Fernandez  <jfernandez@igalia.com>
+
+        [CSS Grid Layout] Setting height on a grid item doesn't have any effect
+        https://bugs.webkit.org/show_bug.cgi?id=145604
+
+        Reviewed by Sergio Villar Senin.
+
+        Box Alignment spec states that stretch is only possible when height is
+        'auto' and no 'auto' margins are used.
+
+        It might be the case that style changes so that stretching is not allowed,
+        hence we need to detect it and clear the override height the stretching
+        algorithm previously set. The new layout triggered by the style change
+        will then set grid item's height according to the new style rules.
+
+        Test: fast/css-grid-layout/grid-item-should-not-be-stretched-when-height-or-margin-change.html
+
+        * rendering/RenderGrid.cpp:
+        (WebCore::RenderGrid::applyStretchAlignmentToChildIfNeeded):
+
 2015-06-08  Brady Eidson  <beidson@apple.com>
 
         Completely remove all IDB properties/constructors when it is disabled at runtime.
index 3bd26a5..bc378a5 100644 (file)
@@ -1296,25 +1296,25 @@ LayoutUnit RenderGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUni
 // FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to RenderBox.
 void RenderGrid::applyStretchAlignmentToChildIfNeeded(RenderBox& child, LayoutUnit gridAreaBreadthForChild)
 {
-    if (RenderStyle::resolveAlignment(style(), child.style(), ItemPositionStretch) != ItemPositionStretch)
+    if (!allowedToStretchLogicalHeightForChild(child) || RenderStyle::resolveAlignment(style(), child.style(), ItemPositionStretch) != ItemPositionStretch) {
+        child.clearOverrideLogicalContentHeight();
         return;
+    }
 
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    if (allowedToStretchLogicalHeightForChild(child)) {
-        // FIXME: If the child has orthogonal flow, then it already has an override height set, so use it.
-        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-        if (!hasOrthogonalWritingMode) {
-            LayoutUnit stretchedLogicalHeight = availableAlignmentSpaceForChildBeforeStretching(gridAreaBreadthForChild, child);
-            LayoutUnit desiredLogicalHeight = child.constrainLogicalHeightByMinMax(stretchedLogicalHeight);
-
-            // FIXME: Can avoid laying out here in some cases. See https://webkit.org/b/87905.
-            bool childNeedsRelayout = desiredLogicalHeight != child.logicalHeight();
-            if (childNeedsRelayout || !child.hasOverrideLogicalContentHeight())
-                child.setOverrideLogicalContentHeight(desiredLogicalHeight - child.borderAndPaddingLogicalHeight());
-            if (childNeedsRelayout) {
-                child.setLogicalHeight(0);
-                child.setNeedsLayout();
-            }
+    // FIXME: If the child has orthogonal flow, then it already has an override height set, so use it.
+    // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
+    if (!hasOrthogonalWritingMode) {
+        LayoutUnit stretchedLogicalHeight = availableAlignmentSpaceForChildBeforeStretching(gridAreaBreadthForChild, child);
+        LayoutUnit desiredLogicalHeight = child.constrainLogicalHeightByMinMax(stretchedLogicalHeight);
+
+        // FIXME: Can avoid laying out here in some cases. See https://webkit.org/b/87905.
+        bool childNeedsRelayout = desiredLogicalHeight != child.logicalHeight();
+        if (childNeedsRelayout || !child.hasOverrideLogicalContentHeight())
+            child.setOverrideLogicalContentHeight(desiredLogicalHeight - child.borderAndPaddingLogicalHeight());
+        if (childNeedsRelayout) {
+            child.setLogicalHeight(0);
+            child.setNeedsLayout();
         }
     }
 }