| Summary: | [CSS Grid Layout] inline margins not honored when not using stretch in row-axis alignment | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Javier Fernandez <jfernandez> | ||||||||||
| Component: | Layout and Rendering | Assignee: | Javier Fernandez <jfernandez> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | commit-queue, esprehn+autocc, glenn, kondapallykalyan, rego, simon.fraser, svillar | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 265616 [details]
Case using no-stretch value
Created attachment 265620 [details]
Patch
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. 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. 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. Created attachment 265742 [details]
Patch
Applied suggested changes
Comment on attachment 265742 [details] Patch Clearing flags on attachment: 265742 Committed r192573: <http://trac.webkit.org/changeset/192573> All reviewed patches have been landed. Closing bug. |
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.