Bug 161379 – [css-grid] Implement fit-content track size

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

[Sergio Villar Senin]

[css-grid] Implement fit-content track size

[Sergio Villar Senin]

Created attachment 287390 [details]
Patch

[WebKit Commit Bot]

Attachment 287390 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1033:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1034:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1035:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Created attachment 287392 [details]
Patch

[Javier Fernandez]

Comment on attachment 287392 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:416
> +void RenderGrid::computeTrackSizesForDirection(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit availableSpace)

Perhaps it'd be a good idea to start removing the "ForDirection" suffix we have in many functions, as you have done already with the freeSpace/setFreeSpace methods.

> Source/WebCore/rendering/RenderGrid.cpp:420
> +    sizingData.setAvailableSpace(availableSpace);

Why gutters are not subtracted from availableSpace, as we do with freeSpace ?

> Source/WebCore/rendering/RenderGrid.cpp:690
> +    const bool hasDefiniteFreeSpace = sizingData.sizingOperation == TrackSizing;

Is this really true for both axis ?

> Source/WebCore/rendering/RenderGrid.cpp:1186
> +static LayoutUnit trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase phase, GridTrack& track, TrackSizeRestriction restriction)

const GridTrack& track

> Source/WebCore/rendering/RenderGrid.cpp:1439
> +    const LayoutUnit maxSize = sizingData.freeSpace(direction).valueOr(0);

Why const ?

[Javier Fernandez]

Comment on attachment 287392 [details]
Patch

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

I think the patch lacks of tests for the new parsing logic added.

> Source/WebCore/css/parser/CSSParser.cpp:5936
> +    auto* arguments = downcast<CSSFunctionValue>(value).arguments();

Perhaps better using a reference.
   auto& arguments = 

Otherwise we should have nullchecks afterwards.

> Source/WebCore/rendering/RenderGrid.cpp:44
> +enum TrackSizeRestriction {

Wouldn't be enough a boolean variable instead of such a simple enumeration ?

> Source/WebCore/rendering/RenderGrid.cpp:704
> +                track.setGrowthLimitCap(valueForLength(gridLength.length(), maxSize));

we may call gridLength.length() even in the case that !gridLength.isLength() 
Is that correct ?

> LayoutTests/fast/css-grid-layout/fit-content-columns-expected.html:70
> +	<div class="item">XXX</div>

Wouldn't be clearer with some indentation ?

[Sergio Villar Senin]

(In reply to comment #4)
> Comment on attachment 287392 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287392&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:416
> > +void RenderGrid::computeTrackSizesForDirection(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit availableSpace)
> 
> Perhaps it'd be a good idea to start removing the "ForDirection" suffix we
> have in many functions, as you have done already with the
> freeSpace/setFreeSpace methods.

Yes definitely. They make names more complex without a clear reason.

> > Source/WebCore/rendering/RenderGrid.cpp:420
> > +    sizingData.setAvailableSpace(availableSpace);
> 
> Why gutters are not subtracted from availableSpace, as we do with freeSpace ?

Because the available space is the one you have to put tracks and gutters. The free space on the other hand is the one that is free after sizing tracks.
 
> > Source/WebCore/rendering/RenderGrid.cpp:690
> > +    const bool hasDefiniteFreeSpace = sizingData.sizingOperation == TrackSizing;
> 
> Is this really true for both axis ?

Yes.
 
> > Source/WebCore/rendering/RenderGrid.cpp:1186
> > +static LayoutUnit trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase phase, GridTrack& track, TrackSizeRestriction restriction)
> 
> const GridTrack& track
> 
> > Source/WebCore/rendering/RenderGrid.cpp:1439
> > +    const LayoutUnit maxSize = sizingData.freeSpace(direction).valueOr(0);
> 
> Why const ?

Well because we do not want to modify it.

[Sergio Villar Senin]

Comment on attachment 287392 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/css/parser/CSSParser.cpp:5936
>> +    auto* arguments = downcast<CSSFunctionValue>(value).arguments();
> 
> Perhaps better using a reference.
>    auto& arguments = 
> 
> Otherwise we should have nullchecks afterwards.

OK

>> Source/WebCore/rendering/RenderGrid.cpp:44
>> +enum TrackSizeRestriction {
> 
> Wouldn't be enough a boolean variable instead of such a simple enumeration ?

There is literature about why enums are much better than booleans for the APIs. See for example https://blogs.msdn.microsoft.com/brada/2004/01/12/design-guidelines-update-enums-vs-boolean-arguments/

>> Source/WebCore/rendering/RenderGrid.cpp:704
>> +                track.setGrowthLimitCap(valueForLength(gridLength.length(), maxSize));
> 
> we may call gridLength.length() even in the case that !gridLength.isLength() 
> Is that correct ?

That's indeed a mistake, good catch. The actual condition is (!gridLength.isPercentage() || hasDefiniteFreeSpace)

>>> Source/WebCore/rendering/RenderGrid.cpp:1186
>>> +static LayoutUnit trackSizeForTrackSizeComputationPhase(TrackSizeComputationPhase phase, GridTrack& track, TrackSizeRestriction restriction)
>> 
>> const GridTrack& track
> 
> Well because we do not want to modify it.

Sorry this comment was not for this line.

>> LayoutTests/fast/css-grid-layout/fit-content-columns-expected.html:70
>> +	<div class="item">XXX</div>
> 
> Wouldn't be clearer with some indentation ?

Sure, I should have done something wrong before uploading...

[Sergio Villar Senin]

Created attachment 287517 [details]
Patch

[Darin Adler]

Comment on attachment 287517 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:719
>          ASSERT(!track.growthLimitIsInfinite());

RenderGrid.cpp:719:23: error: 'growthLimitIsInfinite' is a private member of 'WebCore::GridTrack'

[Sergio Villar Senin]

Created attachment 288128 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 288128 [details]
Patch

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

Amazing patch! I've to admit that I don't get all the details of the track sizing algorithm though.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1029
> +        fitContentTrackSize->append(specifiedValueForGridTrackBreadth(trackSize.fitContentTrackBreadth(), style));

I guess we shouldn't allow "1fr" or things like that for fit-content.
Probably we should use something different than specifiedValueForGridTrackBreadth().
And add a test to verify that this syntax is wrong.

> Source/WebCore/css/parser/CSSParser.cpp:6097
> +        if (!trackBreadth || trackBreadth->isFlex())

Do we need the isFlex() check here?
I guess if it's "1fr" it should be already invalid in the previous if.
Maybe we can move the isFlex() to an assert after.

> Source/WebCore/rendering/RenderGrid.cpp:105
> +        m_tempSize += tempSize;

Shouldn't we use setTemptSize() to be sure that we fulfill the ASSERTs?

> Source/WebCore/rendering/RenderGrid.cpp:1181
> +            ASSERT(trackSize.fitContentTrackBreadth().isLength());

Is it possible it's not a length?

> Source/WebCore/rendering/RenderGrid.cpp:1182
> +            growthLimit = std::min(growthLimit, valueForLength(trackSize.fitContentTrackBreadth().length(), sizingData.availableSpace().valueOr(LayoutUnit())));

Nit: We've a mix of valueOr(0) vs valueOr(LayoutUnit()).

[Sergio Villar Senin]

Committed r205966: <http://trac.webkit.org/changeset/205966>

[Ryan Haddad]

13 css-grid-layout tests are failing this assertion with this change:

ASSERT(growthLimitIsInfinite() || growthLimit() >= m_tempSize)/

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r205966%20(8189)/results.html
https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r205966%20(14999)/results.html

[Sergio Villar Senin]

Reverted r205966 for reason:

Triggered some assertions

Committed r205972: <http://trac.webkit.org/changeset/205972>

[Sergio Villar Senin]

Created attachment 288955 [details]
Patch for landing

I added an extra wrong assertion in the very last moment inspired by one of the review comments. The assert was wrong because the tempSize could be sometimes larger than the cap during the 'grow over the limits' phase

[Sergio Villar Senin]

Comment on attachment 288955 [details]
Patch for landing

Clearing flags on attachment: 288955

Committed r205977: <http://trac.webkit.org/changeset/205977>

[Sergio Villar Senin]

All reviewed patches have been landed.  Closing bug.