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
Created attachment 369784 [details] Patch
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 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.
Created attachment 369790 [details] Patch
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 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
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 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
Created attachment 369843 [details] Patch for landing
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 on attachment 369843 [details] Patch for landing Clearing flags on attachment: 369843 Committed r245279: <https://trac.webkit.org/changeset/245279>
All reviewed patches have been landed. Closing bug.
<rdar://problem/50766816>