[css-grid] Content box incorrectly used as non-auto min-height
Created attachment 275023 [details] Patch
Comment on attachment 275023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275023&action=review The change looks good to me but I think we need more work on the test. > LayoutTests/fast/css-grid-layout/min-height-border-box.html:9 > + display:grid; This line is not needed, as it's already added by grid.css. Otherwise you'd be missing the prefix. > LayoutTests/fast/css-grid-layout/min-height-border-box.html:15 > + border: 5px solid blue; I'd remove the border here, as we've later the "border" class. So, IMHO we could be testing 4 things for each case: * "item" * "item border" * "item padding" * "item border padding" > LayoutTests/fast/css-grid-layout/min-height-border-box.html:21 > +h2 { background-color: cyan; } > +h3 { background-color: lightgreen; } We don't use colors usually here in other tests. > LayoutTests/fast/css-grid-layout/min-height-border-box.html:45 > + Why not checking "item padding" and "item border padding"? > LayoutTests/fast/css-grid-layout/min-height-border-box.html:59 > +<h3>min-height: 0px. grid height: auto; NO STRETCH</h3> Nit: I'd put "NO STRETCH" before the dot. As it's related to the grid item and not the grid container. > LayoutTests/fast/css-grid-layout/min-height-border-box.html:70 > +<div class="container"> > + <div class="grid" data-expected-width="150" data-expected-height="20"> > + <div class="item alignSelfStart" style="min-height: 0px;" data-expected-width="140" data-expected-height="35">XXXX</div> > + </div> > +</div> This is repeated twice, probably you're missing some class like "border", "padding" and "border padding". > LayoutTests/fast/css-grid-layout/min-height-border-box.html:98 > +<h3>min-height: 30px</h3> Add "grid height: auto;" as in previous cases. > LayoutTests/fast/css-grid-layout/min-height-border-box.html:111 > +<h3>min-height: 30px NO STRETCH</h3> Ditto.
Created attachment 275543 [details] Improved test
Comment on attachment 275543 [details] Improved test Thanks for the new test. LGTM!
Comment on attachment 275543 [details] Improved test Clearing flags on attachment: 275543 Committed r199153: <http://trac.webkit.org/changeset/199153>
All reviewed patches have been landed. Closing bug.