Bug 113085

Summary: [CSS Grid Layout] content-sized row tracks with percentage logical height grid items don't resolve properly
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, esprehn+autocc, ojan.autocc, ojan, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Proposed change 1: Set our override logical height to -1 to prevent percent resolution.
none
Patch for landing none

Description Julien Chaffraix 2013-03-22 11:19:21 PDT
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).
Comment 1 Julien Chaffraix 2013-03-22 11:25:15 PDT
Created attachment 194604 [details]
Proposed change 1: Set our override logical height to -1 to prevent percent resolution.
Comment 2 Tony Chang 2013-03-22 14:07:30 PDT
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?
Comment 3 Julien Chaffraix 2013-03-22 14:37:25 PDT
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
Comment 4 Tony Chang 2013-03-22 16:50:36 PDT
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.
Comment 5 Tony Chang 2013-03-22 16:51:30 PDT
As an aside, we may want to clear the override containing block sizes after we use them to save some memory between layouts.
Comment 6 Julien Chaffraix 2013-03-22 17:23:03 PDT
(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.
Comment 7 Julien Chaffraix 2013-03-22 17:27:30 PDT
Created attachment 194665 [details]
Patch for landing
Comment 8 WebKit Review Bot 2013-03-22 18:09:44 PDT
Comment on attachment 194665 [details]
Patch for landing

Clearing flags on attachment: 194665

Committed r146697: <http://trac.webkit.org/changeset/146697>
Comment 9 WebKit Review Bot 2013-03-22 18:09:48 PDT
All reviewed patches have been landed.  Closing bug.