Use unclamped subgrid track sizes to compute track positions, so that…

… 'gap' is accounted for correctly. https://bugs.webkit.org/show_bug.cgi?id=245936 <rdar://100939450> Reviewed by Alan Baradlay. When computing subgrid track sizes, we copy the sizing from the parent grid, and then adjust for any gap differences specific to the current subgrid. This value gets clamped a minimum of 0, losing data about cases where the track size wasn't expanded to include the subgrid's gap (because the track sizing function didn't allow it, or there were no items within the subgrid that got contributed to that track to include it). When we convert from track sizes to track positions, we use the size array and pad it by the gap value (as we do for normal grids), and fail to account for the fact that the gaps weren't always compensated for in the sizes. This changes us to store the unclamped value, and makes all of the other callers use a getter which applies clamping. Track position computation uses the unclamped sizes so that we can always add the gap back in. An alternative approach here would be to have a subgrid-specific codepath in populateGridPositionsForDirection, which instead of using the size array, just copies the position array from the parent. We could the adjust for gap separately for each position (so that we don't accumulate incorrect gaps), or apply the gap when we set the item position within the subgrid. * LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010.html: Added. Adds a new WPT where no items get added to the first column, and ensures that we don't add space for a gap in that column. * LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011.html: Added. Adds a WPT where the column widths are fixed, and the subgrid tries to add gaps that would cover more than the entire middle column. Ensures that the positioning of the item in the third column doesn't accumulate extra left padding from the overlap. * Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp: (WebCore::GridTrack::baseSize const): (WebCore::GridTrack::unclampedBaseSize const): (WebCore::GridTrack::growthLimit const): (WebCore::GridTrack::growthLimitIfNotInfinite const): (WebCore::GridTrack::ensureGrowthLimitIsBiggerThanBaseSize): (WebCore::GridTrackSizingAlgorithm::copyUsedTrackSizesForSubgrid): * Source/WebCore/rendering/GridTrackSizingAlgorithm.h: (WebCore::GridTrack::isGrowthLimitBiggerThanBaseSize const): * Source/WebCore/rendering/RenderGrid.cpp: (WebCore::RenderGrid::populateGridPositionsForDirection): Canonical link: https://commits.webkit.org/256621@main
WebKit · Nov 13, 2022 · 87d76d7dac687152f8f064a087dd79b02a889203 · 87d76d7
1 parent d16d7ec
commit 87d76d7dac687152f8f064a087dd79b02a889203
Show file tree

Hide file tree

Showing 9 changed files with 96 additions and 12 deletions.
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010-expected.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010-expected.html
@@ -0,0 +1,11 @@
+<!DOCTYPE HTML>
+<html><head>
+  <meta charset="utf-8">
+  <title>CSS Grid Test: Subgrids with empty tracks and column gap</title>
+  <link rel="author" title="Matt Woodrow" href="mailto:mattwoodrow@apple.com">
+  <link rel="help" href="https://drafts.csswg.org/css-grid-2">
+</head>
+<body>
+<div style="width:100px; height: 100px; background-color: blue; position:relative; left:50px;"></div>
+</body>
+</html>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010-ref.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010-ref.html
@@ -0,0 +1,11 @@
+<!DOCTYPE HTML>
+<html><head>
+  <meta charset="utf-8">
+  <title>CSS Grid Test: Subgrids with empty tracks and column gap</title>
+  <link rel="author" title="Matt Woodrow" href="mailto:mattwoodrow@apple.com">
+  <link rel="help" href="https://drafts.csswg.org/css-grid-2">
+</head>
+<body>
+<div style="width:100px; height: 100px; background-color: blue; position:relative; left:50px;"></div>
+</body>
+</html>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-010.html
@@ -0,0 +1,16 @@
+<!DOCTYPE HTML>
+<html><head>
+  <meta charset="utf-8">
+  <title>CSS Grid Test: Subgrids with empty tracks and column gap</title>
+  <link rel="author" title="Matt Woodrow" href="mailto:mattwoodrow@apple.com">
+  <link rel="help" href="https://drafts.csswg.org/css-grid-2">
+  <link rel="match" href="grid-gap-010-ref.html">
+</head>
+<body>
+<div style="display:grid; grid-template-columns: auto 200px; width: 200px">
+    <div style="display:grid; grid-template-columns: subgrid; gap: 100px; grid-column: span 2">
+        <div style="width:100px; height: 100px; background-color: blue; grid-column: 2"></div>
+    </div>
+</div>
+</body>
+</html>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011-expected.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011-expected.html
@@ -0,0 +1,11 @@
+<!DOCTYPE HTML>
+<html><head>
+  <meta charset="utf-8">
+  <title>CSS Grid Test: Subgrids with empty tracks and column gap</title>
+  <link rel="author" title="Matt Woodrow" href="mailto:mattwoodrow@apple.com">
+  <link rel="help" href="https://drafts.csswg.org/css-grid-2">
+</head>
+<body>
+<div style="width:200px; height: 100px; background-color: blue; position:relative; left:175px;"></div>
+</body>
+</html>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011-ref.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011-ref.html
@@ -0,0 +1,11 @@
+<!DOCTYPE HTML>
+<html><head>
+  <meta charset="utf-8">
+  <title>CSS Grid Test: Subgrids with empty tracks and column gap</title>
+  <link rel="author" title="Matt Woodrow" href="mailto:mattwoodrow@apple.com">
+  <link rel="help" href="https://drafts.csswg.org/css-grid-2">
+</head>
+<body>
+<div style="width:200px; height: 100px; background-color: blue; position:relative; left:175px;"></div>
+</body>
+</html>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-gap-011.html
@@ -0,0 +1,17 @@
+<!DOCTYPE HTML>
+<html><head>
+  <meta charset="utf-8">
+  <title>CSS Grid Test: Subgrids with column gap larger than the track size</title>
+  <link rel="author" title="Matt Woodrow" href="mailto:mattwoodrow@apple.com">
+  <link rel="help" href="https://drafts.csswg.org/css-grid-2">
+  <link rel="match" href="grid-gap-011-ref.html">
+</head>
+<body>
+<div style="display:grid; grid-template-columns: 100px 100px 100px; width: 300px">
+    <div style="display:grid; grid-template-columns: subgrid; gap: 150px; grid-column: span 3">
+        <div style="width:100px; height: 100px; background-color: blue; grid-column: 2"></div>
+        <div style="width:100px; height: 100px; background-color: blue; grid-column: 3"></div>
+    </div>
+</div>
+
+</body>
diff --git a/Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp b/Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp
@@ -34,7 +34,13 @@
 
 namespace WebCore {
 
-const LayoutUnit& GridTrack::baseSize() const
+LayoutUnit GridTrack::baseSize() const
+{
+    ASSERT(isGrowthLimitBiggerThanBaseSize());
+    return std::max(m_baseSize, 0_lu);
+}
+
+LayoutUnit GridTrack::unclampedBaseSize() const
 {
     ASSERT(isGrowthLimitBiggerThanBaseSize());
     return m_baseSize;
@@ -43,7 +49,7 @@ const LayoutUnit& GridTrack::baseSize() const
 const LayoutUnit& GridTrack::growthLimit() const
 {
     ASSERT(isGrowthLimitBiggerThanBaseSize());
-    ASSERT(!m_growthLimitCap || m_growthLimitCap.value() >= m_growthLimit || m_baseSize >= m_growthLimitCap.value());
+    ASSERT(!m_growthLimitCap || m_growthLimitCap.value() >= m_growthLimit || baseSize() >= m_growthLimitCap.value());
     return m_growthLimit;
 }
 
@@ -59,10 +65,10 @@ void GridTrack::setGrowthLimit(LayoutUnit growthLimit)
     ensureGrowthLimitIsBiggerThanBaseSize();
 }
 
-const LayoutUnit& GridTrack::growthLimitIfNotInfinite() const
+LayoutUnit GridTrack::growthLimitIfNotInfinite() const
 {
     ASSERT(isGrowthLimitBiggerThanBaseSize());
-    return m_growthLimit == infinity ? m_baseSize : m_growthLimit;
+    return m_growthLimit == infinity ? baseSize() : m_growthLimit;
 }
 
 void GridTrack::setTempSize(const LayoutUnit& tempSize)
@@ -91,8 +97,8 @@ void GridTrack::setCachedTrackSize(const GridTrackSize& cachedTrackSize)
 
 void GridTrack::ensureGrowthLimitIsBiggerThanBaseSize()
 {
-    if (m_growthLimit != infinity && m_growthLimit < m_baseSize)
-        m_growthLimit = m_baseSize;
+    if (m_growthLimit != infinity && m_growthLimit < std::max(m_baseSize, 0_lu))
+        m_growthLimit = std::max(m_baseSize, 0_lu);
 }
 
 // Static helper methods.
@@ -1588,7 +1594,7 @@ bool GridTrackSizingAlgorithm::copyUsedTrackSizesForSubgrid()
             size -= gapDifference;
         if (i != numTracks - 1)
             size -= gapDifference;
-        allTracks[i].setBaseSize(std::max(size, 0_lu));
+        allTracks[i].setBaseSize(size);
     }
     return true;
 }

diff --git a/Source/WebCore/rendering/GridTrackSizingAlgorithm.h b/Source/WebCore/rendering/GridTrackSizingAlgorithm.h
@@ -61,15 +61,16 @@ class GridTrack {
 public:
     GridTrack() = default;
 
-    const LayoutUnit& baseSize() const;
+    LayoutUnit baseSize() const;
+    LayoutUnit unclampedBaseSize() const;
     void setBaseSize(LayoutUnit);
 
     const LayoutUnit& growthLimit() const;
     bool growthLimitIsInfinite() const { return m_growthLimit == infinity; }
     void setGrowthLimit(LayoutUnit);
 
     bool infiniteGrowthPotential() const { return growthLimitIsInfinite() || m_infinitelyGrowable; }
-    const LayoutUnit& growthLimitIfNotInfinite() const;
+    LayoutUnit growthLimitIfNotInfinite() const;
 
     const LayoutUnit& plannedSize() const { return m_plannedSize; }
     void setPlannedSize(LayoutUnit plannedSize) { m_plannedSize = plannedSize; }
@@ -88,7 +89,7 @@ class GridTrack {
     void setCachedTrackSize(const GridTrackSize&);
 
 private:
-    bool isGrowthLimitBiggerThanBaseSize() const { return growthLimitIsInfinite() || m_growthLimit >= m_baseSize; }
+    bool isGrowthLimitBiggerThanBaseSize() const { return growthLimitIsInfinite() || m_growthLimit >= std::max(m_baseSize, 0_lu); }
 
     void ensureGrowthLimitIsBiggerThanBaseSize();
 

diff --git a/Source/WebCore/rendering/RenderGrid.cpp b/Source/WebCore/rendering/RenderGrid.cpp
@@ -1194,8 +1194,8 @@ void RenderGrid::populateGridPositionsForDirection(GridTrackSizingDirection dire
         unsigned nextToLastLine = numberOfLines - 2;
 
         for (unsigned i = 0; i < nextToLastLine; ++i)
-            positions[i + 1] = positions[i] + offset.distributionOffset + tracks[i].baseSize() + gap;
-        positions[lastLine] = positions[nextToLastLine] + tracks[nextToLastLine].baseSize();
+            positions[i + 1] = positions[i] + offset.distributionOffset + tracks[i].unclampedBaseSize() + gap;
+        positions[lastLine] = positions[nextToLastLine] + tracks[nextToLastLine].unclampedBaseSize();
 
         // Adjust collapsed gaps. Collapsed tracks cause the surrounding gutters to collapse (they
         // coincide exactly) except on the edges of the grid where they become 0.