Log Message
[css-grid] Empty grid without explicit tracks shouldn't have any size https://bugs.webkit.org/show_bug.cgi?id=155197
Reviewed by Darin Adler. Source/WebCore: The internal representation of the grid is a Vector of Vector representing rows and columns. Because of that it was not possible to have columns without having at least one row. That forced us to have a 1x1 internal representation of the grid even if it was actually empty. That works for most of the cases except when the grid is actually empty. By changing the way we compute the sizes we can overcome that implementation restriction. This allowed us also to thighten the conditions under we could use the GridIterator. From now on it won't be possible to use it on empty grids so callers should enforce that restriction. A new bool was added to verify that placeItemsOnGrid() has been already called. The previous code was relying on the fact that there were items in the internal representation, which is wrong, as there might be no items in the grid. Test: fast/css-grid-layout/empty-grid.html * rendering/RenderGrid.cpp: (WebCore::RenderGrid::GridIterator::GridIterator): Added ASSERTs. (WebCore::RenderGrid::GridIterator::nextGridItem): Ditto. (WebCore::RenderGrid::GridIterator::isEmptyAreaEnough): Ditto. (WebCore::RenderGrid::GridIterator::nextEmptyGridArea): Ditto. (WebCore::RenderGrid::gridColumnCount): Use the style to resolve the number of columns if the internal representation is empty. (WebCore::RenderGrid::gridRowCount): (WebCore::RenderGrid::guttersSize): Allow to pass 0 as span, this permits using the return value of gridColumnCount|gridRowCount directly to call this method. (WebCore::RenderGrid::computeIntrinsicLogicalWidths): Use m_gridIsDirty. (WebCore::RenderGrid::computeUsedBreadthOfGridTracks): Do not examine the contents of grid tracks if there are no items in the grid. (WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions): Ditto. (WebCore::RenderGrid::placeItemsOnGrid): Set m_gridIsDirty to false. (WebCore::RenderGrid::populateExplicitGridAndOrderIterator): (WebCore::RenderGrid::clearGrid): (WebCore::RenderGrid::populateGridPositionsForDirection): * rendering/RenderGrid.h: Moved gridColumnCount/gridRowCount to cpp file. LayoutTests: Make sure that empty grids (and grids with one empty axis) are properly handled. Do also verify that removing all the items from a grid also generates an correct empty grid. * fast/css-grid-layout/empty-grid-expected.txt: Added. * fast/css-grid-layout/empty-grid.html: Added.
Modified Paths
- trunk/LayoutTests/ChangeLog
- trunk/Source/WebCore/ChangeLog
- trunk/Source/WebCore/rendering/RenderGrid.cpp
- trunk/Source/WebCore/rendering/RenderGrid.h
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (201509 => 201510)
--- trunk/LayoutTests/ChangeLog 2016-05-31 10:34:29 UTC (rev 201509)
+++ trunk/LayoutTests/ChangeLog 2016-05-31 15:17:03 UTC (rev 201510)
@@ -1,3 +1,16 @@
+2016-05-25 Sergio Villar Senin <svil...@igalia.com>
+
+ [css-grid] Empty grid without explicit tracks shouldn't have any size
+ https://bugs.webkit.org/show_bug.cgi?id=155197
+
+ Reviewed by Darin Adler.
+
+ Make sure that empty grids (and grids with one empty axis) are properly handled. Do also
+ verify that removing all the items from a grid also generates an correct empty grid.
+
+ * fast/css-grid-layout/empty-grid-expected.txt: Added.
+ * fast/css-grid-layout/empty-grid.html: Added.
+
2016-05-30 Per Arne Vollan <pvol...@apple.com>
Unreviewed test gardening.
Added: trunk/LayoutTests/fast/css-grid-layout/empty-grid-expected.txt (0 => 201510)
--- trunk/LayoutTests/fast/css-grid-layout/empty-grid-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/css-grid-layout/empty-grid-expected.txt 2016-05-31 15:17:03 UTC (rev 201510)
@@ -0,0 +1,12 @@
+This test checks that grids with no in-flow items are actually empty.
+
+PASS
+XXXX
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
Property changes on: trunk/LayoutTests/fast/css-grid-layout/empty-grid-expected.txt
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/fast/css-grid-layout/empty-grid.html (0 => 201510)
--- trunk/LayoutTests/fast/css-grid-layout/empty-grid.html (rev 0)
+++ trunk/LayoutTests/fast/css-grid-layout/empty-grid.html 2016-05-31 15:17:03 UTC (rev 201510)
@@ -0,0 +1,61 @@
+<!DOCTYPE html>
+<link href="" rel="stylesheet">
+<link href="" rel=stylesheet>
+<style>
+.gridWithAbsolutePositionedItem {
+ /* Ensures that the grid container is the containing block of the absolutely positioned grid children. */
+ position: relative;
+}
+
+.grid {
+ grid-auto-columns: 200px;
+ grid-auto-rows: 200px;
+}
+
+.item {
+ position: absolute;
+ font: 10px/1 Ahem;
+}
+
+</style>
+
+<script>
+function addRemoveItem()
+{
+ var gridItem = document.createElement("div");
+ gridItem.style.width = "100px";
+ gridItem.style.height = "100px";
+ gridItem.style.backgroundColor = "red";
+ var gridElement = document.getElementById("dynamicGrid");
+ gridElement.appendChild(gridItem);
+ gridElement.removeChild(gridItem);
+}
+
+function doTest() {
+ addRemoveItem();
+ checkLayout(".grid");
+}
+</script>
+<script src=""
+
+<body _onload_="doTest()">
+
+<p>This test checks that grids with no in-flow items are actually empty.</p>
+
+<div class="grid min-content" data-expected-width="0" data-expected-height="0"></div>
+
+<div class="grid min-content gridWithAbsolutePositionedItem" data-expected-width="0" data-expected-height="0">
+ <div class="item" data-expected-width="40" data-expected-height="10">XXXX</div>
+</div>
+
+<div id="dynamicGrid" class="grid min-content" data-expected-width="0" data-expected-height="0"></div>
+
+<div class="grid min-content" style="grid-template-rows: 100px;" data-expected-width="0" data-expected-height="100"></div>
+<div class="grid min-content" style="grid-template-rows: auto;" data-expected-width="0" data-expected-height="0"></div>
+<div class="grid min-content" style="grid-template-rows: 1fr;" data-expected-width="0" data-expected-height="0"></div>
+
+<div class="grid min-content" style="grid-template-columns: 100px;" data-expected-width="100" data-expected-height="0"></div>
+<div class="grid min-content" style="grid-template-columns: auto;" data-expected-width="0" data-expected-height="0"></div>
+<div class="grid min-content" style="grid-template-columns: 1fr;" data-expected-width="0" data-expected-height="0"></div>
+
+</body>
Property changes on: trunk/LayoutTests/fast/css-grid-layout/empty-grid.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:eol-style
Modified: trunk/Source/WebCore/ChangeLog (201509 => 201510)
--- trunk/Source/WebCore/ChangeLog 2016-05-31 10:34:29 UTC (rev 201509)
+++ trunk/Source/WebCore/ChangeLog 2016-05-31 15:17:03 UTC (rev 201510)
@@ -1,3 +1,46 @@
+2016-05-25 Sergio Villar Senin <svil...@igalia.com>
+
+ [css-grid] Empty grid without explicit tracks shouldn't have any size
+ https://bugs.webkit.org/show_bug.cgi?id=155197
+
+ Reviewed by Darin Adler.
+
+ The internal representation of the grid is a Vector of Vector representing rows and
+ columns. Because of that it was not possible to have columns without having at least one
+ row. That forced us to have a 1x1 internal representation of the grid even if it was
+ actually empty. That works for most of the cases except when the grid is actually empty.
+
+ By changing the way we compute the sizes we can overcome that implementation
+ restriction. This allowed us also to thighten the conditions under we could use the
+ GridIterator. From now on it won't be possible to use it on empty grids so callers should
+ enforce that restriction.
+
+ A new bool was added to verify that placeItemsOnGrid() has been already called. The previous
+ code was relying on the fact that there were items in the internal representation, which is
+ wrong, as there might be no items in the grid.
+
+ Test: fast/css-grid-layout/empty-grid.html
+
+ * rendering/RenderGrid.cpp:
+ (WebCore::RenderGrid::GridIterator::GridIterator): Added ASSERTs.
+ (WebCore::RenderGrid::GridIterator::nextGridItem): Ditto.
+ (WebCore::RenderGrid::GridIterator::isEmptyAreaEnough): Ditto.
+ (WebCore::RenderGrid::GridIterator::nextEmptyGridArea): Ditto.
+ (WebCore::RenderGrid::gridColumnCount): Use the style to resolve the number of columns if
+ the internal representation is empty.
+ (WebCore::RenderGrid::gridRowCount):
+ (WebCore::RenderGrid::guttersSize): Allow to pass 0 as span, this permits using the return
+ value of gridColumnCount|gridRowCount directly to call this method.
+ (WebCore::RenderGrid::computeIntrinsicLogicalWidths): Use m_gridIsDirty.
+ (WebCore::RenderGrid::computeUsedBreadthOfGridTracks): Do not examine the contents of grid
+ tracks if there are no items in the grid.
+ (WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions): Ditto.
+ (WebCore::RenderGrid::placeItemsOnGrid): Set m_gridIsDirty to false.
+ (WebCore::RenderGrid::populateExplicitGridAndOrderIterator):
+ (WebCore::RenderGrid::clearGrid):
+ (WebCore::RenderGrid::populateGridPositionsForDirection):
+ * rendering/RenderGrid.h: Moved gridColumnCount/gridRowCount to cpp file.
+
2016-05-30 Brady Eidson <beid...@apple.com>
Move CrossThreadCopier/CrossThreadTask to WTF.
Modified: trunk/Source/WebCore/rendering/RenderGrid.cpp (201509 => 201510)
--- trunk/Source/WebCore/rendering/RenderGrid.cpp 2016-05-31 10:34:29 UTC (rev 201509)
+++ trunk/Source/WebCore/rendering/RenderGrid.cpp 2016-05-31 15:17:03 UTC (rev 201510)
@@ -139,14 +139,16 @@
, m_columnIndex((direction == ForColumns) ? fixedTrackIndex : varyingTrackIndex)
, m_childIndex(0)
{
+ ASSERT(!m_grid.isEmpty());
+ ASSERT(!m_grid[0].isEmpty());
ASSERT(m_rowIndex < m_grid.size());
ASSERT(m_columnIndex < m_grid[0].size());
}
RenderBox* nextGridItem()
{
- if (!m_grid.size())
- return 0;
+ ASSERT(!m_grid.isEmpty());
+ ASSERT(!m_grid[0].isEmpty());
unsigned& varyingTrackIndex = (m_direction == ForColumns) ? m_rowIndex : m_columnIndex;
const unsigned endOfVaryingTrackIndex = (m_direction == ForColumns) ? m_grid.size() : m_grid[0].size();
@@ -162,6 +164,9 @@
bool isEmptyAreaEnough(unsigned rowSpan, unsigned columnSpan) const
{
+ ASSERT(!m_grid.isEmpty());
+ ASSERT(!m_grid[0].isEmpty());
+
// Ignore cells outside current grid as we will grow it later if needed.
unsigned maxRows = std::min<unsigned>(m_rowIndex + rowSpan, m_grid.size());
unsigned maxColumns = std::min<unsigned>(m_columnIndex + columnSpan, m_grid[0].size());
@@ -180,7 +185,10 @@
std::unique_ptr<GridArea> nextEmptyGridArea(unsigned fixedTrackSpan, unsigned varyingTrackSpan)
{
- ASSERT(fixedTrackSpan >= 1 && varyingTrackSpan >= 1);
+ ASSERT(!m_grid.isEmpty());
+ ASSERT(!m_grid[0].isEmpty());
+ ASSERT(fixedTrackSpan >= 1);
+ ASSERT(varyingTrackSpan >= 1);
if (m_grid.isEmpty())
return nullptr;
@@ -319,6 +327,23 @@
}
}
+unsigned RenderGrid::gridColumnCount() const
+{
+ ASSERT(!m_gridIsDirty);
+ // Due to limitations in our internal representation, we cannot know the number of columns from
+ // m_grid *if* there is no row (because m_grid would be empty). That's why in that case we need
+ // to get it from the style. Note that we know for sure that there are't any implicit tracks,
+ // because not having rows implies that there are no "normal" children (out-of-flow children are
+ // not stored in m_grid).
+ return m_grid.size() ? m_grid[0].size() : GridPositionsResolver::explicitGridColumnCount(style(), m_autoRepeatColumns);
+}
+
+unsigned RenderGrid::gridRowCount() const
+{
+ ASSERT(!m_gridIsDirty);
+ return m_grid.size();
+}
+
LayoutUnit RenderGrid::computeTrackBasedLogicalHeight(const GridSizingData& sizingData) const
{
LayoutUnit logicalHeight;
@@ -420,9 +445,7 @@
LayoutUnit RenderGrid::guttersSize(GridTrackSizingDirection direction, unsigned span) const
{
- ASSERT(span >= 1);
-
- if (span == 1)
+ if (span <= 1)
return { };
const Length& trackGap = direction == ForColumns ? style().gridColumnGap() : style().gridRowGap();
@@ -436,7 +459,7 @@
void RenderGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
{
- bool wasPopulated = gridWasPopulated();
+ bool wasPopulated = !m_gridIsDirty;
if (!wasPopulated)
const_cast<RenderGrid*>(this)->placeItemsOnGrid();
@@ -589,16 +612,18 @@
for (const auto& trackIndex : flexibleSizedTracksIndex)
flexFraction = std::max(flexFraction, normalizedFlexFraction(tracks[trackIndex], gridTrackSize(direction, trackIndex).maxTrackBreadth().flex()));
- for (unsigned i = 0; i < flexibleSizedTracksIndex.size(); ++i) {
- GridIterator iterator(m_grid, direction, flexibleSizedTracksIndex[i]);
- while (RenderBox* gridItem = iterator.nextGridItem()) {
- const GridSpan span = cachedGridSpan(*gridItem, direction);
+ if (!m_gridItemArea.isEmpty()) {
+ for (unsigned i = 0; i < flexibleSizedTracksIndex.size(); ++i) {
+ GridIterator iterator(m_grid, direction, flexibleSizedTracksIndex[i]);
+ while (auto* gridItem = iterator.nextGridItem()) {
+ GridSpan& span = cachedGridSpan(*gridItem, direction);
- // Do not include already processed items.
- if (i > 0 && span.startLine() <= flexibleSizedTracksIndex[i - 1])
- continue;
+ // Do not include already processed items.
+ if (i > 0 && span.startLine() <= flexibleSizedTracksIndex[i - 1])
+ continue;
- flexFraction = std::max(flexFraction, findFlexFactorUnitSize(tracks, span, direction, maxContentForChild(*gridItem, direction, sizingData)));
+ flexFraction = std::max(flexFraction, findFlexFactorUnitSize(tracks, span, direction, maxContentForChild(*gridItem, direction, sizingData)));
+ }
}
}
}
@@ -898,21 +923,23 @@
{
sizingData.itemsSortedByIncreasingSpan.shrink(0);
HashSet<RenderBox*> itemsSet;
- for (auto trackIndex : sizingData.contentSizedTracksIndex) {
- GridIterator iterator(m_grid, direction, trackIndex);
- GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackIndex] : sizingData.rowTracks[trackIndex];
+ if (!m_gridItemArea.isEmpty()) {
+ 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 GridSpan& span = cachedGridSpan(*gridItem, direction);
- if (span.integerSpan() == 1)
- resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, span, *gridItem, track, sizingData);
- else if (!spanningItemCrossesFlexibleSizedTracks(span, direction))
- sizingData.itemsSortedByIncreasingSpan.append(GridItemWithSpan(*gridItem, span));
+ while (auto* gridItem = iterator.nextGridItem()) {
+ if (itemsSet.add(gridItem).isNewEntry) {
+ GridSpan& span = cachedGridSpan(*gridItem, direction);
+ if (span.integerSpan() == 1)
+ resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, span, *gridItem, track, sizingData);
+ else if (!spanningItemCrossesFlexibleSizedTracks(span, direction))
+ sizingData.itemsSortedByIncreasingSpan.append(GridItemWithSpan(*gridItem, span));
+ }
}
}
+ std::sort(sizingData.itemsSortedByIncreasingSpan.begin(), sizingData.itemsSortedByIncreasingSpan.end());
}
- std::sort(sizingData.itemsSortedByIncreasingSpan.begin(), sizingData.itemsSortedByIncreasingSpan.end());
auto it = sizingData.itemsSortedByIncreasingSpan.begin();
auto end = sizingData.itemsSortedByIncreasingSpan.end();
@@ -1295,13 +1322,14 @@
void RenderGrid::placeItemsOnGrid()
{
- ASSERT(!gridWasPopulated());
+ ASSERT(m_gridIsDirty);
ASSERT(m_gridItemArea.isEmpty());
m_autoRepeatColumns = computeAutoRepeatTracksCount(ForColumns);
m_autoRepeatRows = computeAutoRepeatTracksCount(ForRows);
populateExplicitGridAndOrderIterator();
+ m_gridIsDirty = false;
Vector<RenderBox*> autoMajorAxisAutoGridItems;
Vector<RenderBox*> specifiedMajorAxisAutoGridItems;
@@ -1349,8 +1377,8 @@
{
OrderIteratorPopulator populator(m_orderIterator);
m_smallestRowStart = m_smallestColumnStart = 0;
- unsigned maximumRowIndex = std::max<unsigned>(1, GridPositionsResolver::explicitGridRowCount(style(), m_autoRepeatRows));
- unsigned maximumColumnIndex = std::max<unsigned>(1, GridPositionsResolver::explicitGridColumnCount(style(), m_autoRepeatColumns));
+ unsigned maximumRowIndex = GridPositionsResolver::explicitGridRowCount(style(), m_autoRepeatRows);
+ unsigned maximumColumnIndex = GridPositionsResolver::explicitGridColumnCount(style(), m_autoRepeatColumns);
for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
if (child->isOutOfFlowPositioned())
@@ -1510,6 +1538,7 @@
{
m_grid.clear();
m_gridItemArea.clear();
+ m_gridIsDirty = true;
}
static const StyleContentAlignmentData& contentAlignmentNormalBehaviorGrid()
@@ -1767,16 +1796,19 @@
unsigned numberOfTracks = tracks.size();
unsigned numberOfLines = numberOfTracks + 1;
unsigned lastLine = numberOfLines - 1;
- unsigned nextToLastLine = numberOfLines - 2;
+
ContentAlignmentData offset = computeContentPositionAndDistributionOffset(direction, sizingData.freeSpaceForDirection(direction).value(), numberOfTracks);
LayoutUnit trackGap = guttersSize(direction, 2);
auto& positions = isRowAxis ? m_columnPositions : m_rowPositions;
positions.resize(numberOfLines);
auto borderAndPadding = isRowAxis ? borderAndPaddingLogicalLeft() : borderAndPaddingBefore();
positions[0] = borderAndPadding + offset.positionOffset;
- for (unsigned i = 0; i < nextToLastLine; ++i)
- positions[i + 1] = positions[i] + offset.distributionOffset + tracks[i].baseSize() + trackGap;
- positions[lastLine] = positions[nextToLastLine] + tracks[nextToLastLine].baseSize();
+ if (numberOfLines > 1) {
+ unsigned nextToLastLine = numberOfLines - 2;
+ for (unsigned i = 0; i < nextToLastLine; ++i)
+ positions[i + 1] = positions[i] + offset.distributionOffset + tracks[i].baseSize() + trackGap;
+ positions[lastLine] = positions[nextToLastLine] + tracks[nextToLastLine].baseSize();
+ }
auto& offsetBetweenTracks = isRowAxis ? m_offsetBetweenColumns : m_offsetBetweenRows;
offsetBetweenTracks = offset.distributionOffset;
}
Modified: trunk/Source/WebCore/rendering/RenderGrid.h (201509 => 201510)
--- trunk/Source/WebCore/rendering/RenderGrid.h 2016-05-31 10:34:29 UTC (rev 201509)
+++ trunk/Source/WebCore/rendering/RenderGrid.h 2016-05-31 15:17:03 UTC (rev 201510)
@@ -173,20 +173,10 @@
bool tracksAreWiderThanMinTrackBreadth(GridTrackSizingDirection, GridSizingData&);
#endif
- bool gridWasPopulated() const { return !m_grid.isEmpty() && !m_grid[0].isEmpty(); }
-
bool spanningItemCrossesFlexibleSizedTracks(const GridSpan&, GridTrackSizingDirection) const;
- unsigned gridColumnCount() const
- {
- ASSERT(gridWasPopulated());
- return m_grid[0].size();
- }
- unsigned gridRowCount() const
- {
- ASSERT(gridWasPopulated());
- return m_grid.size();
- }
+ unsigned gridColumnCount() const;
+ unsigned gridRowCount() const;
bool hasDefiniteLogicalSize(GridTrackSizingDirection) const;
LayoutUnit translateRTLCoordinate(LayoutUnit) const;
@@ -207,6 +197,8 @@
unsigned m_autoRepeatColumns { 0 };
unsigned m_autoRepeatRows { 0 };
+
+ bool m_gridIsDirty { true };
};
size_t inline RenderGrid::autoRepeatCountForDirection(GridTrackSizingDirection direction) const
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
