Bug 163282

Created attachment 291264 [details] Patch

Comment on attachment 291264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291264&action=review The change looks good to me. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:43 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Shouldn't we file a new WebKit bug for this issue ? > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:58 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:89 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:106 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:116 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:133 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:49 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:64 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:95 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:112 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:121 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:138 > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:140 > +<div class="grid gridAutoMinContent" style="justify-items: start"> I'd rather using the style classes in grid-alignment.css > Source/WebCore/rendering/style/GridTrackSize.h:108 > + // This values depend on the above ones so keep them here. s/This/These

(In reply to comment #2) > Comment on attachment 291264 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291264&action=review > > The change looks good to me. > > > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:43 > > +<!-- Fails due to https://bugs.chromium.org/p/chromium/issues/detail?id=654737 --> > > Shouldn't we file a new WebKit bug for this issue ? Beh, I just forgot to update that file. The bug was indeed reported days ago: https://bugs.webkit.org/show_bug.cgi?id=163283 > > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:140 > > +<div class="grid gridAutoMinContent" style="justify-items: start"> > > I'd rather using the style classes in grid-alignment.css Right. > > Source/WebCore/rendering/style/GridTrackSize.h:108 > > + // This values depend on the above ones so keep them here. > > s/This/These OK.

Created attachment 291466 [details] Patch

Created attachment 291467 [details] Patch Forgot about updating one comment

Comment on attachment 291467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291467&action=review The patch looks good to me, but I've a hard time understanding the results of the last 6 cases on the test. Probably I'd need to re-read the track sizing algorithm again. :-) > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:135 > +<div class="grid" style="grid-template: 20px / calc(105px/2) 5px calc(85px/2);" style="justify-items: start"> Nit: You're already setting "justify-items: start" in the CSS.

Comment on attachment 291467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291467&action=review After a private conversation and a review of the spec it seems that the results are the expected ones. r=me > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:129 > +<div class="grid gridAutoMinContent"> > + <div class="item" style="min-width: 15px;">XXXX XXXX</div> > + <div class="abs col1"></div> > + <div class="abs col2"></div> > + <div class="abs col3"></div> > +</div> This was one of the hard to get for me, but now it's more clear. The algorithm does the following: 1) The minimums are easy: [0, ?] [5, ?] [0, ?] 2) Then for maximums, the 2nd column is fixed so it's 5px. And we use 10 px to be shared in the other columns (10px are get from 15px min-width - 5px of the 2nd column): [0, 5] [5, 5] [0, 5] 3) Then the first column maximum grows as it's auto, so it can be 90px: [0, 90] [5, 5] [0, 5] As the width of the grid container is 100px, it's fine that the columns are 90px, 5px and 5px.

Committed r207288: <http://trac.webkit.org/changeset/207288>