Bug 202258 – [css-grid] auto repeat broken in getComputedStyle for non-grid containers

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Oriol Brufau]

**What steps will reproduce the problem?**
Load this URL data:text/html,<div id="foo" style="grid-template-columns: [a] repeat(auto-fill, 10px [b]) [c]"></div><script>document.write(getComputedStyle(foo).gridTemplateColumns);</script>

**What is the expected result?**
You get `[a] repeat(auto-fill, 10px [b]) [c]`.
According to https://drafts.csswg.org/css-grid/#resolved-track-list, the resolved value is the computed value for non-grid displays.

**What happens instead?**
You get `[a]`. Or a failed assert crash in debug builds.

Has been fixed in Blink: https://crbug.com/988516

[Oriol Brufau]

Created attachment 379626 [details]
Patch

[Oriol Brufau]

Created attachment 379627 [details]
Patch

[Oriol Brufau]

Created attachment 379629 [details]
Patch

[Javier Fernandez]

Comment on attachment 379629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379629&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1257
> +    for (unsigned i = start; i < end; ++i) {

Wouldn't be equivalent, and clearer, to initialize 'i' to 'start + offset' ?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1297
> +        populateGridTrackList(list, collector, grid->trackSizesForComputedStyle(direction), [&](const LayoutUnit& v) {

I think 'list' has Ref<CSSValueList> as type, so either populateGridTrackList argument receives a Ref as well and you transfer the ownership or just use a non ref-counted type, like the  addValuesForNamedGridLinesAtIndex function does; I think this is the right approach, since populateGridTrackList doesn't need to keep or create a new object reference. In that case, you use  the 'get' function to get the raw pointer reference.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1311
> +        populateGridTrackList(list, collector, trackSizes, getTrackSize);

Ditto

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1317
> +    populateGridTrackList(list, collector, trackSizes, getTrackSize, 0, autoRepeatInsertionPoint);

Ditto

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1323
> +    populateGridTrackList(*repeatedValues, repeatCollector, autoRepeatTrackSizes, getTrackSize);

Ditto

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1327
> +    populateGridTrackList(list, collector, trackSizes, getTrackSize, autoRepeatInsertionPoint, trackSizes.size(), 1);

Ditto

[Oriol Brufau]

Comment on attachment 379629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379629&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1257
>> +    for (unsigned i = start; i < end; ++i) {
> 
> Wouldn't be equivalent, and clearer, to initialize 'i' to 'start + offset' ?

But then I would need tracks[i - offset]

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1297
>> +        populateGridTrackList(list, collector, grid->trackSizesForComputedStyle(direction), [&](const LayoutUnit& v) {
> 
> I think 'list' has Ref<CSSValueList> as type, so either populateGridTrackList argument receives a Ref as well and you transfer the ownership or just use a non ref-counted type, like the  addValuesForNamedGridLinesAtIndex function does; I think this is the right approach, since populateGridTrackList doesn't need to keep or create a new object reference. In that case, you use  the 'get' function to get the raw pointer reference.

OK

I will do these changes:

I will do these changes:

diff --git a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp
index d55eddb56b..5806fac7b2 100644
--- a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp
+++ b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp
@@ -1294,7 +1294,7 @@ static Ref<CSSValue> valueForGridTrackList(GridTrackSizingDirection direction, R
     if (isRenderGrid) {
         auto* grid = downcast<RenderGrid>(renderer);
         OrderedNamedLinesCollectorInGridLayout collector(style, isRowAxis, grid->autoRepeatCountForDirection(direction));
-        populateGridTrackList(list, collector, grid->trackSizesForComputedStyle(direction), [&](const LayoutUnit& v) {
+        populateGridTrackList(list.get(), collector, grid->trackSizesForComputedStyle(direction), [&](const LayoutUnit& v) {
             return zoomAdjustedPixelValue(v, style);
         });
         return list;
@@ -1308,23 +1308,23 @@ static Ref<CSSValue> valueForGridTrackList(GridTrackSizingDirection direction, R
 
     if (autoRepeatTrackSizes.isEmpty()) {
         // If there's no auto repeat(), just add all the line names and track sizes.
-        populateGridTrackList(list, collector, trackSizes, getTrackSize);
+        populateGridTrackList(list.get(), collector, trackSizes, getTrackSize);
         return list;
     }
 
     // Add the line names and track sizes that precede the auto repeat().
     unsigned autoRepeatInsertionPoint = isRowAxis ? style.gridAutoRepeatColumnsInsertionPoint() : style.gridAutoRepeatRowsInsertionPoint();
-    populateGridTrackList(list, collector, trackSizes, getTrackSize, 0, autoRepeatInsertionPoint);
+    populateGridTrackList(list.get(), collector, trackSizes, getTrackSize, 0, autoRepeatInsertionPoint);
 
     // Add a CSSGridAutoRepeatValue with the contents of the auto repeat().
     AutoRepeatType autoRepeatType = isRowAxis ? style.gridAutoRepeatColumnsType() : style.gridAutoRepeatRowsType();
-    RefPtr<CSSValueList> repeatedValues = CSSGridAutoRepeatValue::create(autoRepeatType == AutoRepeatType::Fill ? CSSValueAutoFill : CSSValueAutoFit);
+    auto repeatedValues = CSSGridAutoRepeatValue::create(autoRepeatType == AutoRepeatType::Fill ? CSSValueAutoFill : CSSValueAutoFit);
     OrderedNamedLinesCollectorInsideRepeat repeatCollector(style, isRowAxis);
-    populateGridTrackList(*repeatedValues, repeatCollector, autoRepeatTrackSizes, getTrackSize);
-    list->append(repeatedValues.releaseNonNull());
+    populateGridTrackList(repeatedValues.get(), repeatCollector, autoRepeatTrackSizes, getTrackSize);
+    list->append(repeatedValues.get());
 
     // Add the line names and track sizes that follow the auto repeat().
-    populateGridTrackList(list, collector, trackSizes, getTrackSize, autoRepeatInsertionPoint, trackSizes.size(), 1);
+    populateGridTrackList(list.get(), collector, trackSizes, getTrackSize, autoRepeatInsertionPoint, trackSizes.size(), 1);
     return list;
 }

[Oriol Brufau]

Created attachment 380101 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 380101 [details]
Patch

Clearing flags on attachment: 380101

Committed r250715: <https://trac.webkit.org/changeset/250715>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55978112>