Bug 177300 – [css-grid] fit-content() tracks shouldn't stretch

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

The CSS WG has resolved that fit-content() tracks are not affected by stretch:
https://github.com/w3c/csswg-drafts/commit/a6b069786a80a2fd029fa38cc2d4cafa40576180

More info at https://github.com/w3c/csswg-drafts/issues/1732

[Manuel Rego Casasnovas]

Created attachment 321429 [details]
Patch

[Javier Fernandez]

Comment on attachment 321429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321429&action=review

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:997
> +            m_autoSizedTracksForStretchIndex.append(i);

I'm not sure about the new name for this vector. Before this change, we stored elements with 'auto' as max sizing function, hence the name. Now we are adding a more restrictive condition, but the name doesn't reflect that.
If we want a new name, associated to the use this vector will have I'd suggest m_stretchableTracks.

However, I wouldn't oppose to keep the current name.

> LayoutTests/fast/css-grid-layout/grid-content-alignment-stretch-only-valid-for-auto-sized-tracks.html:45
> +.fitContentTracks { grid: fit-content(20px) fit-content(20px) / fit-content(40px) fit-content(40px); font: 10px/1 Ahem; }

Wouldn't be clearer to have the font style in a separate rule ?

[Manuel Rego Casasnovas]

Comment on attachment 321429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321429&action=review

Thanks for the review, some comments inline.

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:997
>> +            m_autoSizedTracksForStretchIndex.append(i);
> 
> I'm not sure about the new name for this vector. Before this change, we stored elements with 'auto' as max sizing function, hence the name. Now we are adding a more restrictive condition, but the name doesn't reflect that.
> If we want a new name, associated to the use this vector will have I'd suggest m_stretchableTracks.
> 
> However, I wouldn't oppose to keep the current name.

Sergio asked for a new name in the Blink review, and I came up wit this "autoSizedTracksForStretch".
Let me explain my reasons, in the track sizing algorithm we have 2 steps:
stretchFlexibleTracks() and stretchAutoTracks().
That's why I didn't like the idea of "tracksForStretch" or "strechableTracks"
as it could be confused with the "stretchFlexibleTracks()" step.
And I preferred to keep the "autoSizedTracks" in the name,
as the step is actually called like that.

Also in the spec the steps are:
4. Expand Flexible Tracks
5. Expand Stretched auto Tracks
And the sections:
* 11.7. Expand Flexible Tracks (https://drafts.csswg.org/css-grid/#algo-flex-tracks)
* 11.8. Stretch auto Tracks (https://drafts.csswg.org/css-grid/#algo-stretch)

>> LayoutTests/fast/css-grid-layout/grid-content-alignment-stretch-only-valid-for-auto-sized-tracks.html:45
>> +.fitContentTracks { grid: fit-content(20px) fit-content(20px) / fit-content(40px) fit-content(40px); font: 10px/1 Ahem; }
> 
> Wouldn't be clearer to have the font style in a separate rule ?

Ahem font is only used in this grid, but I can put the rule directly in ".grid" as it won't affect the other cases.
Creating a new class ".ahemFont" only to be applied in ".fitContentTracks" grid doesn't look a great idea to me.

[WebKit Commit Bot]

Comment on attachment 321429 [details]
Patch

Clearing flags on attachment: 321429

Committed r222440: <http://trac.webkit.org/changeset/222440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693176>