Bug 155946

Summary: [css-grid] Content box incorrectly used as non-auto min-height
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, jfernandez, kling, koivisto, kondapallykalyan, rego
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Improved test none

Sergio Villar Senin
Reported 2016-03-28 09:26:00 PDT
[css-grid] Content box incorrectly used as non-auto min-height
Attachments
Patch (8.21 KB, patch)
2016-03-28 09:29 PDT, Sergio Villar Senin
no flags
Improved test (8.18 KB, patch)
2016-04-04 02:55 PDT, Sergio Villar Senin
no flags
Sergio Villar Senin
Comment 1 2016-03-28 09:29:39 PDT
Manuel Rego Casasnovas
Comment 2 2016-03-29 01:31:12 PDT
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.
Sergio Villar Senin
Comment 3 2016-04-04 02:55:15 PDT
Created attachment 275543 [details] Improved test
Manuel Rego Casasnovas
Comment 4 2016-04-04 03:24:38 PDT
Comment on attachment 275543 [details] Improved test Thanks for the new test. LGTM!
WebKit Commit Bot
Comment 5 2016-04-07 03:23:54 PDT
Comment on attachment 275543 [details] Improved test Clearing flags on attachment: 275543 Committed r199153: <http://trac.webkit.org/changeset/199153>
WebKit Commit Bot
Comment 6 2016-04-07 03:23:57 PDT
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.