Bug 102968 – Support proper <percent> / calc() resolution for grid items

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

[Julien Chaffraix]

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).

[Julien Chaffraix]

Created attachment 175498 [details]
Proposed change: Size the RenderGrid to the grid's area so properly compute the size.

[Build Bot]

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

[Julien Chaffraix]

Created attachment 175533 [details]
Same patch but fixed a dumb typo in expected files.

[Ojan Vafai]

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?

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 175696 [details]
Updated patch. Needs other bug to apply and pass.

[Ojan Vafai]

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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 176064 [details]
Proposed change 3: Added a comment about box-sizing and updated to fully pass the tests.

[Ojan Vafai]

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.

[Julien Chaffraix]

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();

[Julien Chaffraix]

Created attachment 176374 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 176374 [details]
Patch for landing

Clearing flags on attachment: 176374

Committed r135965: <http://trac.webkit.org/changeset/135965>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.