Bug 102968

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

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

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

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?

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.

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

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

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

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

Created attachment 176374 [details] Patch for landing

Comment on attachment 176374 [details] Patch for landing Clearing flags on attachment: 176374 Committed r135965: <http://trac.webkit.org/changeset/135965>

All reviewed patches have been landed. Closing bug.