Bug 197854 - [css-grid] Use max size to compute auto repeat tracks
Summary: [css-grid] Use max size to compute auto repeat tracks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords: BlinkMergeCandidate, BrowserCompat, InRadar
Depends on:
Blocks:
 
Reported: 2019-05-13 15:05 PDT by Manuel Rego Casasnovas
Modified: 2019-05-14 08:09 PDT (History)
8 users (show)

See Also:


Attachments
Patch (29.10 KB, patch)
2019-05-13 15:11 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (27.93 KB, patch)
2019-05-13 15:36 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (13.51 MB, application/zip)
2019-05-14 01:26 PDT, Build Bot
no flags Details
Patch for landing (27.96 KB, patch)
2019-05-14 07:33 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2019-05-13 15:05:35 PDT
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
Comment 1 Manuel Rego Casasnovas 2019-05-13 15:11:17 PDT
Created attachment 369784 [details]
Patch
Comment 2 Oriol Brufau 2019-05-13 15:31:20 PDT
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
Comment 3 Manuel Rego Casasnovas 2019-05-13 15:35:42 PDT
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.
Comment 4 Manuel Rego Casasnovas 2019-05-13 15:36:13 PDT
Created attachment 369790 [details]
Patch
Comment 5 Manuel Rego Casasnovas 2019-05-14 00:57:02 PDT
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?
Comment 6 Build Bot 2019-05-14 01:26:10 PDT
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
Comment 7 Build Bot 2019-05-14 01:26:12 PDT
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
Comment 8 Javier Fernandez 2019-05-14 02:10:43 PDT
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
Comment 9 Manuel Rego Casasnovas 2019-05-14 07:33:50 PDT
Created attachment 369843 [details]
Patch for landing
Comment 10 Manuel Rego Casasnovas 2019-05-14 07:40:41 PDT
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.
Comment 11 WebKit Commit Bot 2019-05-14 08:08:52 PDT
Comment on attachment 369843 [details]
Patch for landing

Clearing flags on attachment: 369843

Committed r245279: <https://trac.webkit.org/changeset/245279>
Comment 12 WebKit Commit Bot 2019-05-14 08:08:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-05-14 08:09:29 PDT
<rdar://problem/50766816>