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

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

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

[Sergio Villar Senin]

Created attachment 291264 [details]
Patch

[Javier Fernandez]

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

[Sergio Villar Senin]

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

[Sergio Villar Senin]

Created attachment 291466 [details]
Patch

[Sergio Villar Senin]

Created attachment 291467 [details]
Patch

Forgot about updating one comment

[Manuel Rego Casasnovas]

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 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]

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