Bug 108403 – [CSS Grid Layout] computePreferredLogicalWidths doesn't handle minmax tracks

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

The layout code properly accounts for minmax but computePreferredLogicalWidths wasn't updated and thus doesn't compute the min / max preferred logical widths of the grid element properly.

We should be able to see the bad sizing using fill-available on the grid element or its container. While at it, 'auto' should also be covered by the tests now that we also support it.

[Julien Chaffraix]

Created attachment 185859 [details]
Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination.

[Tony Chang]

Comment on attachment 185859 [details]
Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination.

I defer computePreferredLogicalWidths to Ojan.

[Ojan Vafai]

Comment on attachment 185859 [details]
Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination.

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

Just a few minor issues.

> Source/WebCore/rendering/RenderGrid.cpp:164
> +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(PreferredWidthType type, size_t trackIndex) const

Instead of passing in the type, can we just pass in the Length itself? Then we don't need the enum, which seems better to me.

> Source/WebCore/rendering/RenderGrid.cpp:169
> +    if (length.isFixed())
> +        return length.intValue();

It's not possible to put border/padding/margin on a grid cell, right? If so, it might be worth adding a comment here about that being why it's OK to not worry about them here.

> Source/WebCore/rendering/RenderGrid.cpp:178
> +            // FIXME: We should include the child's fixed margins like RenderBlock.

RenderBlock does crazy with margins. You might want to point to RenderFlexibleBox as a closer example. In fact, I think we probably want to do exactly what RenderFlexibleBox does, which might involve moving marginLogicalWidthForChild up into RenderBlock so it can be shared.

> Source/WebCore/rendering/RenderGrid.cpp:198
> +    // FIXME: What should we do for the other values (e.g. <percentage> and calc())
> +    return 0;

Right now, every other class treats anything other than Fixed the same a Auto. The sizing spec says to resolve "definite" lengths, but we don't do that anywhere currently. We should try to and see if we can get away with it without breaking the web, but I've been putting that off until I'm done reducing code duplication across computePreferredLogicalWidth overrides.

> LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html:23
> +.gridMinMinContent {
> +    -webkit-grid-columns: minmax(-webkit-min-content, 40px) minmax(-webkit-min-content, 40px);
> +    -webkit-grid-rows: 10px;
> +    font: 10px/1 Ahem;
> +}
> +
> +.gridMinMaxContent {
> +    -webkit-grid-columns: minmax(30px, -webkit-min-content) minmax(30px, -webkit-min-content);
> +    -webkit-grid-rows: 10px;
> +    font: 10px/1 Ahem;
> +}
> +
> +.gridMaxContent {

Nit: these class names don't seem to use a consistent naming scheme.

[Ojan Vafai]

Comment on attachment 185859 [details]
Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination.

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

> Source/WebCore/rendering/RenderGrid.cpp:173
> +        for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {

Also, maybe add a FIXME to come up with a more efficient way of iterating over the grid items in a given track?

[Julien Chaffraix]

Comment on attachment 185859 [details]
Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination.

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

>> Source/WebCore/rendering/RenderGrid.cpp:164
>> +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(PreferredWidthType type, size_t trackIndex) const
> 
> Instead of passing in the type, can we just pass in the Length itself? Then we don't need the enum, which seems better to me.

I started with that and came to the opposite conclusion :)

Will do the change as it's fine either way.

>> Source/WebCore/rendering/RenderGrid.cpp:169
>> +        return length.intValue();
> 
> It's not possible to put border/padding/margin on a grid cell, right? If so, it might be worth adding a comment here about that being why it's OK to not worry about them here.

It's not possible (and the grid cell is now called grid area)

>> Source/WebCore/rendering/RenderGrid.cpp:173
>> +        for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
> 
> Also, maybe add a FIXME to come up with a more efficient way of iterating over the grid items in a given track?

This is at the back of my mind as we are starting to have *a lot* of grid items' iterations in the code. grid-auto-flow is really the point where having a grid representation for the grid items is required to determine efficiently if a grid area is empty or not.

>> Source/WebCore/rendering/RenderGrid.cpp:178
>> +            // FIXME: We should include the child's fixed margins like RenderBlock.
> 
> RenderBlock does crazy with margins. You might want to point to RenderFlexibleBox as a closer example. In fact, I think we probably want to do exactly what RenderFlexibleBox does, which might involve moving marginLogicalWidthForChild up into RenderBlock so it can be shared.

Sounds good.

>> Source/WebCore/rendering/RenderGrid.cpp:198
>> +    return 0;
> 
> Right now, every other class treats anything other than Fixed the same a Auto. The sizing spec says to resolve "definite" lengths, but we don't do that anywhere currently. We should try to and see if we can get away with it without breaking the web, but I've been putting that off until I'm done reducing code duplication across computePreferredLogicalWidth overrides.

Sounds good, will keep the FIXME and expand it to add some of this explanation.

[Julien Chaffraix]

Created attachment 185910 [details]
Proposed fix 2: Updated after Ojan's comments.

[Ojan Vafai]

Comment on attachment 185910 [details]
Proposed fix 2: Updated after Ojan's comments.

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

> Source/WebCore/rendering/RenderGrid.cpp:164
> +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(const Length& length, size_t trackIndex) const

Didn't notice this in the last patch. This might be a bit more consistent with RenderBox if you called this computePreferredTrackWidthUsing or something like that (e.g. see RenderBox::computeLogicalHeightUsing). resolve is a fairly overloaded verb. :)

> Source/WebCore/rendering/RenderGrid.cpp:200
> +    // (including <percentage> and calc()) but we don't it elsewhere.

Missing a word or two here. :)

[Julien Chaffraix]

Comment on attachment 185910 [details]
Proposed fix 2: Updated after Ojan's comments.

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

>> Source/WebCore/rendering/RenderGrid.cpp:164
>> +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(const Length& length, size_t trackIndex) const
> 
> Didn't notice this in the last patch. This might be a bit more consistent with RenderBox if you called this computePreferredTrackWidthUsing or something like that (e.g. see RenderBox::computeLogicalHeightUsing). resolve is a fairly overloaded verb. :)

Fair enough.

My take would be computePreferredTrackWidth, dropping the Using as I don't see much value in it.

[Julien Chaffraix]

Created attachment 186040 [details]
Patch for landing.

[Julien Chaffraix]

Created attachment 186072 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 186072 [details]
Patch for landing

Clearing flags on attachment: 186072

Committed r141616: <http://trac.webkit.org/changeset/141616>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.