Bug 102968

Summary: Support proper <percent> / calc() resolution for grid items
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, eric, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 103075    
Bug Blocks: 60731    
Attachments:
Description Flags
Proposed change: Size the RenderGrid to the grid's area so properly compute the size.
none
Same patch but fixed a dumb typo in expected files.
none
Updated patch. Needs other bug to apply and pass.
none
Proposed change 3: Added a comment about box-sizing and updated to fully pass the tests.
none
Patch for landing none

Description Julien Chaffraix 2012-11-21 11:47:59 PST
We currently resolve the <percent> / calc() lengths against the grid element's logical width / height.

The specification says that we should size them according to the grid area's size (ie taking the grid track sizing into account).
Comment 1 Julien Chaffraix 2012-11-21 12:06:35 PST
Created attachment 175498 [details]
Proposed change: Size the RenderGrid to the grid's area so properly compute the size.
Comment 2 Build Bot 2012-11-21 15:12:37 PST
Comment on attachment 175498 [details]
Proposed change: Size the RenderGrid to the grid's area so properly compute the size.

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

New failing tests:
fast/css-grid-layout/calc-resolution-grid-item.html
Comment 3 Julien Chaffraix 2012-11-21 15:15:13 PST
Created attachment 175533 [details]
Same patch but fixed a dumb typo in expected files.
Comment 4 Ojan Vafai 2012-11-21 15:39:44 PST
Comment on attachment 175533 [details]
Same patch but fixed a dumb typo in expected files.

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

Confused...

> Source/WebCore/rendering/RenderGrid.cpp:180
> +        setLogicalWidth(columnTracks[columnTrack].m_usedBreadth);

I don't think this is right. Where in the spec does it say that the grids width is sized to it's contents?

> Source/WebCore/rendering/RenderGrid.cpp:193
> +    setLogicalWidth(currentLogicalWidth);
> +    setLogicalHeight(currentLogicalHeight);

Why do we set the width and height just to set them back to their original values?
Comment 5 Julien Chaffraix 2012-11-22 09:54:08 PST
We discussed the patch with Ojan and we need to cheat by returning the grid area's size when the grid item tries to query their containing block's size.

We will introduce a mechanism similar to what we do to override logical width / height but for the containing block. Reading the code, this concept could probably serve 2 other areas: tables (for caption sizing) and multi-column renderer.
Comment 6 Julien Chaffraix 2012-11-22 10:57:12 PST
Created attachment 175696 [details]
Updated patch. Needs other bug to apply and pass.
Comment 7 Ojan Vafai 2012-11-22 11:54:26 PST
Comment on attachment 175696 [details]
Updated patch. Needs other bug to apply and pass.

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

> Source/WebCore/rendering/RenderGrid.cpp:169
> +        child->setOverrideContainingBlockContentLogicalWidth(columnTracks[columnTrack].m_usedBreadth);
> +        child->setOverrideContainingBlockContentLogicalHeight(rowTracks[rowTrack].m_usedBreadth);

Do you need to subtract border and padding width/height here? Are usedBreaths border box or padding box sizes? I would guess that they're border box sizes.

Actually, since you can't style the grid area, I suppose there can never be border/padding and this doesn't matter? Might be worth adding a comment to that effect if that's the case.

> LayoutTests/ChangeLog:10
> +        height). This is orthogonal to this change though.

I don't think this is orthogonal. You just need to hook RenderBox::perpendicularContainingBlockLogicalHeight as well to know about override containing block heights.
Comment 8 Julien Chaffraix 2012-11-26 13:07:57 PST
Comment on attachment 175696 [details]
Updated patch. Needs other bug to apply and pass.

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

>> Source/WebCore/rendering/RenderGrid.cpp:169
>> +        child->setOverrideContainingBlockContentLogicalHeight(rowTracks[rowTrack].m_usedBreadth);
> 
> Do you need to subtract border and padding width/height here? Are usedBreaths border box or padding box sizes? I would guess that they're border box sizes.
> 
> Actually, since you can't style the grid area, I suppose there can never be border/padding and this doesn't matter? Might be worth adding a comment to that effect if that's the case.

As you pointed out, which box size we use is irrelevant as the grid areas are not styled (no border, padding or margin).

>> LayoutTests/ChangeLog:10
>> +        height). This is orthogonal to this change though.
> 
> I don't think this is orthogonal. You just need to hook RenderBox::perpendicularContainingBlockLogicalHeight as well to know about override containing block heights.

Doh, I misread the code and thought we would fall back to using the containing block's style's logical height.
Comment 9 Julien Chaffraix 2012-11-26 14:22:43 PST
Created attachment 176064 [details]
Proposed change 3: Added a comment about box-sizing and updated to fully pass the tests.
Comment 10 Ojan Vafai 2012-11-27 14:33:11 PST
Comment on attachment 176064 [details]
Proposed change 3: Added a comment about box-sizing and updated to fully pass the tests.

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

Would be nice to have some test cases of percent grid items inside percentage sized grids and grid track. Even if we fail them now, I think it's fine to check them in.

> Source/WebCore/rendering/RenderBox.cpp:1321
>      if (!region)

Not sure how regions and grids are supposed to interact. But this might deserve a FIXME for the region case since it won't use the override.
Comment 11 Julien Chaffraix 2012-11-27 17:29:41 PST
Comment on attachment 176064 [details]
Proposed change 3: Added a comment about box-sizing and updated to fully pass the tests.

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

> Would be nice to have some test cases of percent grid items inside percentage sized grids and grid track. Even if we fail them now, I think it's fine to check them in.

I have added 2 tests. Percentage sized grid items inside percentage sized grid tracks inside a fixed sized or a percentage sized grid element.

>> Source/WebCore/rendering/RenderBox.cpp:1321
>>      if (!region)
> 
> Not sure how regions and grids are supposed to interact. But this might deserve a FIXME for the region case since it won't use the override.

Done:

// FIXME: It's unclear if a region's content should use the containing block's override logical width.
// If it should, the following line should call containingBlockLogicalWidthForContent.
LayoutUnit result = cb->availableLogicalWidth();
Comment 12 Julien Chaffraix 2012-11-27 17:35:29 PST
Created attachment 176374 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-11-27 18:17:25 PST
Comment on attachment 176374 [details]
Patch for landing

Clearing flags on attachment: 176374

Committed r135965: <http://trac.webkit.org/changeset/135965>
Comment 14 WebKit Review Bot 2012-11-27 18:17:29 PST
All reviewed patches have been landed.  Closing bug.