Bug 197854 – [css-grid] Use max size to compute auto repeat tracks

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

This issues comes from a bug report in Firefox:
https://bugzilla.mozilla.org/show_bug.cgi?id=1544854

The example to reproduce the issue is:
https://bug1544854.bmoattachments.org/attachment.cgi?id=9058982

We should have 3 columns, when we have only 1.
The reason is that we don't use max-width to compute the columns.

Spec is very clear about that (https://drafts.csswg.org/css-grid/#auto-repeat):
> When auto-fill is given as the repetition number,
> if the grid container has a definite size or **max size** in the relevant axis

[Manuel Rego Casasnovas]

Created attachment 369784 [details]
Patch

[Oriol Brufau]

Comment on attachment 369784 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/w3c-import.log:28
> +/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-change-auto-repeat-tracks.html

Actually, this one will be imported in https://bugs.webkit.org/show_bug.cgi?id=197849, not here

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/w3c-import.log:51
> +/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-support-repeat-002.html

And this one in https://bugs.webkit.org/show_bug.cgi?id=197840

[Manuel Rego Casasnovas]

Comment on attachment 369784 [details]
Patch

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

Thanks for the review, issues fixed on the new version.

>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/w3c-import.log:28
>> +/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-change-auto-repeat-tracks.html
> 
> Actually, this one will be imported in https://bugs.webkit.org/show_bug.cgi?id=197849, not here

True, I forgot to update w3c-import.log sorry.

>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/w3c-import.log:51
>> +/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-support-repeat-002.html
> 
> And this one in https://bugs.webkit.org/show_bug.cgi?id=197840

Ditto.

[Manuel Rego Casasnovas]

Created attachment 369790 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 369790 [details]
Patch

Thanks for the informal review Oriol, but you're not reviewer yet, so I need an official review from someone else.
Javi could you take a look?

[EWS Watchlist]

Comment on attachment 369790 [details]
Patch

Attachment 369790 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12185544

New failing tests:
js/dom/custom-constructors.html

[EWS Watchlist]

Created attachment 369826 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Javier Fernandez]

Comment on attachment 369790 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:463
> +        LayoutUnit availableMaxSize = LayoutUnit();

I don't think we need to initialize to LayoutUnit()

> Source/WebCore/rendering/RenderGrid.cpp:472
> +        if (!availableMaxSize && !minSize.isSpecified())

I think the use of ! here is confusing (it may imply the availableMaxSize is an optional value) Wouldn't be better (clearer) to use > 0 instead ?

> Source/WebCore/rendering/RenderGrid.cpp:475
> +        LayoutUnit availableMinSize = LayoutUnit();

Ditto

[Manuel Rego Casasnovas]

Created attachment 369843 [details]
Patch for landing

[Manuel Rego Casasnovas]

Comment on attachment 369790 [details]
Patch

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

Thanks for the review.

>> Source/WebCore/rendering/RenderGrid.cpp:463
>> +        LayoutUnit availableMaxSize = LayoutUnit();
> 
> I don't think we need to initialize to LayoutUnit()

Ok.

>> Source/WebCore/rendering/RenderGrid.cpp:472
>> +        if (!availableMaxSize && !minSize.isSpecified())
> 
> I think the use of ! here is confusing (it may imply the availableMaxSize is an optional value) Wouldn't be better (clearer) to use > 0 instead ?

Ok, I've made it Optional so it's clearer now.

>> Source/WebCore/rendering/RenderGrid.cpp:475
>> +        LayoutUnit availableMinSize = LayoutUnit();
> 
> Ditto

Ok.

[WebKit Commit Bot]

Comment on attachment 369843 [details]
Patch for landing

Clearing flags on attachment: 369843

Committed r245279: <https://trac.webkit.org/changeset/245279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50766816>