[css-grid] Margin wrong applied when stretching an orthogonal item in fixed size...
authorjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Jun 2017 09:08:15 +0000 (09:08 +0000)
committerjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Jun 2017 09:08:15 +0000 (09:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172590

Reviewed by Sergio Villar Senin.

LayoutTests/imported/w3c:

All the test cases of these tests pass with this change, so updating their expectations accordingly.

* web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-006-expected.txt:
* web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-vertical-lr-006-expected.txt:
* web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-vertical-rl-006-expected.txt:

Source/WebCore:

We need to consider orthogonality when using the item's logical margin to
compute the available space for stretching.

The issue this patch fixes is only reproducible when the grid layout logic
is executed several times, since probably the item doesn't need to be
laid out again. In such cases, we just get the cached logical margins
but we were not taking orthogonality into account.

Test: fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts.html

* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::layoutBlock):
(WebCore::RenderGrid::marginLogicalSizeForChild):
(WebCore::RenderGrid::availableAlignmentSpaceForChildBeforeStretching):
* rendering/RenderGrid.h:

LayoutTests:

* TestExpectations: 2 tests pass now but 3 more fail because of bug #172836
* fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts-expected.html: Added.
* fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts-expected.html [new file with mode: 0644]
LayoutTests/fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts.html [new file with mode: 0644]
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-006-expected.txt
LayoutTests/imported/w3c/web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-vertical-lr-006-expected.txt
LayoutTests/imported/w3c/web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-vertical-rl-006-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderGrid.cpp
Source/WebCore/rendering/RenderGrid.h

index cded69f..440b4f7 100644 (file)
@@ -1,3 +1,14 @@
+2017-06-02  Javier Fernandez  <jfernandez@igalia.com>
+
+        [css-grid] Margin wrong applied when stretching an orthogonal item in fixed size track
+        https://bugs.webkit.org/show_bug.cgi?id=172590
+
+        Reviewed by Sergio Villar Senin.
+
+        * TestExpectations: 2 tests pass now but 3 more fail because of bug #172836
+        * fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts-expected.html: Added.
+        * fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts.html: Added.
+
 2017-06-02  Zan Dobersek  <zdobersek@igalia.com>
 
         [WPE] Enable SUBTLE_CRYPTO
index c172bdd..d6c5a7c 100644 (file)
@@ -284,10 +284,11 @@ webkit.org/b/136754 css3/flexbox/csswg/ttwf-reftest-flex-wrap.html [ ImageOnlyFa
 # grid layout tests
 webkit.org/b/165062 fast/css-grid-layout/grid-baseline.html [ ImageOnlyFailure ]
 webkit.org/b/165062 fast/css-grid-layout/grid-baseline-margins.html [ ImageOnlyFailure ]
-webkit.org/b/172590 imported/w3c/web-platform-tests/css/css-grid-1/abspos/orthogonal-positioned-grid-items-013.html [ ImageOnlyFailure ]
-webkit.org/b/172590 imported/w3c/web-platform-tests/css/css-grid-1/abspos/orthogonal-positioned-grid-items-017.html [ ImageOnlyFailure ]
 webkit.org/b/149891 imported/w3c/web-platform-tests/css/css-grid-1/grid-layout-properties.html [ Failure ]
 webkit.org/b/169271 imported/w3c/web-platform-tests/css/css-grid-1/grid-items/grid-items-sizing-alignment-001.html [ ImageOnlyFailure ]
+webkit.org/b/172836 imported/w3c/web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-014.html [ Failure ]
+webkit.org/b/172836 imported/w3c/web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-vertical-rl-014.html [ Failure ]
+webkit.org/b/172836 imported/w3c/web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-vertical-lr-014.html [ Failure ]
 
 # selectors4
 webkit.org/b/64861 imported/w3c/web-platform-tests/css/selectors4/selectors-dir-selector-ltr-001.html [ ImageOnlyFailure ]
diff --git a/LayoutTests/fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts-expected.html b/LayoutTests/fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts-expected.html
new file mode 100644 (file)
index 0000000..fbbab3b
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<style>
+body { overflow: hidden; } /* Ensure there wont be scrollbar to avoid the extra layout. */
+.grid {
+  display: inline-grid;
+  border: solid thick;
+  grid: 100px / 100px;
+  align-items: start;
+}
+.item {
+  background: magenta;
+  writing-mode: vertical-lr;
+  margin-right: 25px;
+}
+</style>
+<p>Check whether stretching logic is not affected by multiple layouts becuase of the grid item's override height.<br>The test passes if there is a margin-right of 25px.</p>
+<div class="grid">
+  <div class="item">item</div>
+</div>
diff --git a/LayoutTests/fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts.html b/LayoutTests/fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts.html
new file mode 100644 (file)
index 0000000..16ec9a8
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<link rel="match" href="grid-item-stretching-must-not-depend-on-previous-layouts-expected.html">
+<style>
+.grid {
+  display: inline-grid;
+  border: solid thick;
+  grid: 100px / 100px;
+  align-items: start;
+}
+.item {
+  background: magenta;
+  writing-mode: vertical-lr;
+  margin-right: 25px;
+}
+</style>
+<p>Check whether stretching logic is not affected by multiple layouts becuase of the grid item's override height.<br>The test passes if there is a margin-right of 25px.</p>
+<div class="grid">
+  <div class="item">item</div>
+</div>
index e685cc6..8d33581 100644 (file)
@@ -1,3 +1,16 @@
+2017-06-02  Javier Fernandez  <jfernandez@igalia.com>
+
+        [css-grid] Margin wrong applied when stretching an orthogonal item in fixed size track
+        https://bugs.webkit.org/show_bug.cgi?id=172590
+
+        Reviewed by Sergio Villar Senin.
+
+        All the test cases of these tests pass with this change, so updating their expectations accordingly.
+
+        * web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-006-expected.txt:
+        * web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-vertical-lr-006-expected.txt:
+        * web-platform-tests/css/css-grid-1/alignment/grid-self-alignment-stretch-vertical-rl-006-expected.txt:
+
 2017-06-01  Javier Fernandez  <jfernandez@igalia.com>
 
         [css-grid] Update W3C web platform tests for the CSS Grid Layout feature
index 3e1b5f1..003aff1 100644 (file)
@@ -9,12 +9,5 @@ X XXX
 X
 XX XXX
 
-FAIL .grid 1 assert_equals: 
-<div class="grid">
-  <div data-offset-x="0" data-offset-y="0" data-expected-width="90" data-expected-height="60" class="firstRowFirstColumn">X XX X</div>
-  <div data-offset-x="100" data-offset-y="0" data-expected-width="40" data-expected-height="130" class="firstRowSecondColumn">XX X<br>X XXX<br>X<br>XX XXX</div>
-  <div data-offset-x="0" data-offset-y="150" data-expected-width="10" data-expected-height="60" class="secondRowFirstColumn">X XX X</div>
-  <div data-offset-x="100" data-offset-y="150" data-expected-width="130" data-expected-height="90" class="secondRowSecondColumn">XX X<br>X XXX<br>X<br>XX XXX</div>
-</div>
-width expected 90 but got 100
+PASS .grid 1 
 
index 6c93af9..003aff1 100644 (file)
@@ -9,12 +9,5 @@ X XXX
 X
 XX XXX
 
-FAIL .grid 1 assert_equals: 
-<div class="grid">
-  <div data-offset-x="0" data-offset-y="0" data-expected-width="60" data-expected-height="90" class="firstRowFirstColumn">X XX X</div>
-  <div data-offset-x="0" data-offset-y="100" data-expected-width="130" data-expected-height="40" class="firstRowSecondColumn">XX X<br>X XXX<br>X<br>XX XXX</div>
-  <div data-offset-x="150" data-offset-y="0" data-expected-width="60" data-expected-height="10" class="secondRowFirstColumn">X XX X</div>
-  <div data-offset-x="150" data-offset-y="100" data-expected-width="90" data-expected-height="130" class="secondRowSecondColumn">XX X<br>X XXX<br>X<br>XX XXX</div>
-</div>
-height expected 90 but got 100
+PASS .grid 1 
 
index 34d8e3f..003aff1 100644 (file)
@@ -9,12 +9,5 @@ X XXX
 X
 XX XXX
 
-FAIL .grid 1 assert_equals: 
-<div class="grid">
-  <div data-offset-x="190" data-offset-y="0" data-expected-width="60" data-expected-height="90" class="firstRowFirstColumn">X XX X</div>
-  <div data-offset-x="100" data-offset-y="100" data-expected-width="130" data-expected-height="40" class="firstRowSecondColumn">XX X<br>X XXX<br>X<br>XX XXX</div>
-  <div data-offset-x="40" data-offset-y="0" data-expected-width="60" data-expected-height="10" class="secondRowFirstColumn">X XX X</div>
-  <div data-offset-x="0" data-offset-y="100" data-expected-width="90" data-expected-height="130" class="secondRowSecondColumn">XX X<br>X XXX<br>X<br>XX XXX</div>
-</div>
-height expected 90 but got 100
+PASS .grid 1 
 
index fe0c27e..a10fd24 100644 (file)
@@ -1,3 +1,26 @@
+2017-06-02  Javier Fernandez  <jfernandez@igalia.com>
+
+        [css-grid] Margin wrong applied when stretching an orthogonal item in fixed size track
+        https://bugs.webkit.org/show_bug.cgi?id=172590
+
+        Reviewed by Sergio Villar Senin.
+
+        We need to consider orthogonality when using the item's logical margin to
+        compute the available space for stretching.
+
+        The issue this patch fixes is only reproducible when the grid layout logic
+        is executed several times, since probably the item doesn't need to be
+        laid out again. In such cases, we just get the cached logical margins
+        but we were not taking orthogonality into account.
+
+        Test: fast/css-grid-layout/grid-item-stretching-must-not-depend-on-previous-layouts.html
+
+        * rendering/RenderGrid.cpp:
+        (WebCore::RenderGrid::layoutBlock):
+        (WebCore::RenderGrid::marginLogicalSizeForChild):
+        (WebCore::RenderGrid::availableAlignmentSpaceForChildBeforeStretching):
+        * rendering/RenderGrid.h:
+
 2017-06-01  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK] Cache RenderThemeGadget hierarchies for rendering themed elements with GTK+ 3.20+
index 6193848..d82791b 100644 (file)
@@ -1135,10 +1135,9 @@ static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, Lay
     return 0;
 }
 
-// FIXME: This logic is shared by RenderFlexibleBox, so it should be moved to RenderBox.
-LayoutUnit RenderGrid::marginLogicalHeightForChild(const RenderBox& child) const
+LayoutUnit RenderGrid::marginLogicalSizeForChild(GridTrackSizingDirection direction, const RenderBox& child) const
 {
-    return isHorizontalWritingMode() ? child.verticalMarginExtent() : child.horizontalMarginExtent();
+    return flowAwareDirectionForChild(child, direction) == ForColumns ? child.marginLogicalWidth() : child.marginLogicalHeight();
 }
 
 LayoutUnit RenderGrid::computeMarginLogicalSizeForChild(GridTrackSizingDirection direction, const RenderBox& child) const
@@ -1161,7 +1160,8 @@ LayoutUnit RenderGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUni
     // 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() ? computeMarginLogicalSizeForChild(ForRows, child) : marginLogicalHeightForChild(child));
+    GridTrackSizingDirection childBlockFlowDirection = flowAwareDirectionForChild(child, ForRows);
+    return gridAreaBreadthForChild - (child.needsLayout() ? computeMarginLogicalSizeForChild(childBlockFlowDirection, child) : marginLogicalSizeForChild(childBlockFlowDirection, child));
 }
 
 StyleSelfAlignmentData RenderGrid::alignSelfForChild(const RenderBox& child) const
index f7732c9..d3c4946 100644 (file)
@@ -130,7 +130,7 @@ private:
     void applyStretchAlignmentToTracksIfNeeded(GridTrackSizingDirection);
 
     void paintChildren(PaintInfo& forSelf, const LayoutPoint& paintOffset, PaintInfo& forChild, bool usePrintRect) override;
-    LayoutUnit marginLogicalHeightForChild(const RenderBox&) const;
+    LayoutUnit marginLogicalSizeForChild(GridTrackSizingDirection, const RenderBox&) const;
     LayoutUnit computeMarginLogicalSizeForChild(GridTrackSizingDirection, const RenderBox&) const;
     LayoutUnit availableAlignmentSpaceForChildBeforeStretching(LayoutUnit gridAreaBreadthForChild, const RenderBox&) const;
     StyleSelfAlignmentData justifySelfForChild(const RenderBox&) const;