Changeset 179987 in webkit for trunk/Source/WebCore/rendering/RenderGrid.cpp

Timestamp:

Feb 12, 2015, 8:14:16 AM (11 years ago)

Author:

svillar@igalia.com

Message:

[CSS Grid Layout] Invalid initialization of track sizes with non spanning grid items
​https://bugs.webkit.org/show_bug.cgi?id=140763

Reviewed by Antti Koivisto.

Source/WebCore:

Content sized tracks with non-spanning grid items were not
properly sized because the growth limit was sometimes infinity
(-1) after calling resolveContentBasedTrackSizingFunctions() when
it should not. This patch adds an special initialization phase for
non-spanning grid items as the new track sizing algorithm
describes.

Granted, that was handled in the old algorithm in
distributeSpaceToTracks() as a special case. The problem is that
it regressed after the optimization added in r173868 because that
method is no longer called when the space to distribute is 0.

That's why we could fix this by allowing calls to
distributeSpaceToTracks() with spaceToDistribute>=0 but by fixing
it with an explicit initialization our implementation becomes
closer to the new algorithm and the initialization is now explicit
in the code instead of a side effect of calling
distributeSpaceToTracks() with no space to be distributed. It also
brings a slight performance improvement as we save sorts and hash
lookups.

I also took the change to add caching to several GridTrackSize
methods that were hot on the profiler (each one accounted for ~1%
of the total time, now they account for ~0.3% each).

Test: fast/css-grid-layout/grid-initialize-span-one-items.html

• rendering/RenderGrid.cpp:

(WebCore::GridItemWithSpan::span): New helper method for ASSERTs.
(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions):
Exclude non spanning grid items from the calls to
resolveContentBasedTrackSizingFunctionsForItems().
(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems):
New method to resolve track sizes only using non-spanning grid
items.
(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctionsForItems):
Ensure that it isn't called for non-spanning grid items.

• rendering/RenderGrid.h:
• rendering/style/GridTrackSize.h:

(WebCore::GridTrackSize::GridTrackSize): Cache return values.
(WebCore::GridTrackSize::setLength): Ditto.
(WebCore::GridTrackSize::setMinMax): Ditto.
(WebCore::GridTrackSize::cacheMinMaxTrackBreadthTypes): New method
that caches the return values for hasXXXTrackBreadth() methods.
(WebCore::GridTrackSize::hasMinOrMaxContentMinTrackBreadth): Use
the cached return value.
(WebCore::GridTrackSize::hasMaxContentMaxTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMinContentMaxTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMinOrMaxContentMaxTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMaxContentMinTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMinContentMinTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth):
Ditto.
(WebCore::GridTrackSize::hasMaxContentMinTrackBreadthAndMaxContentMaxTrackBreadth):
Ditto.

LayoutTests:

• fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt:
• fast/css-grid-layout/grid-content-sized-columns-resolution.html:
• fast/css-grid-layout/grid-initialize-span-one-items-expected.txt: Added.
• fast/css-grid-layout/grid-initialize-span-one-items.html: Added.

File:

• trunk/Source/WebCore/rendering/RenderGrid.cpp (modified) (3 diffs)

trunk/Source/WebCore/rendering/RenderGrid.cpp

@@ -585 +585 @@
     RenderBox& gridItem() const { return m_gridItem; }
     GridCoordinate coordinate() const { return m_coordinate; }
+#if !ASSERT_DISABLED
+    size_t span() const { return m_span; }
+#endif
 
     bool operator<(const GridItemWithSpan other) const
@@ -620 +623 @@
     for (auto trackIndex : sizingData.contentSizedTracksIndex) {
         GridIterator iterator(m_grid, direction, trackIndex);
+        GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackIndex] : sizingData.rowTracks[trackIndex];
 
         while (RenderBox* gridItem = iterator.nextGridItem()) {
             if (itemsSet.add(gridItem).isNewEntry) {
                 const GridCoordinate& coordinate = cachedGridCoordinate(*gridItem);
-                // We should not include items spanning more than one track that span tracks with flexible sizing functions.
-                if (integerSpanForDirection(coordinate, direction) == 1 || !spanningItemCrossesFlexibleSizedTracks(coordinate, direction))
+                if (integerSpanForDirection(coordinate, direction) == 1)
+                    resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, coordinate, *gridItem, track, sizingData.columnTracks);
+                else if (!spanningItemCrossesFlexibleSizedTracks(coordinate, direction))
                     sizingData.itemsSortedByIncreasingSpan.append(GridItemWithSpan(*gridItem, coordinate, direction));
             }
@@ -646 +651 @@
 }
 
+void RenderGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection direction, const GridCoordinate& coordinate, RenderBox& gridItem, GridTrack& track, Vector<GridTrack>& columnTracks)
+{
+    const GridResolvedPosition trackPosition = (direction == ForColumns) ? coordinate.columns.resolvedInitialPosition : coordinate.rows.resolvedInitialPosition;
+    GridTrackSize trackSize = gridTrackSize(direction, trackPosition.toInt());
+
+    if (trackSize.hasMinContentMinTrackBreadth())
+        track.setBaseSize(std::max(track.baseSize(), minContentForChild(gridItem, direction, columnTracks)));
+    else if (trackSize.hasMaxContentMinTrackBreadth())
+        track.setBaseSize(std::max(track.baseSize(), maxContentForChild(gridItem, direction, columnTracks)));
+
+    if (trackSize.hasMinContentMaxTrackBreadth())
+        track.setGrowthLimit(std::max(track.growthLimit(), minContentForChild(gridItem, direction, columnTracks)));
+    else if (trackSize.hasMaxContentMaxTrackBreadth())
+        track.setGrowthLimit(std::max(track.growthLimit(), maxContentForChild(gridItem, direction, columnTracks)));
+}
+
 void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection direction, GridSizingData& sizingData, GridItemWithSpan& gridItemWithSpan, FilterFunction filterFunction, SizingFunction sizingFunction, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, FilterFunction growAboveMaxBreadthFilterFunction)
 {
+    ASSERT(gridItemWithSpan.span() > 1);
     const GridCoordinate& coordinate = gridItemWithSpan.coordinate();
     const GridResolvedPosition initialTrackPosition = (direction == ForColumns) ? coordinate.columns.resolvedInitialPosition : coordinate.rows.resolvedInitialPosition;