RESOLVED FIXED 151323
[CSS Grid Layout] inline margins not honored when not using stretch in row-axis alignment 
https://bugs.webkit.org/show_bug.cgi?id=151323
Summary [CSS Grid Layout] inline margins not honored when not using stretch in row-ax...
Javier Fernandez
Reported 2015-11-16 14:13:25 PST
Created attachment 265615 [details] Case using min-width auto It seems that when using alignment vales different than 'stretch' we don't respect inline maxis when placing the grid item inside its grid area. There are similar issues even using 'stretch' when the item's minimum preferred width is bigger than its grid area and grid item's min-width property is set to 'auto' (default). Attached some examples to reproduce these issues.
Attachments
Case using min-width auto (265 bytes, text/html)
2015-11-16 14:13 PST, Javier Fernandez 		no flags
Details
Case using no-stretch value (300 bytes, text/html)
2015-11-16 14:14 PST, Javier Fernandez 		no flags
Details
Patch (49.70 KB, patch)
2015-11-16 14:30 PST, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (49.79 KB, patch)
2015-11-18 03:44 PST, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Javier Fernandez
Comment 1 2015-11-16 14:14:19 PST
Created attachment 265616 [details] Case using no-stretch value
Javier Fernandez
Comment 2 2015-11-16 14:30:27 PST
Created attachment 265620 [details] Patch
Sergio Villar Senin
Comment 3 2015-11-18 01:14:14 PST
Comment on attachment 265620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265620&action=review Terrific simplification, I like it a lot. Consider my comments before landing. > Source/WebCore/rendering/RenderBox.cpp:2394 > + // TODO (lajava) Move this logic to the LayoutGrid class. This comment is Blink's style, we use FIXME. In any case I don't see how we could move this without altering too much the flow as it's in between computations of this method... Also as in the original block of code you could store minPreferredLogicalWidth() and use it twice > Source/WebCore/rendering/RenderGrid.cpp:287 > + Would it make sense to define just one function selfAlignmentChangedFromStretch() and pass the direction as an argument ? > Source/WebCore/rendering/RenderGrid.cpp:312 > + } Remove the { }, not needed for a one liner. I'm starting to wonder whether doing all these checks is actually faster than relaying out... > Source/WebCore/rendering/RenderGrid.cpp:1645 > + ASSERT(child.overrideContainingBlockContentLogicalHeight()); Why just the height? > Source/WebCore/rendering/RenderGrid.cpp:1649 > + child.clearOverrideLogicalContentHeight(); Ditto. > LayoutTests/fast/css-grid-layout/grid-item-auto-sized-align-justify-margin-border-padding.html:3 > +<head> No need for <html> and/or <head> > LayoutTests/fast/css-grid-layout/grid-item-auto-sized-align-justify-margin-border-padding.html:69 > +} I added a grid-alignment.css some time ago. You might find several of these ones there. If not I think it's better to add them to that css file so other tests could use them in the future without having to replicate them over and over again. > LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html:2 > +<html> Ditto.
Sergio Villar Senin
Comment 4 2015-11-18 01:16:30 PST
Comment on attachment 265620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265620&action=review > Source/WebCore/rendering/RenderBox.cpp:2532 > + bool hasAutoSizeInRowAxis = isHorizontalWritingMode() ? style().width().isAuto() : style().height().isAuto(); Use logicalWidth() here, that's what it is for.
Javier Fernandez
Comment 5 2015-11-18 03:33:57 PST
Comment on attachment 265620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265620&action=review >> Source/WebCore/rendering/RenderGrid.cpp:287 >> + > > Would it make sense to define just one function selfAlignmentChangedFromStretch() and pass the direction as an argument ? I'm not sure about this. I rally like the "Row/Column axis" terminology, which is the opposite to the GridTrackSizingDirection. We could just create another enumeration, but I'm not sure it worths. >> Source/WebCore/rendering/RenderGrid.cpp:312 >> + } > > Remove the { }, not needed for a one liner. > > I'm starting to wonder whether doing all these checks is actually faster than relaying out... It seems that multiline conditions require { } About the checks, they are not so expensive, just accessing some Rare style attributes from self and parent RenderStyle instances. My guess is that it's way cheaper than forcing the layout on every children. However, we could try to gather some performance measurements if anybody thinks it´s necessary. >> Source/WebCore/rendering/RenderGrid.cpp:1645 >> + ASSERT(child.overrideContainingBlockContentLogicalHeight()); > > Why just the height? We only stretch in the column size now, hence we need just the grid area's height. >> Source/WebCore/rendering/RenderGrid.cpp:1649 >> + child.clearOverrideLogicalContentHeight(); > > Ditto. Same reason as above.
Javier Fernandez
Comment 6 2015-11-18 03:44:35 PST
Created attachment 265742 [details] Patch Applied suggested changes
WebKit Commit Bot
Comment 7 2015-11-18 04:53:30 PST
Comment on attachment 265742 [details] Patch Clearing flags on attachment: 265742 Committed r192573: <http://trac.webkit.org/changeset/192573>
WebKit Commit Bot
Comment 8 2015-11-18 04:53:33 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.