Bug 202258 - [css-grid] auto repeat broken in getComputedStyle for non-grid containers
Summary: [css-grid] auto repeat broken in getComputedStyle for non-grid containers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oriol Brufau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-26 01:40 PDT by Oriol Brufau
Modified: 2019-10-04 03:07 PDT (History)
10 users (show)

See Also:


Attachments
Patch (39.29 KB, patch)
2019-09-26 01:51 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (39.34 KB, patch)
2019-09-26 02:40 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (39.34 KB, patch)
2019-09-26 03:13 PDT, Oriol Brufau
jfernandez: review+
Details | Formatted Diff | Diff
Patch (39.34 KB, patch)
2019-10-03 05:55 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oriol Brufau 2019-09-26 01:40:12 PDT
**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
Comment 1 Oriol Brufau 2019-09-26 01:51:28 PDT
Created attachment 379626 [details]
Patch
Comment 2 Oriol Brufau 2019-09-26 02:40:35 PDT
Created attachment 379627 [details]
Patch
Comment 3 Oriol Brufau 2019-09-26 03:13:13 PDT
Created attachment 379629 [details]
Patch
Comment 4 Javier Fernandez 2019-10-02 02:42:23 PDT
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
Comment 5 Oriol Brufau 2019-10-03 05:54:25 PDT
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;
 }
Comment 6 Oriol Brufau 2019-10-03 05:55:19 PDT
Created attachment 380101 [details]
Patch
Comment 7 WebKit Commit Bot 2019-10-04 03:06:27 PDT
Comment on attachment 380101 [details]
Patch

Clearing flags on attachment: 380101

Committed r250715: <https://trac.webkit.org/changeset/250715>
Comment 8 WebKit Commit Bot 2019-10-04 03:06:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-10-04 03:07:22 PDT
<rdar://problem/55978112>