[CSS Grid Layout] Fix missing layout in flexible and content sized columns
authorrego@igalia.com <rego@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Feb 2014 10:57:43 +0000 (10:57 +0000)
committerrego@igalia.com <rego@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Feb 2014 10:57:43 +0000 (10:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=128672

Reviewed by Sergio Villar Senin.

Source/WebCore:

RenderGrid::logicalContentHeightForChild() is called for some items at the beginning of RenderGrid::layoutGridItems()
from RenderGrid::computeUsedBreadthOfGridTracks(). This causes that the comparison inside the for loop in
RenderGrid::layoutGridItems() does not detect width changes, so elements won't be marked as needsLayout.

So the comparison is done in RenderGrid::logicalContentHeightForChild() and the element is marked to perform a layout if
the width has changed.

The issue can be reproduced easily with a simple grid with one flexible or content sized column, all the available width
is not used. On top of that, when you resize the window the flexible or content sized columns are not updating their
size properly.

CSS Grid Layout perftest results are around 4% worse, which is expected as we're adding a missing layout.

Test: fast/css-grid-layout/flex-content-sized-column-use-available-width.html

* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::logicalContentHeightForChild): Check width changes and mark element as needed layout if required.

LayoutTests:

Add test that reproduce the issue for both cases flexible and content sized columns.

* fast/css-grid-layout/flex-content-sized-column-use-available-width-expected.html: Added.
* fast/css-grid-layout/flex-content-sized-column-use-available-width.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/css-grid-layout/flex-content-sized-column-use-available-width-expected.html [new file with mode: 0644]
LayoutTests/fast/css-grid-layout/flex-content-sized-column-use-available-width.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderGrid.cpp

index 55b6cff..212160d 100644 (file)
@@ -1,3 +1,15 @@
+2014-02-17  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        [CSS Grid Layout] Fix missing layout in flexible and content sized columns
+        https://bugs.webkit.org/show_bug.cgi?id=128672
+
+        Reviewed by Sergio Villar Senin.
+
+        Add test that reproduce the issue for both cases flexible and content sized columns.
+
+        * fast/css-grid-layout/flex-content-sized-column-use-available-width-expected.html: Added.
+        * fast/css-grid-layout/flex-content-sized-column-use-available-width.html: Added.
+
 2014-02-15  Filip Pizlo  <fpizlo@apple.com>
 
         FTL should inline polymorphic heap accesses
diff --git a/LayoutTests/fast/css-grid-layout/flex-content-sized-column-use-available-width-expected.html b/LayoutTests/fast/css-grid-layout/flex-content-sized-column-use-available-width-expected.html
new file mode 100644 (file)
index 0000000..b7705ad
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style type="text/css">
+        .item {
+            background-color: blue;
+        }
+    </style>
+</head>
+<body>
+    <h1>Description</h1>
+    <p>Flex and content sized column should use all the available width and you should not see the grid background (grey at the end).</p>
+    <h1>Grid 1 flex column</h1>
+    <div>
+        <div class="item">grid item</div>
+    </div>
+    <h1>Grid 1 auto column</h1>
+    <div>
+        <div class="item">
+            grid item grid item grid item grid item grid item grid item grid item grid item grid item grid item
+            grid item grid item grid item grid item grid item grid item grid item grid item grid item grid item
+            grid item grid item grid item grid item grid item grid item grid item grid item grid item grid item
+            grid item grid item grid item grid item grid item grid item grid item grid item grid item grid item
+        </div>
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/fast/css-grid-layout/flex-content-sized-column-use-available-width.html b/LayoutTests/fast/css-grid-layout/flex-content-sized-column-use-available-width.html
new file mode 100644 (file)
index 0000000..f1a3b53
--- /dev/null
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+        if (window.testRunner)
+            testRunner.overridePreference("WebKitCSSGridLayoutEnabled", 1);
+    </script>
+    <link href="resources/grid.css" rel="stylesheet">
+    <style type="text/css">
+        #grid-1 {
+            -webkit-grid-template-columns: 1fr;
+        }
+
+        #grid-2 {
+            -webkit-grid-template-columns: auto;
+        }
+    </style>
+</head>
+<body>
+    <h1>Description</h1>
+    <p>Flex and content sized column should use all the available width and you should not see the grid background (grey at the end).</p>
+    <h1>Grid 1 flex column</h1>
+    <div id="grid-1" class="grid">
+        <div class="firstRowFirstColumn">grid item</div>
+    </div>
+    <h1>Grid 1 auto column</h1>
+    <div id="grid-2" class="grid">
+        <div class="firstRowFirstColumn">
+            grid item grid item grid item grid item grid item grid item grid item grid item grid item grid item
+            grid item grid item grid item grid item grid item grid item grid item grid item grid item grid item
+            grid item grid item grid item grid item grid item grid item grid item grid item grid item grid item
+            grid item grid item grid item grid item grid item grid item grid item grid item grid item grid item
+        </div>
+    </div>
+</body>
+</html>
index 4ca9e14..487e157 100644 (file)
@@ -1,3 +1,28 @@
+2014-02-17  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        [CSS Grid Layout] Fix missing layout in flexible and content sized columns
+        https://bugs.webkit.org/show_bug.cgi?id=128672
+
+        Reviewed by Sergio Villar Senin.
+
+        RenderGrid::logicalContentHeightForChild() is called for some items at the beginning of RenderGrid::layoutGridItems()
+        from RenderGrid::computeUsedBreadthOfGridTracks(). This causes that the comparison inside the for loop in
+        RenderGrid::layoutGridItems() does not detect width changes, so elements won't be marked as needsLayout.
+
+        So the comparison is done in RenderGrid::logicalContentHeightForChild() and the element is marked to perform a layout if
+        the width has changed.
+
+        The issue can be reproduced easily with a simple grid with one flexible or content sized column, all the available width
+        is not used. On top of that, when you resize the window the flexible or content sized columns are not updating their
+        size properly.
+
+        CSS Grid Layout perftest results are around 4% worse, which is expected as we're adding a missing layout.
+
+        Test: fast/css-grid-layout/flex-content-sized-column-use-available-width.html
+
+        * rendering/RenderGrid.cpp:
+        (WebCore::RenderGrid::logicalContentHeightForChild): Check width changes and mark element as needed layout if required.
+
 2014-02-16  Andreas Kling  <akling@apple.com>
 
         Ensure that removing an iframe from the DOM tree disconnects its Frame.
index 0677952..44b85ca 100644 (file)
@@ -424,10 +424,12 @@ size_t RenderGrid::explicitGridSizeForSide(GridPositionSide side) const
 
 LayoutUnit RenderGrid::logicalContentHeightForChild(RenderBox* child, Vector<GridTrack>& columnTracks)
 {
-    if (child->style().logicalHeight().isPercent())
+    LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child->hasOverrideContainingBlockLogicalWidth() ? child->overrideContainingBlockContentLogicalWidth() : LayoutUnit();
+    LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(child, ForColumns, columnTracks);
+    if (child->style().logicalHeight().isPercent() || oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth)
         child->setNeedsLayout(MarkOnlyThis);
 
-    child->setOverrideContainingBlockContentLogicalWidth(gridAreaBreadthForChild(child, ForColumns, columnTracks));
+    child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
     // If |child| has a percentage logical height, we shouldn't let it override its intrinsic height, which is
     // what we are interested in here. Thus we need to set the override logical height to -1 (no possible resolution).
     child->setOverrideContainingBlockContentLogicalHeight(-1);