Bug 113085 – [CSS Grid Layout] content-sized row tracks with percentage logical height grid items don't resolve properly

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

[Julien Chaffraix]

This was uncovered by bug 103333. See the failing tests in http://trac.webkit.org/browser/trunk/LayoutTests/fast/css-grid-layout/grid-auto-columns-rows-auto-flow-resolution-expected.txt

This is because we try to resolve a grid item's percentage logical height against the grid size (as we don't have a containing block for the grid area, using the override).

[Julien Chaffraix]

Created attachment 194604 [details]
Proposed change 1: Set our override logical height to -1 to prevent percent resolution.

[Tony Chang]

Comment on attachment 194604 [details]
Proposed change 1: Set our override logical height to -1 to prevent percent resolution.

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

> Source/WebCore/ChangeLog:11
> +        resolve percentage logical height against the grid element (wrong containing block
> +        as a grid item's containing is the grid area).

"wrong containing block
as a grid item's containing *block* is the grid area"

> Source/WebCore/rendering/RenderGrid.cpp:377
> -    child->clearOverrideContainingBlockContentLogicalHeight();
> +    // If |child| has a percentage logical height, we shouldn't let it override its intrinsic height, which is
> +    // what we are interested in here. Thus we need to set the override logical height to -1 (no possible resolution).
> +    child->setOverrideContainingBlockContentLogicalHeight(-1);

Does this mean that we never clear the override?  Does that work correctly for relayout?

[Julien Chaffraix]

Comment on attachment 194604 [details]
Proposed change 1: Set our override logical height to -1 to prevent percent resolution.

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

>> Source/WebCore/rendering/RenderGrid.cpp:377
>> +    child->setOverrideContainingBlockContentLogicalHeight(-1);
> 
> Does this mean that we never clear the override?  Does that work correctly for relayout?

We clear any override from a previous layout here as we override it with -1 and will set the override to our grid area's size in RenderGrid::layoutGridItems. Also -1 is not a possible value for UsedBreadth so that should ensure a relayout in layoutGridItems for any content-sized grid items. This is an existing bug in our case, covered by a FIXME in layoutGridItems.

We have several test covering relaying out out our children:
* percent-grid-item-in-percent-grid-track-update.html
* percent-padding-margin-resolution-grid-item-update.html
* grid-dynamic-updates-relayout.html

[Tony Chang]

Comment on attachment 194604 [details]
Proposed change 1: Set our override logical height to -1 to prevent percent resolution.

Ojan clarified some things for me.  This looks fine.

[Tony Chang]

As an aside, we may want to clear the override containing block sizes after we use them to save some memory between layouts.

[Julien Chaffraix]

(In reply to comment #5)
> As an aside, we may want to clear the override containing block sizes after we use them to save some memory between layouts.

Filed bug 113118 about this idea as I don't know if we couldn't reuse them to avoid the extra layouts.

[Julien Chaffraix]

Created attachment 194665 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194665 [details]
Patch for landing

Clearing flags on attachment: 194665

Committed r146697: <http://trac.webkit.org/changeset/146697>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.