Bug 145520 – [CSS Grid Layout] Support for Content Alignment in grid layout

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

[Javier Fernandez]

Now that Content Alignment CSS properties were upgrade to the new CSS Box Alignment specification is time now to implement their functionality in Grid Layout. 

This bug is about the align-content support, which means that we should be able to align the grid container along the block/column axis supporting different writing modes and flow directions.

[Javier Fernandez]

Created attachment 254093 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderGrid.cpp:417:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 27 files


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

[Javier Fernandez]

Created attachment 254097 [details]
Patch

[Darin Adler]

Comment on attachment 254097 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:132
> +    ContentAlignmentData()
> +        : positionOffset(-1)
> +        , distributionOffset(-1)
> +    {
> +    }
> +
> +    ContentAlignmentData(LayoutUnit position, LayoutUnit distribution)
> +        : positionOffset(position)
> +        , distributionOffset(distribution)
> +    {
> +    }

We should omit these.

> Source/WebCore/rendering/RenderGrid.cpp:135
> +    LayoutUnit positionOffset;
> +    LayoutUnit distributionOffset;

We should initialize these like this:

    LayoutUnit positionOffset { -1 };
    LayoutUnit distributionOffset { -1 };

> Source/WebCore/rendering/RenderGrid.cpp:137
> +    bool isValid() { return positionOffset > -1 && distributionOffset > -1; };

Should be const.

Using -1 as a magic number to mean invalid is really ugly. Do we need to follow this pattern?

> Source/WebCore/rendering/RenderGrid.cpp:1527
> +        return ContentAlignmentData();

Should just be:

    return { };

> Source/WebCore/rendering/RenderGrid.cpp:1534
> +        return ContentAlignmentData(0, availableFreeSpace / (numberOfGridTracks - 1));

Should just be:

    return { 0 , availableFreeSpace / (numberOfGridTracks - 1) };

> Source/WebCore/rendering/RenderGrid.cpp:1629
> +        // crbug.com/234191

Hmm, lets not use a crbug URL in WebKit.

[Javier Fernandez]

*** Bug 145521 has been marked as a duplicate of this bug. ***

[Javier Fernandez]

Created attachment 254178 [details]
Patch

Applied suggested changes.

[Sergio Villar Senin]

Comment on attachment 254178 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +

This does not look correct. It isn't only for columns right?

> Source/WebCore/rendering/RenderGrid.cpp:411
> +        distributeSpaceToTracks<MaximizeTracks>(tracksForDistribution, needToStretch ? &tracksToStretch : nullptr, availableLogicalSpace);

Hmm I don't understand this change. The second argument of distributeSpaceToTracks should be used just to specify the tracks that should grow over the limits, something that is clearly specified in the algorithm. You are using this for a completely different thing.

Also it was nullptr on purpose because on the step 3 of the algorithm we should just grow tracks to their maximums, we should not grow over that limit at this point.

> Source/WebCore/rendering/RenderGrid.cpp:1272
> +    m_columnPositions.resize(numberOfColumnTracks + 1);

Why resize() instead of resizeToFit() ?

> Source/WebCore/rendering/RenderGrid.cpp:1279
> +    m_rowPositions.resize(numberOfRowTracks + 1);

Ditto.

> Source/WebCore/rendering/RenderGrid.cpp:1525
> +        return nullptr;

Perhaps move this check above the previous one.

> Source/WebCore/rendering/RenderGrid.cpp:1557
> +    ContentAlignmentData* contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks);

You're leaking the returned object as it was created with new.

> Source/WebCore/rendering/RenderGrid.cpp:1598
> +    ContentAlignmentData* contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks);

You're leaking the returned object as it was created with new.

> Source/WebCore/rendering/RenderGrid.h:43
> +struct ContentAlignmentData;

Seems like this is not needed here?

[Javier Fernandez]

Comment on attachment 254178 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +
> 
> This does not look correct. It isn't only for columns right?

Sure, it was like that in the first versions of the patch, but I ended up implementing logic for both properties. I'll rephrase it accordingly.

>> Source/WebCore/rendering/RenderGrid.cpp:411
>> +        distributeSpaceToTracks<MaximizeTracks>(tracksForDistribution, needToStretch ? &tracksToStretch : nullptr, availableLogicalSpace);
> 
> Hmm I don't understand this change. The second argument of distributeSpaceToTracks should be used just to specify the tracks that should grow over the limits, something that is clearly specified in the algorithm. You are using this for a completely different thing.
> 
> Also it was nullptr on purpose because on the step 3 of the algorithm we should just grow tracks to their maximums, we should not grow over that limit at this point.

umm, it's really a good catch, indeed. I must look for an alternative approach to implement stretch which should happen after the track sizing algorithm is executed.

>> Source/WebCore/rendering/RenderGrid.cpp:1272
>> +    m_columnPositions.resize(numberOfColumnTracks + 1);
> 
> Why resize() instead of resizeToFit() ?

It was not my intention to change that; I just wanted to use numberOfColumnTracks so I'll keep the original function.

>> Source/WebCore/rendering/RenderGrid.cpp:1525
>> +        return nullptr;
> 
> Perhaps move this check above the previous one.

We can t do that because we need to resolve the fallback position before returning.

>> Source/WebCore/rendering/RenderGrid.cpp:1557
>> +    ContentAlignmentData* contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks);
> 
> You're leaking the returned object as it was created with new.

Sure, I'll fix that.

>> Source/WebCore/rendering/RenderGrid.cpp:1598
>> +    ContentAlignmentData* contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks);
> 
> You're leaking the returned object as it was created with new.

I need to fix this too, indeed.

>> Source/WebCore/rendering/RenderGrid.h:43
>> +struct ContentAlignmentData;
> 
> Seems like this is not needed here?

Yes it is, some header functions return that type.

[Sergio Villar Senin]

Comment on attachment 254178 [details]
Patch

Let's not land this

[Javier Fernandez]

Created attachment 254265 [details]
Patch

Fixed commented issues and changed the approach to deal with stretch and spans.

[Sergio Villar Senin]

Comment on attachment 254265 [details]
Patch

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

Looks good in general. Some nits here and there. 

I'm setting r- because the cache thing is still not completely well resolved. We also need the CSSWG clarifications for some of the stuff you're adding before landing.

> Source/WebCore/rendering/RenderGrid.cpp:371
> +    sizingData.invalidateTrackPositionsCache(direction);

Wouldn't it make sense just to explicitly initialize the invalid flag in GridSizingData ?

> Source/WebCore/rendering/RenderGrid.cpp:1189
> +    // We consider auto-sized tracks as content-sized (min-content, max-content, auto).

We need a clarification in the specs for this.

> Source/WebCore/rendering/RenderGrid.cpp:1191
> +    unsigned numberAutoSizedTracks = autoSizedTracksIndex.size();

You're copying a Vector here just to get the size, used sizingData.contentSizedTracksIndex.size() directly instead

> Source/WebCore/rendering/RenderGrid.cpp:1195
> +        return;

Actually all these checks doesn't require checking the size. I'd modify the order of the checks and put sizingData.contentSizedTracksIndex.size < 1, at the very end so that we don't normally need to even check it.

> Source/WebCore/rendering/RenderGrid.cpp:1204
> +        // FIXME: Respecting the constraints imposed by max-height/max-width.

I'll wait for the CSSWG reply before giving the r+ as it wouldn't make sense to approve this to change it soon.

> Source/WebCore/rendering/RenderGrid.cpp:1293
> +    if (!trackPositionsCacheIsValid)

This does not make sense, you have an ASSERT(trackPositionsCacheIsValid) just two lines above, so it's either something that is always true (and thus the ASSERT) or an allowed value (and thus the "if" check)

> Source/WebCore/rendering/RenderGrid.cpp:1325
> +    sizingData.rowTracksPositionsCacheValid = true;

Do we ever modify the m_rowPositions without modifying m_columnPositions or viceversa? If so we just need 1 boolean instead of two.

Also it's a bit weird to have a function to invalidate the cache and at the same time validate it directly by setting the flags to true. So I guess we better have a setWhateverCacheValid(bool) instead.

[Javier Fernandez]

Comment on attachment 254265 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:371
>> +    sizingData.invalidateTrackPositionsCache(direction);
> 
> Wouldn't it make sense just to explicitly initialize the invalid flag in GridSizingData ?

I'm not sure. I think this method should explicitly invalidate the cache because it builds again the track position vectors. It's true that in the current code we always pass a newly created GridSizingData instance, but I think we shouldn't trust on that., make the logic as independent as possible in order to be safe against future code refactoring.

>> Source/WebCore/rendering/RenderGrid.cpp:1189
>> +    // We consider auto-sized tracks as content-sized (min-content, max-content, auto).
> 
> We need a clarification in the specs for this.

Agreed.

>> Source/WebCore/rendering/RenderGrid.cpp:1191
>> +    unsigned numberAutoSizedTracks = autoSizedTracksIndex.size();
> 
> You're copying a Vector here just to get the size, used sizingData.contentSizedTracksIndex.size() directly instead

Well, it's used as well in the loop below. This way we only copy it once.

>> Source/WebCore/rendering/RenderGrid.cpp:1195
>> +        return;
> 
> Actually all these checks doesn't require checking the size. I'd modify the order of the checks and put sizingData.contentSizedTracksIndex.size < 1, at the very end so that we don't normally need to even check it.

Good idea.

>> Source/WebCore/rendering/RenderGrid.cpp:1204
>> +        // FIXME: Respecting the constraints imposed by max-height/max-width.
> 
> I'll wait for the CSSWG reply before giving the r+ as it wouldn't make sense to approve this to change it soon.

Agreed.

>> Source/WebCore/rendering/RenderGrid.cpp:1293
>> +    if (!trackPositionsCacheIsValid)
> 
> This does not make sense, you have an ASSERT(trackPositionsCacheIsValid) just two lines above, so it's either something that is always true (and thus the ASSERT) or an allowed value (and thus the "if" check)

I tried to implement a safe path for release, but yeah, not a lot of sense, indeed.

>> Source/WebCore/rendering/RenderGrid.cpp:1325
>> +    sizingData.rowTracksPositionsCacheValid = true;
> 
> Do we ever modify the m_rowPositions without modifying m_columnPositions or viceversa? If so we just need 1 boolean instead of two.
> 
> Also it's a bit weird to have a function to invalidate the cache and at the same time validate it directly by setting the flags to true. So I guess we better have a setWhateverCacheValid(bool) instead.

Again, I'd rather keep logic independence for this cache thing. We may want to modify either row or column tracks only so I think it's better to maintain separated caches for them.

[Javier Fernandez]

Created attachment 262053 [details]
New patch using now the min/maxTrackBreadth auto values logic, which landed recently.

[Build Bot]

Comment on attachment 262053 [details]
New patch using now the min/maxTrackBreadth auto values logic, which landed recently.

Attachment 262053 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/222998

New failing tests:
fast/repaint/justify-self-overflow-change.html
fast/repaint/justify-items-overflow-change.html

[Build Bot]

Created attachment 262054 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 262053 [details]
New patch using now the min/maxTrackBreadth auto values logic, which landed recently.

Attachment 262053 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/222994

New failing tests:
fast/repaint/justify-self-overflow-change.html
fast/repaint/justify-items-overflow-change.html

[Build Bot]

Created attachment 262055 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Javier Fernandez]

Created attachment 262062 [details]
Fixed layout tests failures.

[Darin Adler]

Comment on attachment 262062 [details]
Fixed layout tests failures.

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

> Source/WebCore/rendering/RenderGrid.cpp:127
> +    ContentAlignmentData() { };
> +    ContentAlignmentData(LayoutUnit position, LayoutUnit distribution)
> +        : positionOffset(position)
> +        , distributionOffset(distribution)
> +    {
> +    }

These constructors aren’t needed. We can already construct this object with default values, or with explicit values in aggregate syntax. Don’t need constructors.

> Source/WebCore/rendering/RenderGrid.cpp:1257
> +        GridTrack* track = tracks.data() + trackIndex;

I’d write:

    GridTrack& track = tracks[trackIndex];

> Source/WebCore/rendering/RenderGrid.cpp:1359
> +    const Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;
> +    const GridCoordinate& coordinate = cachedGridCoordinate(child);
> +    const GridSpan& span = (direction == ForColumns) ? coordinate.columns : coordinate.rows;
> +    const Vector<LayoutUnit>& linePositions = (direction == ForColumns) ? m_columnPositions : m_rowPositions;

I’d use more auto here. The types aren’t so important.

> Source/WebCore/rendering/RenderGrid.cpp:1683
> +{
> +    return (distribution == ContentDistributionStretch || ContentDistributionStretch == ContentDistributionDefault) ? LayoutUnit(0) : trackPositions[1] - trackPositions[0] - childBreadth;
> +
> +}

Stray blank line here.

[Javier Fernandez]

Created attachment 262158 [details]
Applied suggested changes.

[Javier Fernandez]

Created attachment 262159 [details]
Patch

Patch rebased and applied suggested changes.

[Javier Fernandez]

Created attachment 262244 [details]
Patch

Fixed compilation errors.

[Sergio Villar Senin]

Comment on attachment 262244 [details]
Patch

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

It's a r+ for me too but please correct the few minor things I spotted.

> Source/WebCore/rendering/RenderGrid.cpp:1343
> +    // Track Positions vector stores the 'start' grid line of each track, so w have to add last track's baseSize.

Nit: "so we". Also perhaps add some empty lines before and after defining the xxxTrackPosition just to make this block more readable.

> Source/WebCore/rendering/RenderGrid.cpp:1662
> +    return (distribution == ContentDistributionStretch || ContentDistributionStretch == ContentDistributionDefault) ? LayoutUnit(0) : trackPositions[1] - trackPositions[0] - childBreadth;

Nit: just use LayoutUnit() which is a bit faster than LayoutUnit(0)

> Source/WebCore/rendering/RenderGrid.cpp:1668
> +    size_t childStartLine = coordinate.rows.resolvedInitialPosition.toInt();

In Webkit we switched from size_t to unsigned so better use unsigned.

> Source/WebCore/rendering/RenderGrid.cpp:1679
> +        size_t childEndLine = coordinate.rows.resolvedFinalPosition.next().toInt();

Ditto unsigned.

> Source/WebCore/rendering/RenderGrid.cpp:1697
> +    size_t childStartLine = coordinate.columns.resolvedInitialPosition.toInt();

Ditto.

> Source/WebCore/rendering/RenderGrid.cpp:1708
> +        size_t childEndLine = coordinate.columns.resolvedFinalPosition.next().toInt();

Ditto.

[Javier Fernandez]

Created attachment 262326 [details]
Patch

Applied additional suggested changes.

[WebKit Commit Bot]

Comment on attachment 262326 [details]
Patch

Clearing flags on attachment: 262326

Committed r190484: <http://trac.webkit.org/changeset/190484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.