Bug 163282

Summary:

[css-grid] Fix intrinsic maximums resolution with fit-content and auto

Product:

WebKit

Reporter:

Sergio Villar Senin <svillar>

Component:

New Bugs

Assignee:

Sergio Villar Senin <svillar>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, esprehn+autocc, glenn, jfernandez, kling, koivisto, kondapallykalyan, rego

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

60731, 163283

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	rego: review+

Sergio Villar Senin

Reported 2016-10-11 08:55:02 PDT

[css-grid] Fix intrinsic maximums resolution with fit-content and auto

Attachments
Patch (16.00 KB, patch) 2016-10-11 09:03 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (16.01 KB, patch) 2016-10-13 01:36 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (16.01 KB, patch) 2016-10-13 01:46 PDT, Sergio Villar Senin	rego: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Sergio Villar Senin

Comment 1 2016-10-11 09:03:58 PDT

Created attachment 291264 [details] Patch

Javier Fernandez

Comment 2 2016-10-12 04:52:58 PDT

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 > + Shouldn't we file a new WebKit bug for this issue ? > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:58 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:89 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:106 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:116 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums-expected.html:133 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:49 > + Ditto > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:64 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:95 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:112 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:121 > + Ditto. > LayoutTests/fast/css-grid-layout/grid-intrinsic-maximums.html:138 > + 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

Sergio Villar Senin

Comment 3 2016-10-13 01:13:52 PDT

(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 > > + > > 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.

Sergio Villar Senin

Comment 4 2016-10-13 01:36:39 PDT

Created attachment 291466 [details] Patch

Sergio Villar Senin

Comment 5 2016-10-13 01:46:22 PDT

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

Manuel Rego Casasnovas

Comment 6 2016-10-13 05:20:56 PDT

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.

Manuel Rego Casasnovas

Comment 7 2016-10-13 08:22:44 PDT

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.

Sergio Villar Senin

Comment 8 2016-10-13 08:27:59 PDT

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

Note You need to log in before you can comment on or make changes to this bug.